GNOME Bugzilla – Bug 677809
Regression: gtkiconview changes in 3.4.2 / master break cheese's thumbnail list
Last modified: 2012-07-04 16:31:31 UTC
Created attachment 216057 [details] Screenshot showing half thumbnails, caused by gtkiconview not using horizonontal scrolling. Hi, Cheese has either a vertical (single column iconview) or horizontal (single row iconview) list with thumbnails of (earlier) made photos / videos. The iconview changes gtk-3.4.2 break this list in horizontal mode. There are 2 problems with cheese's iconview use in horizontal mode, when upgrading to gtk-3.4.2: 1) Despite being embedded in a GtkScrolledWindow, it will never scroll in the horizontal direction, instead it will just try to fit into the width allocated for the GtkScrollWindow, removing columns on the left side of the thumbnails to make them fit. See the attached screenshot-half-thumbs.png, for how the thumbs should look see screenshot-full-thumbs.png This can be worked around by using gtk_scrolled_window_add_with_viewport (which should not be necessary with an iconview), but this workaround causes a crash with SIGFPE when there are no thumbnails, or when all present thumbnails get deleted, which may very well be the result of another bug. 2) When the thumbnail list is in horizontal mode, cheese does a set_columns(5000) on the iconview, so that all thumbs will end up in a single row. This works fine with gtk <= 3.4.1, but with >= 3.4.2, this no longer works, because it seems that gtkiconview no longer only uses the number of columns it actually needs, and thus tries to get the width needed for 5000 thumbnails, at which point nothing gets rendered. Reducing the column setting to number of thumbnails + 1 results in an empty thumbnail at the end of the list, "proving" my assumption that the problem is the iconview actually trying to draw 5000 columns even though it needs way less. One could argue this is actually a cheese bug and I've a patch ready for cheese to call set_columns with the exact number of thumbnails, instead of the "oh 5000 should be plenty" it is currently doing. Regards, Hans
Created attachment 216058 [details] screenshot showing the full thumbnails (in vertical mode)
Created attachment 216059 [details] [review] PATCH: cheese: set columns to actual number needed in horizontal mode In case it helps, here is the cheese patch for setting the number of columns to the actual needed number, rather then the semi-random value of 5000. Also if you're going to test this with cheese, be aware that cheese in Fedora-17 has some other bugs which causes it to crash or otherwise misbehave. I strongly suggest getting the gstreamer* updates from updates-testing which fix quite a few cheese issues.
I think the number of columns behavior is intended. It seemed unreasonable to me that the iconview would try to shrink as you remove icons when you had explicitly set a number of columns. I don't have a strong opinion on this though.
Created attachment 216060 [details] [review] iconview: Don't shrink items The previous code assumed that the width was always enough for more than one column, which is obviously not correct when a number of columns is hardcoded. With this patch, it will now always check that the width is enough and otherwise cause scrolling.
(In reply to comment #3) > I think the number of columns behavior is intended. It seemed unreasonable to > me that the iconview would try to shrink as you remove icons when you had > explicitly set a number of columns. I don't have a strong opinion on this > though. As said I'm fine with fixing cheese wrt this, I even already have a patch for this. It is the never doing horizontal scrolling and instead shrinking the thumbnails horizontally (and going on until they are 0 sized) otoh is something which really ought to be fixed, but I guess we agree on that :) Also something worth looking into is the sigfpe crash when there are no icons in the iconview. This only happens when added to the scrolledwindow with gtk_scrolled_window_add_with_viewport , which is probably a don't do that you silly. Still it seems a divide by 0 is lurking in some code paths, and this may be triggerable in another way too.
Can you attach a stacktrace for this? Seems like it should be rather easy to reproduce. I'd like to fix the issue and add a testcase for it if it's a GTK bug.
Created attachment 216069 [details] [review] New version of: PATCH: iconview: Don't shrink items Thanks for the gtk patch for this! I've tested it and unfortunately it did nto fix things. Here is a new version which does fix things, see the commit msg for details. While working on this I've also noticed 2 other small issues (or so I believe) I'll attach patches for those too.
Created attachment 216070 [details] [review] iconview: gtk_icon_view_compute_n_items_for_size bugfix
Created attachment 216071 [details] [review] iconview: gtk_icon_view_compute_n_items_for_size bugfix 2
Created attachment 216151 [details] [review] iconview: Avoid divide by 0 in gtk_icon_view_get_preferred_width_for_height (In reply to comment #6) > Can you attach a stacktrace for this? > I assume that with this you mean the divide by 0, I just generated a backtrace for you, and since the cause is quite obvious once one has the backtrace, I can do better then a backtrace :) So here is a patch fixing this.
Comment on attachment 216071 [details] [review] iconview: gtk_icon_view_compute_n_items_for_size bugfix 2 This is by design and was introduced in http://git.gnome.org/browse/gtk+/commit/?id=e133c6cb71e8416c5848ad06c680e6b5a0b9b16e Granted, it looks kinda weird, but it's by design, I swear!
Created attachment 216319 [details] [review] iconview: Don't shrink items The previous code assumed that the width was always enough for more than one column, which is obviously not correct when a number of columns is hardcoded. With this patch, it will now always check that the width is enough and otherwise cause scrolling.
Created attachment 216320 [details] [review] iconview: gtk_icon_view_compute_n_items_for_size bugfix While working on the "iconview: Don't shrink items" patch I noticed that gtk_icon_view_compute_n_items_for_size modifies the natural and minimum item sizes it got from gtk_icon_view_get_preferred_item_size when calculating the max number of items which will fit, but later on it checks against these sizes when calculating the item_size, and these checks expect these values to be unmodified. This patch fixes this by modifying the natural and minimum values in advance and doing all computations with modified values.
Created attachment 216321 [details] [review] iconview: Add documentation abour different sizes Did I mention I hate code that has different kinds of "boxes" or coordinate systems but no clear way to distinguish them? It's all ints here...
Created attachment 216322 [details] [review] iconview: Include column spacing in calculation The function is probably almost never used, so nobody has ever seen this bug, but we should still get it right.
Created attachment 216325 [details] [review] iconview: clamp item size to be in [minimum, natural] Minimum size is necessary so you can see the item. If we can't get that we need to scroll. Natural as the maximum is used so that the spacing between items doesn't increase when resizing the iconview, but empty space is added to the right/bottom instead.
Review of attachment 216321 [details] [review]: this is useful, thanks
Review of attachment 216319 [details] [review]: ok
Review of attachment 216320 [details] [review]: ok
Review of attachment 216322 [details] [review]: good catch
Review of attachment 216325 [details] [review]: ok
Review of attachment 216151 [details] [review]: ::: gtk/gtkiconview.c @@ +1552,3 @@ + *max_items = (n_items + priv->columns - 1) / priv->columns; + if (*max_items == 0) + *max_items = 1; I would probably save some lines here and just add MAX(1,...) to the rhs.
(In reply to comment #22) > Review of attachment 216151 [details] [review]: > > ::: gtk/gtkiconview.c > @@ +1552,3 @@ > + *max_items = (n_items + priv->columns - 1) / priv->columns; > + if (*max_items == 0) > + *max_items = 1; > > I would probably save some lines here and just add MAX(1,...) to the rhs. The patch is actually wrong. Because the number of rows for 0 items should be 0 and not 1. And this would cause computations to reserve space for 1 row (which would be 0 pixels cell size, but would include the padding), when instead it could have a 0x0 size request. Now, the fun question is "How much space should an empty icon view with 5 columns request?" Should it request space for 0 columns and 0 rows or should it request space for 5 columns and 1 row? (We currently reserve 5 columns and 0 rows.) I'd want to say "use 5x1", but that doesn't work because we can't usually request space for those as we have no clue about the size of cells, because we can't check the cell renderers' sizes if we have 0 items. Also, there's a second code path for the "columns isn't set" case. That code path computes space for as many items as possible. But as said above, with 0 items we don't know the item size, so we get a completely bogus number. (It also potentially leads to a bunch of other div-by-0 errors down the line when cell sizes are returned as 0 and we then divide by them.) So this whole code path needs some fixes for the 0 item case. And of course figuring out how many items fir in a given size if you don't know the item size. :)
Comment on attachment 216319 [details] [review] iconview: Don't shrink items Committed this to master with the margin fix Hans mentioned above.
Patches as committed look good. I assume that since the original iconview changes causing this regression are in 3.4, that these fixed will be cherry-picked into 3.4 too ?
Yes. I'll do that once I've fixed the div by 0 issues.
Seeing the same problem with eog's image collection (actually even worse as we use G_MAXINT for icon_view_set_columns). Using the code in master right now seems to work, if _set_columns is used with the right column count (= number of items in view). Do I understand correctly that that's the expected behaviour with the new code (and eog needs correction too)?
(In reply to comment #27) > Do I understand correctly > that that's the expected behaviour with the new code (and eog needs correction > too)? Yes, eog needs correction too.
I just pushed the patches to the gtk-3-4 branch (including the ones that never showed up here for the div-by-0 issues). It would be nice if you could make sure that they work and work well with your apps.
Hi, (In reply to comment #29) > I just pushed the patches to the gtk-3-4 branch (including the ones that never > showed up here for the div-by-0 issues). > It would be nice if you could make sure that they work and work well with your > apps. I've run various tests with cheese with the 3.4 branch and things are looking good! I'm looking forward to the next gtk+-3.4.x release. Regards, Hans
*** Bug 679365 has been marked as a duplicate of this bug. ***