Retitling to include other similar complaints about the button.

Bug 690257 suggests that it should be part of the list instead of being a button at the bottom of the list.

*** Bug 690257 has been marked as a duplicate of this bug. ***

We should be using GtkScrolledWindow::edge-overshot. See 738881

Created attachment 293617 [details] [review]
view-container: Replace "Load More" button with edge hit detection

Review of attachment 293617 [details] [review]:

Thanks for the patch, Pranav. Looks good except a few minor niggles.

::: src/photos-view-container.c
@@ +78,3 @@
 static void
+photos_view_container_view_changed (PhotosViewContainer *self,
+                                    GtkPositionType pos)

This should be on the same line.

@@ +81,2 @@
 {
   PhotosViewContainerPrivate *priv = self->priv;

We can remove this because we are using priv in only one place.

@@ +90,3 @@
 photos_view_container_connect_view (PhotosViewContainer *self)
 {
   PhotosViewContainerPrivate *priv = self->priv;

Ditto.

Created attachment 293864 [details] [review]
view-container: Replace "Load More" button with edge hit detection

I took the liberty to fix the above issues and committed it. Thanks again.

Created attachment 293866 [details] [review]
view-container: code cleanup

Use g_signal_connect_swapped instead of g_signal_connect_object

Review of attachment 293866 [details] [review]:

Thanks for working on this, Pranav.

The commit message should be tweaked a bit. We usually use a capital letter after the ':' and it should be "view-container: Clean up" to match other similar commits.

Other than that, it would be good to explain why g_signal_connect_swapped is enough. It is obvious from reading the patch that we are replacing g_signal_connect_object with g_signal_connect_swapped, so there is no need to repeat that. The actual reason is not so obvious, especially because we chose to introduce g_signal_connect_object in a previous commit.

::: src/photos-view-container.c
@@ +336,3 @@
+                            "count-changed",
+                            G_CALLBACK (photos_view_container_count_changed),
+                            self);

This one should still be g_signal_connect_object because priv->offset_cntrlr can out live the PhotosViewContainer instance (ie. self) because PhotosOffsetControllers are global and will be alive as long as any object holds a reference to it. So, we need to use g_signal_connect_object to make sure that self does not continue to receive 'count-changed' emissions after it has been destroyed. Otherwise photos_view_container_count_changed will be invoked with an invalid self pointer.

@@ +341,3 @@
+                            "query-error",
+                            G_CALLBACK (photos_view_container_query_error),
+                            self);

Ditto.

Created attachment 293914 [details] [review]
view-container: Clean up

Review of attachment 293914 [details] [review]:

Perfect. Just add the URL to the bug in the commit message before pushing.

Review of attachment 293914 [details] [review]:

Just noticed that the commit message is more than 72 characters long. See https://wiki.gnome.org/Git/CommitMessages

Created attachment 293950 [details] [review]
clean up

Fixed the length of the commit message.

Review of attachment 293950 [details] [review]:

Looks good.

Review of attachment 293864 [details] [review]:

There is one bug that I failed to notice:

::: src/photos-load-more-button.c
@@ -66,3 @@
-  remaining = photos_offset_controller_get_remaining (priv->offset_cntrlr);
-  visible = !(remaining <= 0 || priv->block);
-  gtk_widget_set_visible (GTK_WIDGET (self), visible);

We have now lost this logic that prevented us from jumping off the edge. It is now possible to try to load more items than there is (and if you do that you get an empty view)

Created attachment 312834 [details] [review]
offset-controller: Prevent loading more items than there actually is

Review of attachment 312834 [details] [review]:

It makes sense to me.

(In reply to Debarshi Ray from comment #15)
> We have now lost this logic that prevented us from jumping off the edge. It
> is now possible to try to load more items than there is (and if you do that
> you get an empty view)

I was wrong about getting an empty view. That was caused by an unrelated change sitting in my tree, but we were still issuing two needless queries (one to get the items and one to count them), and it is always good to avoid those.