After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 382544 - GtkIconView: Selection/focus should be painted around the item, not just the text
GtkIconView: Selection/focus should be painted around the item, not just the ...
Status: RESOLVED FIXED
Product: gtk+
Classification: Platform
Component: Widget: GtkIconView
2.10.x
Other Linux
: Normal normal
: ---
Assigned To: gtk-bugs
gtk-bugs
Depends on:
Blocks:
 
 
Reported: 2006-12-05 09:35 UTC by Tommi Komulainen
Modified: 2008-09-24 08:06 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
screenshot with focus around text (6.93 KB, image/png)
2006-12-05 09:36 UTC, Tommi Komulainen
  Details
screenshot of focus around the whole item (5.86 KB, image/png)
2006-12-05 09:37 UTC, Tommi Komulainen
  Details
mockup for new/alternative selection for iconview (21.77 KB, image/png)
2006-12-08 14:28 UTC, Tuomas Kuosmanen
  Details
Selection around the whole item. (6.05 KB, patch)
2007-01-25 14:34 UTC, Lucas Rocha
none Details | Review
New version of patch with a minor change in gtk_icon_view_get_path_at_pos to make it consider the whole item area, not only its cells. (6.31 KB, patch)
2007-02-20 15:51 UTC, Lucas Rocha
none Details | Review
Slightly modified patch allowing theming (I think) (6.34 KB, patch)
2007-06-20 16:57 UTC, Denis Washington
needs-work Details | Review
Updated patch to fix points one and two (8.26 KB, patch)
2007-09-27 10:25 UTC, Johannes Schmid
none Details | Review
Updated GtkIconView selection patch (10.34 KB, patch)
2007-12-14 12:02 UTC, Denis Washington
none Details | Review
gtk-engines patch (3.18 KB, patch)
2007-12-14 12:05 UTC, Denis Washington
none Details | Review
Screenshot of the last two patches in action (93.15 KB, image/png)
2007-12-14 12:06 UTC, Denis Washington
  Details
Updated GtkIconView selection patch (alternative click handling) (10.20 KB, patch)
2007-12-14 13:14 UTC, Denis Washington
none Details | Review
Updated GtkIconView selection patch (3) (12.50 KB, patch)
2007-12-22 16:09 UTC, Denis Washington
none Details | Review
Screenshot of the patch with horizontal layout (42.44 KB, image/png)
2007-12-22 16:15 UTC, Denis Washington
  Details
Screenshot of patch with clearlooks (92.79 KB, image/png)
2008-01-10 00:05 UTC, John Spray
  Details
GtkIconView selection patch, updated against trunk (13.28 KB, patch)
2008-07-04 09:59 UTC, Christian Dywan
needs-work Details | Review
GtkIconView patch without "item-padding" (9.17 KB, patch)
2008-08-24 16:29 UTC, Denis Washington
needs-work Details | Review
GtkIconView patch iteration N (10.50 KB, patch)
2008-09-10 18:48 UTC, Denis Washington
none Details | Review
The last patch with keyboard navigation drawing fixed (11.38 KB, patch)
2008-09-11 16:03 UTC, Denis Washington
none Details | Review
updated patch (11.79 KB, patch)
2008-09-15 19:12 UTC, Kristian Rietveld
none Details | Review
Updated patch with selection behaivior changes moved to gtk_icon_view_button_press() (12.34 KB, patch)
2008-09-16 10:02 UTC, Denis Washington
accepted-commit_now Details | Review

Description Tommi Komulainen 2006-12-05 09:35:47 UTC
Currently the selection and focus for an item is painted only around the text part. This of course won't work for icons only (though I didn't check) and is not so visually distinct for short labels, and when the label is next to the icon it just looks bad.

Therefore I'd suggest the selection/focus rectangle to be drawn around the whole item, and not just the text.
Comment 1 Tommi Komulainen 2006-12-05 09:36:50 UTC
Created attachment 77715 [details]
screenshot with focus around text
Comment 2 Tommi Komulainen 2006-12-05 09:37:54 UTC
Created attachment 77716 [details]
screenshot of focus around the whole item
Comment 3 Tuomas Kuosmanen 2006-12-08 14:28:18 UTC
Created attachment 77963 [details]
mockup for new/alternative selection for iconview

For the cairo-candy heads, we could do even something like this :-)

In any case, I never really liked the way how icons are "darkened" by the hilight  - it's subtle enough that its not really noticeable (try it with something like slideshow thumbnail list or such without labels) and yet it makes the pictures "muddy" looking.

I'd like to see this kind of "hilight the background with a bit of border around the icon" -approach.
Comment 4 Matthias Clasen 2007-01-03 18:19:55 UTC
Looks shiny indeed. Of course, you'd really want the theme engine to be able
to draw that. Regarding the focus drawing, we have the same complication as
treeview columns have: there can be multiple editable/activatable cells per 
item, in which case we need to draw the focus rectangle around individual cells.
We should probably do the same thing the treeview column code does, and draw
the focus around the whole item as long as there is at most one editable/activatable cell.
Comment 5 Tuomas Kuosmanen 2007-01-15 14:06:26 UTC
    And of course, to clarify: The diagonally touching corners of the discontinuous
    selection in the mockup are not to be taken literally, it can just as well be
    not touching in the corner, it just looked cute when drawing it :)

    The "select around the icon *and* label" is the important thing. I don't like
    how currently the icon is "painted" with the selection colour, it makes it look
    strange. Better to just draw a background color area around the icon and label
    with a nice margin.

Comment 6 Alexander “weej” Jones 2007-01-15 15:51:56 UTC
Hot. Hot, hot, hot.

OK a few things to realise this mockup:

1. Font hinting disabled (it looks so much better!) :P

2. The icon view needs to have fixed size tiles. I think this will be extremely beneficial, as it introduces some amount of consistency when, say, browsing a folder with Nautilus. Currently you only seem to have to add as much as an emblem to a file to get the whole layout to look horrific, and, e.g. PDF and image preview icons are ridiculously oversized in comparison to normal icons.

3. Have some method of being able to lasso a selection. This is the same issue that we have with tree view, in that there is no void space between active object space in which to initiate the drag selection. A control-click key combination would be a solution, but it is perhaps a little unfamiliar. The more I think about it, however, the more I believe it is more predictable. Perhaps a toolbar button for toggling drag-selection mode could switch on and off automatically when pressing control, and could be locked on by clicking. I think this would be pretty intuitive.
Comment 7 Lucas Rocha 2007-01-25 14:34:59 UTC
Created attachment 81192 [details] [review]
Selection around the whole item.

Ok, this is a kickoff patch to start discussion. I took into account the issues broght by Matthias. I tested the patch with the iconview test program in GTK+ and it works fine.
Comment 8 Thomas D Ahle 2007-02-20 14:44:09 UTC
Btw. KDE is implementing this for KDE 4 in their new filemanager, dolphin: http://www.ereslibre.es/?p=14
Comment 9 Lucas Rocha 2007-02-20 15:48:58 UTC
I've commited this patch to maemo-gtk-2-10 branch in maemo repository and we'll be testing it one of our applications from now on. Still expecting some feedback. :-)
Comment 10 Lucas Rocha 2007-02-20 15:51:26 UTC
Created attachment 82960 [details] [review]
New version of patch with a minor change in gtk_icon_view_get_path_at_pos to make it consider the whole item area, not only its cells.
Comment 11 Murray Cumming 2007-03-21 14:38:59 UTC
That last change will be particularly useful. Users have difficulty selecting items at the moment - they shouldn't have to know which parts are cell renderers.
Comment 12 Murray Cumming 2007-06-16 11:28:25 UTC
What's the feedback from your use of this patch in maemo's GTK+?

Lucas, you should be mentioning this in your weekly bug reports, so it can be reviewed an committed.

Comment 13 Denis Washington 2007-06-16 14:28:54 UTC
I realized that with the patch GtkIconView still makes selected pixbufs blueish. Shouldn't this be removed?
Comment 14 Lucas Rocha 2007-06-17 08:02:55 UTC
Hei Murray,

(In reply to comment #12)
> What's the feedback from your use of this patch in maemo's GTK+?

In Maemo, we're using GTK+ with this patch in Control Panel[1]. It's working perfectly so far.

> Lucas, you should be mentioning this in your weekly bug reports, so it can be
> reviewed an committed.

What weekly report? The GTK+ one?

[1] https://stage.maemo.org/svn/maemo/projects/haf/branches/hildon-control-panel/refactoring/
Comment 15 Murray Cumming 2007-06-17 10:52:17 UTC
Sorry, I was somehow confusing Xan and Lucas. But anyway, I suggest that you try to get it into that weekly report.
Comment 16 Lucas Rocha 2007-06-17 14:31:22 UTC
Nah! Don't confuse me with Xan. I'm much more handsome than him! :-P

Actually, Xan has already added this bug to the weekly report. AFAIK, Matthias is assigned to review the patch. 
Comment 17 Denis Washington 2007-06-20 16:57:57 UTC
Created attachment 90346 [details] [review]
Slightly modified patch allowing theming (I think)

This patch uses gtk_paint_flat_box() instead of cairo_rectangle() to draw the highlighting, in the hope that the detail argument ("icon_view_cell") makes everything more themeable.
Comment 18 Matthias Clasen 2007-07-11 19:41:55 UTC
I think painting the selection around the whole item is fine, in principle.
Some small things where the last patch needs more work:

- The focus rectangle occasionally extends outside the selection. I have
  seen this with testiconview when the focus is on a checkbox. That looks
  bad.

- With multi-selection, there are gaps between adjacent selected items.
  Do we want that ? I assume not.

- The pixbuf tinting was supposed to be removed ?
Comment 19 Johannes Schmid 2007-09-27 10:25:41 UTC
Created attachment 96278 [details] [review]
Updated patch to fix points one and two

This patch draws the selection rectangle around the whole item, including the spacing around it and thus there are no gaps within the selection and the focus rectangle also no longer extends the selection.

Anyway, with this patch there is no margin when items are selected which looks bad. I tried to get around this but did not succeed.

Also be warned that I have no real in-deep knowledge of GTK+ and it could just be crap. Anyway, I really would like to see this in 2.22!
Comment 20 Denis Washington 2007-12-03 19:06:45 UTC
(In reply to comment #18)
> - With multi-selection, there are gaps between adjacent selected items.
>   Do we want that ? I assume not.

I believe that removing the gaps is not necessarily a good idea, especially for themeing the selection rectangle. Imagine that e.g. Clearlooks likes to give each rectangle rounded corners; without gaps, this simply wouldn't look good. Modifying the expose handler to allow something like rounded corners for a _complete_ gapless selection would be far too complicated - one would need a hint for every combination of neighbour selections for each item (left-neighbour-selected, right-neighbor-selected, left-and-right-neighbor-selected, left-and-upper-right-neighbor-selected....), which is pretty overkill and not worth the effort IMO.

To make it short, I would go with gaps in the interest of easy themeability.
Comment 21 Alexander “weej” Jones 2007-12-03 19:34:25 UTC
Why combine the hints? Why not just have 8 compass-point-style hints? n-, ne-, e-, se-, s-, sw-, w-, nw-selected.

That's not too complicated.
Comment 22 Denis Washington 2007-12-04 07:06:42 UTC
(In reply to comment #21)
> Why combine the hints? Why not just have 8 compass-point-style hints? n-, ne-,
> e-, se-, s-, sw-, w-, nw-selected.
> 
> That's not too complicated.
> 

That doesn't solve all cases. Let's say the theme wants to draw a border about the selection, a not very uncommon task IMO. Now consider the following selection pattern:

+   +
+ + +
+   +

The selected item in the middle would need a border at the top and the bottom, but not left and right. None of your proposed hints would be able to express that; instead, the item would need a hint expressing that it's right and left-hand neighbour is selected, as I explained before.

Additionally, even simple background effects like gradients would fail as this gradient would start again in each rectangle, which would look really strange. It is extremely complex, if not impossible, to provide hints that would make a gapless selection-wide background gradient possible.

Keeping the gaps, on the other hand, would need just one single hint and give the theme designer much bigger freedom as each selection rectangle is an independent entity.
Comment 23 Alexander “weej” Jones 2007-12-04 19:44:12 UTC
That pattern is easy -- it's every hint except north and south. The drawing code then knows everything it needs to know.

The gradient/pattern continuity issue can be dealt with by communicating a pixel offset for the drawing.

I don't think any of this is remotely hard.
Comment 24 Denis Washington 2007-12-05 15:01:59 UTC
Is it possible to give multiple hints? from what i see in the description of gtk_paint_box(), just one single "detail" can be passed, i.e. a string hint. Even if this information could be all packed in this single string, implementing this for a theme would still be pretty complicated compared to selection rectangles with gaps, especially if pixel offsets etc would be embedded into this single hint.

I also don't think that gapless selection drawing is really that important that the increased complexity is justified.
Comment 25 Denis Washington 2007-12-14 12:02:34 UTC
Created attachment 100946 [details] [review]
Updated GtkIconView selection patch

I made the following changes in comparison to the old patch:

* Re-added gaps. For one they increase the complexity of themeing the selection rectangle as I already mentioned; second, the column and row spacing regions didn't seem to be properly redrawn in the last patch, which is particularly apparent when selecting items with a selection rectangle.

* Remove the icon tinting on selection.

* Implemented an "item-padding" property which specifies the padding between a selection rectangle and it's contents.

I will also attach a patch for gtk-engines that illustrates how the icon view selections could be themed (it patches the Clearlooks theme to draw rounded corners and a subtle gradient like e.g. for selected menu items). I'll also attach a screenshot of that patch in action.
Comment 26 Denis Washington 2007-12-14 12:05:08 UTC
Created attachment 100947 [details] [review]
gtk-engines patch

The gtk-engines patch mentioned in the last comment.
Comment 27 Denis Washington 2007-12-14 12:06:22 UTC
Created attachment 100948 [details]
Screenshot of the last two patches in action
Comment 28 Denis Washington 2007-12-14 13:14:05 UTC
Created attachment 100949 [details] [review]
Updated GtkIconView selection patch (alternative click handling)

This version of the patch has a slightly different way of handling clicks to select. As long as an item is NOT selected, only clicks on the cells themselves (the icon or the text) are recognized as clicks on the item; if the item IS selected, the click area for the item is the whole selection rectangle. This fixes the following problems with the last version:

* The user cannot make a intuitive connection between some space and the item that is selected when clicking between two items.

* It is extremly difficult to start a rectangle selection as most of the space is occupied by the item's area (it is not easy to hit the space between two items).
Comment 29 Teppo Turtiainen 2007-12-14 16:03:04 UTC
(In reply to comment #27)
> Screenshot of the last two patches in action

Looks very bland compared to the mockup Tuomas made.
Comment 30 Denis Washington 2007-12-14 16:15:39 UTC
True. But mockups are mockups - shiny but not easy to implement. ;)

Making the selection gapless is very hard to do as I already mentioned - I still don't think that we should add a lot of extra complexity to GtkIconView and theme engines just to have them. Implementation aside, having no gaps also brings usability issues; selecting a subsection of a previous selection with the mouse is impossible if there are no gaps in which you can start to drag a new selection rectangle.

The selection box around each item could be much shinier than in my screenshot, naturally. That was just a quick hack to show you more than ugly plain-colored rectangles.
Comment 31 Alexander “weej” Jones 2007-12-14 16:58:39 UTC
Good work. Of course, leave it to the theme engines to bling out. The gapless thing is, admittedly, pointless eyecandy that (more importantly than adding code complexity) adds usability confusion. (For example, it suggests that a dragged selection only includes the things that are visually joined to it when this clearly won't be the case.)
Comment 32 Thomas D Ahle 2007-12-14 17:04:03 UTC
Even though I think your shot looks much greater than the current nautilus, couldn't we borrow some of the architectual ideas from dolphin?

For the "selecting a subsection of a previous selection with
the mouse is impossible if there are no gaps in which you can start to drag a
new selection rectangle." - Couldn't that be done using Ctrl/Alt/Shift drags, like selecting in the gimp?
Comment 33 Denis Washington 2007-12-14 17:19:30 UTC
(In reply to comment #32)
> Even though I think your shot looks much greater than the current nautilus,
> couldn't we borrow some of the architectual ideas from dolphin?

Which architectural ideas do you mean exactly? I seen categories and other things in dolphin, but those are not directly related to this discussion and should probably go into another bug.

> For the "selecting a subsection of a previous selection with
> the mouse is impossible if there are no gaps in which you can start to drag a
> new selection rectangle." - Couldn't that be done using Ctrl/Alt/Shift drags,
> like selecting in the gimp?

Theoretically yes, but keyboard shortcuts are not that easily discoverable - for such a basic feature like selection that's pretty bad. With gaps, we simply don't have that problem.
Comment 34 Thomas D Ahle 2007-12-14 18:56:44 UTC
> Which architectural ideas do you mean exactly?
I naturally refer to the gapless icon selection in dolphin, already linked to in this bug.
Comment 35 Denis Washington 2007-12-15 10:23:36 UTC
(In reply to comment #34)
> > Which architectural ideas do you mean exactly?
> I naturally refer to the gapless icon selection in dolphin, already linked to
> in this bug.
> 

I tried the KDE Four Live CD (KDE 4.0 RC 2) - the current Dolphin doesn't have gapless selection.
Comment 36 Thomas D Ahle 2007-12-15 19:57:38 UTC
Ok, perhaps it was removed due to the usability concerns mentioned in this bug. It just seams like it has been implemented - http://www.ereslibre.es/?p=14
Comment 37 Denis Washington 2007-12-22 16:09:34 UTC
Created attachment 101461 [details] [review]
Updated GtkIconView selection patch (3)

This version of the patch adds the following fixes:

* Horizontal layout now also works - it was broken before because the item padding setting wasn't integrated tightly enough into the layouting code. I'll attach a horizontal layout screenshot as proof. ;)

* When editing text cells, the entry didn't appear at the same position as the text because the item padding was not honored. This is fixed now.

* Removed frame for text editing entries, as the selection rectangle now acts as frame (like it's the case when editing text in a tree view cell).

Unfortunately, I have the feeling that the text in the vertical layout is not 100% centered anymore. I still wasn't able to find out the cause of this small regression; it would be great if someone could test the patch and look at the code, maybe you find what is causing this. In general, testing is very appreciated. :)
Comment 38 Denis Washington 2007-12-22 16:15:52 UTC
Created attachment 101462 [details]
Screenshot of the patch with horizontal layout

This time with my default theme, a dark Clearlooks.
Comment 39 John Spray 2008-01-10 00:05:20 UTC
Created attachment 102499 [details]
Screenshot of patch with clearlooks

Tested svn trunk with my application and Denis's patch.  Seems to work well.
Comment 40 Alexander “weej” Jones 2008-01-10 02:14:40 UTC
Why is there a blank line at the top of the text cell?
Comment 41 John Spray 2008-01-10 10:12:41 UTC
It's added by the application, nothing to do with the iconview.
Comment 42 Thomas D Ahle 2008-01-10 14:32:26 UTC
Does the patch contain support for rounded rectanlgs?
Not speaking of collapsing spacing, just rounded selections like around the text today.
Comment 43 Denis Washington 2008-01-10 14:48:14 UTC
(In reply to comment #42)
> Does the patch contain support for rounded rectanlgs?
> Not speaking of collapsing spacing, just rounded selections like around the
> text today.

The patch draws it's selection with gtk_paint_flat_box() with an iconview-specific detail argument, which enables themes to draw this rectangle like they wish, including rounded corners and e.g. a gradient instead of a plain color. I hacked up a small patch for Clearlooks to show how this could be used - see the screenshot attached to comment #27.

P.S.: Has anyone tried the last patch with a vertical layout (text beneath icons) yet? I have the feeling the text is not completely centered, but couldn't find any logical mistake in my code. (The text cell renderer has exactly the correct position and size, I checked with a debug rectangle.) Maybe it's a bug somewhere else in gtk+, or I am just dreaming. Testing and/or help with that problem is very much appreciated.
Comment 44 Denis Washington 2008-01-20 11:05:07 UTC
Has anyone tested the patch?
Comment 45 Denis Washington 2008-04-27 10:08:44 UTC
What is the reviewing status of this? Any chance this patch could get into in the next GTK+ release?
Comment 46 Christian Dywan 2008-05-20 15:34:02 UTC
I tested the patch against trunk revision 20115 and it works fine.
Comment 47 Diego González 2008-05-20 23:34:37 UTC
Isn't it possible to get closer to the screenshot posted by Toumas or to the selection that is available in the KDE file manager?

http://media.ereslibre.es/2007/02/screenshot.png
Comment 48 Christian Dywan 2008-05-21 00:08:28 UTC
(In reply to comment #47)
> Isn't it possible to get closer to the screenshot posted by Toumas or to the
> selection that is available in the KDE file manager?
> 
> http://media.ereslibre.es/2007/02/screenshot.png
> 

How much closer can you be than this, without removing the gaps? As discussed in previous comments, removing the gaps would conciderably complicate the work of theme (engine) authors and also make it hard in terms of usability.
The rounding and gradients depend on the theme (engine) and will not be available unless you actually have one that already implements special styling for icon view items.
Comment 49 Christian Dywan 2008-06-06 08:58:29 UTC
What's the status on this? Positive or negative input appreciated.
Comment 50 Christian Dywan 2008-07-04 09:59:43 UTC
Created attachment 113968 [details] [review]
GtkIconView selection patch, updated against trunk

Updated the patch and removed an irrelevant include file change in gtkcellrenderertext.
Comment 51 Matthias Clasen 2008-07-05 20:43:35 UTC
Can we split this patch up ? 

Adding an item-padding property can certainly be handled independently.
Why do we need it ?


-      /* FIXME ugly special case */ 
-      if (GTK_IS_ENTRY (editable) || GTK_IS_COMBO_BOX (editable))
-	g_object_set (editable, "has-frame", TRUE, NULL);

Why is this not necessary anymore ?


@@ -1988,17 +2001,18 @@ gtk_icon_view_button_press (GtkWidget   
       item = gtk_icon_view_get_item_at_coords (icon_view, 
 					       event->x, event->y,
 					       TRUE,
-					       &info);      
+					       &info); 
       if (item != NULL)
 	{
-	  g_object_get (info->cell, "mode", &mode, NULL);
-	  
-	  if (mode == GTK_CELL_RENDERER_MODE_ACTIVATABLE ||
-	      mode == GTK_CELL_RENDERER_MODE_EDITABLE)
-	    cursor_cell = g_list_index (icon_view->priv->cell_list, info);
-	  else
-	    cursor_cell = -1;
-	  
+          if (info != NULL)
+            {
+	      g_object_get (info->cell, "mode", &mode, NULL);
+ 
+	      if (mode == GTK_CELL_RENDERER_MODE_ACTIVATABLE ||
+	          mode == GTK_CELL_RENDERER_MODE_EDITABLE)
+	        cursor_cell = g_list_index (icon_view->priv->cell_list, info);
+	    }

This looks like an independent bug fix...


@@ -5359,13 +5396,11 @@ update_pixbuf_cell (GtkIconView *icon_vi
 
 	if (icon_view->priv->orientation == GTK_ORIENTATION_VERTICAL)
 	  g_object_set (info->cell,
-			"follow-state", TRUE, 
 			"xalign", 0.5,
 			"yalign", 1.0,
 			NULL);
 	else
 	  g_object_set (info->cell,
-			"follow-state", TRUE, 
 			"xalign", 0.0,
 			"yalign", 0.0,
 			NULL);

I never saw a rationale for not doing the tinting anymore. Is this just a 
case of 'I like it better without' ?

Comment 52 Denis Washington 2008-07-06 09:14:13 UTC
(In reply to comment #51)
> Can we split this patch up ? 
> 
> Adding an item-padding property can certainly be handled independently.
> Why do we need it ?

"item-padding" is a complement to "row-spacing" and "column-spacing". While the latter two specify the size of the gaps between the items (and thus the selection rectangles), "item-padding" defines the space between the selection rectangle's outer bounds and the item's actual content. I thought it would be good to make this configurable.

> 
> -      /* FIXME ugly special case */ 
> -      if (GTK_IS_ENTRY (editable) || GTK_IS_COMBO_BOX (editable))
> -       g_object_set (editable, "has-frame", TRUE, NULL);
> 
> Why is this not necessary anymore ?

The current GtkIconView draws a frame around a text entry or combo box in edit mode. Because the selection rectangle around the entry/combo already gives these elements a border in the patch version, I removed this special case. If there are any corner cases where this is still needed, it could be re-added without problem, though.

> 
> @@ -1988,17 +2001,18 @@ gtk_icon_view_button_press (GtkWidget   
>        item = gtk_icon_view_get_item_at_coords (icon_view, 
>                                                event->x, event->y,
>                                                TRUE,
> -                                              &info);      
> +                                              &info); 
>        if (item != NULL)
>         {
> -         g_object_get (info->cell, "mode", &mode, NULL);
> -         
> -         if (mode == GTK_CELL_RENDERER_MODE_ACTIVATABLE ||
> -             mode == GTK_CELL_RENDERER_MODE_EDITABLE)
> -           cursor_cell = g_list_index (icon_view->priv->cell_list, info);
> -         else
> -           cursor_cell = -1;
> -         
> +          if (info != NULL)
> +            {
> +             g_object_get (info->cell, "mode", &mode, NULL);
> + 
> +             if (mode == GTK_CELL_RENDERER_MODE_ACTIVATABLE ||
> +                 mode == GTK_CELL_RENDERER_MODE_EDITABLE)
> +               cursor_cell = g_list_index (icon_view->priv->cell_list, info);
> +           }
> 
> This looks like an independent bug fix...

I must admit that I don't remember any more. But I think this was actually a new case which didn't appear in the original version (item != NULL && info == NULL), so in this case it isn't actually independent.

> @@ -5359,13 +5396,11 @@ update_pixbuf_cell (GtkIconView *icon_vi
> 
>         if (icon_view->priv->orientation == GTK_ORIENTATION_VERTICAL)
>           g_object_set (info->cell,
> -                       "follow-state", TRUE, 
>                         "xalign", 0.5,
>                         "yalign", 1.0,
>                         NULL);
>         else
>           g_object_set (info->cell,
> -                       "follow-state", TRUE, 
>                         "xalign", 0.0,
>                         "yalign", 0.0,
>                         NULL);
> 
> I never saw a rationale for not doing the tinting anymore. Is this just a 
> case of 'I like it better without' ?

Well, kind of. I felt that tinting was more of a hack to make the selection more visible, at the price of modifying the icon's appearance, which is unpractical if this icon is e.g. an image thumbnail. As the selection is now sufficiently visible through the selection rectangle, I saw no reason for destroying the icon's original appearance and thus removed tinting (which is consistent with GtkTreeView's or GtkMenuItem's, which, if selected, also don't tint icons).
Comment 53 Christian Dywan 2008-08-14 13:36:09 UTC
> > I never saw a rationale for not doing the tinting anymore. Is this just a 
> > case of 'I like it better without' ?
> 
> Well, kind of. I felt that tinting was more of a hack to make the selection
> more visible, at the price of modifying the icon's appearance, which is
> unpractical if this icon is e.g. an image thumbnail. As the selection is now
> sufficiently visible through the selection rectangle, I saw no reason for
> destroying the icon's original appearance and thus removed tinting (which is
> consistent with GtkTreeView's or GtkMenuItem's, which, if selected, also don't
> tint icons).

I totally second that. Tinting has the obvious problem that the selection is only so visible as the colour of the image doesn't visually merge with the tint.

Plus tinting assumes that the visible parts of images are sufficiently big, which fails if much of the image is transparent.

Both issues are not present with a selection painted around the image. The selection is always clearly visible and the image doesn't have to be altered, hence no risk of colour conflict.
Comment 54 Björn Lindqvist 2008-08-23 16:18:39 UTC
The item-padding property really needs to be split out unless there is a compelling use case for it. You could change it to a constant so that it becomes easy to add it if it needs to. Gtk is also api frozen atm. Other than that, it is a great patch!
Comment 55 Denis Washington 2008-08-24 16:29:09 UTC
Created attachment 117305 [details] [review]
GtkIconView patch without "item-padding"

The patch with the "item-padding" property taken out (the padding is now a constant set to 6 pixels).
Comment 56 Kristian Rietveld 2008-09-03 12:12:29 UTC
Comment on attachment 117305 [details] [review]
GtkIconView patch without "item-padding"

I like the idea of this patch.  Below will follow a couple of general and a couple of detailed comments on your patch.  Also, in general we really prefer patches generated with diff -up, since this makes reviewing the patch much easier.  So, please generate your patch using diff -up next time :)

The part of the patch that actually draws the selection around the entire icon view item seems to function well.  The other parts need more work -- the adaptions to the layouting code need restructuring and the editable cells and the focus rectangles are not drawn at the correct positions.  More detailed comments on these below.

I am not very experienced with icon view, please correct me if I am wrong :)


>Index: gtk/gtkiconview.c
>===================================================================
>--- gtk/gtkiconview.c	(revision 21191)
>+++ gtk/gtkiconview.c	(working copy)
>@@ -46,6 +46,7 @@
> #undef DEBUG_ICON_VIEW
> 
> #define SCROLL_EDGE_SIZE 15
>+#define ITEM_PADDING     6
> 
> #define GTK_ICON_VIEW_GET_PRIVATE(obj) (G_TYPE_INSTANCE_GET_PRIVATE ((obj), GTK_TYPE_ICON_VIEW, GtkIconViewPrivate))
> 
>@@ -1301,8 +1302,8 @@
>       /* totally ignore our child's requisition */
>       if (child->cell < 0)
> 	{
>-	  allocation.x = child->item->x;
>-	  allocation.y = child->item->y;
>+	  allocation.x = child->item->x + ITEM_PADDING;
>+	  allocation.y = child->item->y + ITEM_PADDING;
> 	  allocation.width = child->item->width;
> 	  allocation.height = child->item->height;
> 	}

Shouldn't ITEM_PADDING also be subtracted from (width, height) here?

>@@ -1310,8 +1311,8 @@
> 	{
> 	  GdkRectangle *box = &child->item->box[child->cell];
> 
>-	  allocation.x = box->x;
>-	  allocation.y = box->y;
>+	  allocation.x = box->x + ITEM_PADDING;
>+	  allocation.y = box->y + ITEM_PADDING;
> 	  allocation.width = box->width;
> 	  allocation.height = box->height;
> 	}

"box" contains the exact coordinates of the cell and should therefore already be corrected for item-padding during the layouting phase.  This patch chunk should be removed.

>@@ -1770,10 +1771,6 @@
> 						  0);
>       g_free (path_string);      
> 
>-      /* FIXME ugly special case */ 
>-      if (GTK_IS_ENTRY (editable) || GTK_IS_COMBO_BOX (editable))
>-	g_object_set (editable, "has-frame", TRUE, NULL);

I agree that this can be removed now we introduce the new background behavior.  In the tree view we don't draw the frame of the entry.

>@@ -1980,17 +1977,18 @@
>       item = gtk_icon_view_get_item_at_coords (icon_view, 
> 					       event->x, event->y,
> 					       TRUE,
>-					       &info);      
>+					       &info); 
>       if (item != NULL)
> 	{
>-	  g_object_get (info->cell, "mode", &mode, NULL);
>-	  
>-	  if (mode == GTK_CELL_RENDERER_MODE_ACTIVATABLE ||
>-	      mode == GTK_CELL_RENDERER_MODE_EDITABLE)
>-	    cursor_cell = g_list_index (icon_view->priv->cell_list, info);
>-	  else
>-	    cursor_cell = -1;
>-	  
>+          if (info != NULL)
>+            {
>+	      g_object_get (info->cell, "mode", &mode, NULL);
>+ 
>+	      if (mode == GTK_CELL_RENDERER_MODE_ACTIVATABLE ||
>+	          mode == GTK_CELL_RENDERER_MODE_EDITABLE)
>+	        cursor_cell = g_list_index (icon_view->priv->cell_list, info);
>+	    }

Matthias mentioned that this is probably an independent fix.  I think this is caused by the modification of gtk_icon_view_get_item_at_coords() below.  I don't think that modification is necessary and therefore these guards for info != NULL can also be removed.


> 	  gtk_icon_view_scroll_to_item (icon_view, item);
> 	  
> 	  if (icon_view->priv->selection_mode == GTK_SELECTION_NONE)
>@@ -2049,13 +2047,16 @@
> 
> 	  /* cancel the current editing, if it exists */
> 	  gtk_icon_view_stop_editing (icon_view, TRUE);
>-	  
>-	  if (mode == GTK_CELL_RENDERER_MODE_ACTIVATABLE)
>-	    gtk_icon_view_item_activate_cell (icon_view, item, info, 
>-					      (GdkEvent *)event);
>-	  else if (mode == GTK_CELL_RENDERER_MODE_EDITABLE)
>-	    gtk_icon_view_start_editing (icon_view, item, info, 
>-					 (GdkEvent *)event);	  
>+
>+          if (info != NULL)
>+            {
>+	      if (mode == GTK_CELL_RENDERER_MODE_ACTIVATABLE)
>+	        gtk_icon_view_item_activate_cell (icon_view, item, info, 
>+	          			          (GdkEvent *)event);
>+	      else if (mode == GTK_CELL_RENDERER_MODE_EDITABLE)
>+	        gtk_icon_view_start_editing (icon_view, item, info, 
>+	      				     (GdkEvent *)event);	  
>+            }

Same here.

>@@ -2730,6 +2731,7 @@
>       cell_area->width = item->box[info->position].width + 
> 	item->before[info->position] + item->after[info->position];
>       cell_area->height = item->height;
>+      cell_area->height -= ITEM_PADDING * 2;
>     }
>   else
>     {
>@@ -2738,7 +2740,11 @@
>       cell_area->width = item->width;
>       cell_area->height = item->box[info->position].height + 
> 	item->before[info->position] + item->after[info->position];
>+      cell_area->width -= ITEM_PADDING * 2;
>     }
>+
>+  cell_area->x += ITEM_PADDING;
>+  cell_area->y += ITEM_PADDING; 
> }

Just as the height is only corrected for the horizontal case, the y coordinate should probably only be corrected in the horizontal case too.  In the vertical case, the y coordinate is calculated from the (already corrected for item-padding) box coordinates.  Same for the x coordinate in the vertical case.

> static void 
>@@ -2750,6 +2756,8 @@
>   g_return_if_fail (info->position < item->n_cells);
> 
>   *box = item->box[info->position];
>+  box->x += ITEM_PADDING;
>+  box->y += ITEM_PADDING;
> }

AFAIK this should be unnecessary because the box coordinates are already corrected for item-padding.

>@@ -2854,6 +2864,9 @@
> 	  item->height += item->box[info->position].height + (info->position > 0 ? spacing : 0);
> 	}
>     }
>+
>+  item->width += ITEM_PADDING * 2;
>+  item->height += ITEM_PADDING * 2;
> }

Should be fine.

> static void
>@@ -2940,7 +2953,12 @@
> 	  item->box[i].x = item->x + item->width - 
> 	    (item->box[i].x + item->box[i].width - item->x);
> 	}      
>-    }	
>+    }
>+
>+  /* NOTE: increasing item->width by the item padding here
>+     causes the layouting code to crash, but doesn't seem
>+     to be needed anyway */  
>+  item->height += ITEM_PADDING * 2;
> }

This is asking for trouble.  In theory, we are only modifying how the items are being laid out internally -- the layouting algorithm of the items does not change and should not crash due to our modifications.  The modifications to the function should be done differently, you want to correct the cell_area that is being set up for item-padding.  Next, the corrected cell_area will be used to calculate the actual positions of the cells contained in the item with the results saved to "box".  The "box" coordinates are now calculated correctly and then we should not have a need for the change to gtk_icon_view_get_cell_box() above.  item->(width, height) are recalculated in this function and we should make sure they (obviously) end up being cell_area.(width, height) + 2 * item-padding.


>@@ -3066,42 +3073,56 @@
> 				drawable,
> 				GTK_WIDGET (icon_view),
> 				&cell_area, &cell_area, area, flags);
>-      
>     }
> 
>-  /* FIXME where to draw the focus with generic cell renderers ? */
>   if (draw_focus && 
>       GTK_WIDGET_HAS_FOCUS (icon_view) &&
>       item == icon_view->priv->cursor_item)
>-    for (l = icon_view->priv->cell_list, i = 0; l; l = l->next, i++)
>-      {
>-	GtkIconViewCellInfo *info = (GtkIconViewCellInfo *)l->data;
>-	
>-	if (!info->cell->visible)
>-	  continue;
>-	
>-	if (icon_view->priv->cursor_cell < 0 &&
>-	    (info->cell->mode != GTK_CELL_RENDERER_MODE_INERT ||
>-	     GTK_IS_CELL_RENDERER_TEXT (info->cell)))
>-	  icon_view->priv->cursor_cell = i;
>-	
>-	if (i == icon_view->priv->cursor_cell)
>-	  {
>-	    gtk_icon_view_get_cell_box (icon_view, item, info, &box);
>+    {
>+      for (l = icon_view->priv->cell_list, i = 0; l; l = l->next, i++)
>+        {
>+          GtkIconViewCellInfo *info = (GtkIconViewCellInfo *)l->data;
>+          
>+          if (!info->cell->visible)
>+            continue;
> 
>-	    gtk_paint_focus (GTK_WIDGET (icon_view)->style,
>-			     drawable,
>-			     GTK_STATE_NORMAL,
>-			     area,
>-			     GTK_WIDGET (icon_view),
>-			     "icon_view",
>-			     x - item->x + box.x - padding,
>-			     y - item->y + box.y - padding,
>-			     box.width + 2 * padding,
>-			     box.height + 2 * padding);
>-	    break;
>-	  }
>-      }
>+          /* If found a editable/activatable cell, draw focus on it. */          
>+          if (icon_view->priv->cursor_cell < 0 &&
>+              info->cell->mode != GTK_CELL_RENDERER_MODE_INERT)
>+            icon_view->priv->cursor_cell = i;
>+
>+          gtk_icon_view_get_cell_box (icon_view, item, info, &box);
>+          
>+          if (i == icon_view->priv->cursor_cell)
>+            {
>+              gtk_paint_focus (GTK_WIDGET (icon_view)->style,
>+          		     drawable,
>+          		     GTK_STATE_NORMAL,
>+          		     area,
>+          		     GTK_WIDGET (icon_view),
>+          		     "icon_view",
>+                             x - item->x + box.x - padding,
>+                             y - item->y + box.y - padding,
>+                             box.width + 2 * padding,
>+                             box.height + 2 * padding);
>+              break;
>+            }
>+        }
>+
>+      /* If there are no editable/activatable cells, draw focus 
>+       * around the whole item. */          
>+      if (icon_view->priv->cursor_cell < 0)
>+        gtk_paint_focus (GTK_WIDGET (icon_view)->style,
>+            	   drawable,
>+            	   GTK_STATE_NORMAL,
>+            	   area,
>+            	   GTK_WIDGET (icon_view),
>+            	   "icon_view",
>+                   x - padding,
>+                   y - padding,
>+                   item->width + 2 * padding,
>+                   item->height + 2 * padding);
>+    }
> }

I like the idea here.  The indentation and space/tab usage here needs fixing.  Interestingly, this works well when clicking with the mouse on different items, it breaks as soon as I use the keyboard to navigate.


> static void
>@@ -3331,7 +3352,7 @@
> 		    }
> 		}
> 
>-	      if (only_in_cell)
>+	      if (only_in_cell && !item->selected)
> 		return NULL;

Why is this change needed?  I think this causes the need for all the info != NULL guards we saw above.

>@@ -5349,13 +5370,11 @@
> 
> 	if (icon_view->priv->orientation == GTK_ORIENTATION_VERTICAL)
> 	  g_object_set (info->cell,
>-			"follow-state", TRUE, 
> 			"xalign", 0.5,
> 			"yalign", 1.0,
> 			NULL);
> 	else
> 	  g_object_set (info->cell,
>-			"follow-state", TRUE, 
> 			"xalign", 0.0,
> 			"yalign", 0.0,
> 			NULL);


I agree with removing the tinting now we have the larger background.  As others have said, this is in line with what tree view and menu item do.
Comment 57 Denis Washington 2008-09-10 18:44:57 UTC
(In reply to comment #56)
> (From update of attachment 117305 [details] [review] [edit])
> I like the idea of this patch.  Below will follow a couple of general and a
> couple of detailed comments on your patch.  Also, in general we really prefer
> patches generated with diff -up, since this makes reviewing the patch much
> easier.  So, please generate your patch using diff -up next time :)

Thank you for your thorough review. :) An updated patch will be attached immediately after this reply (made with diff -up, I promise ;)).

> >@@ -1301,8 +1302,8 @@
> >       /* totally ignore our child's requisition */
> >       if (child->cell < 0)
> > 	{
> >-	  allocation.x = child->item->x;
> >-	  allocation.y = child->item->y;
> >+	  allocation.x = child->item->x + ITEM_PADDING;
> >+	  allocation.y = child->item->y + ITEM_PADDING;
> > 	  allocation.width = child->item->width;
> > 	  allocation.height = child->item->height;
> > 	}
> 
> Shouldn't ITEM_PADDING also be subtracted from (width, height) here?

Yeah, it should - fixed that in the updated patch.

> >@@ -1310,8 +1311,8 @@
> > 	{
> > 	  GdkRectangle *box = &child->item->box[child->cell];
> > 
> >-	  allocation.x = box->x;
> >-	  allocation.y = box->y;
> >+	  allocation.x = box->x + ITEM_PADDING;
> >+	  allocation.y = box->y + ITEM_PADDING;
> > 	  allocation.width = box->width;
> > 	  allocation.height = box->height;
> > 	}
> 
> "box" contains the exact coordinates of the cell and should therefore already
> be corrected for item-padding during the layouting phase.  This patch chunk
> should be removed.

This isn't correct. That is, it wasn't.

"box" is calculated from the return value of gtk_cell_renderer_get_size(), which naturally doesn't know about the item padding. So whenever "box" is recalculated, the padding needs to additionally be calculated in. I am doing this in gtk_icon_view_calculate_item_size[2]() now, so the above part of the patch is removed anyway.

> >@@ -1980,17 +1977,18 @@
> >       item = gtk_icon_view_get_item_at_coords (icon_view, 
> > 					       event->x, event->y,
> > 					       TRUE,
> >-					       &info);      
> >+					       &info); 
> >       if (item != NULL)
> > 	{
> >-	  g_object_get (info->cell, "mode", &mode, NULL);
> >-	  
> >-	  if (mode == GTK_CELL_RENDERER_MODE_ACTIVATABLE ||
> >-	      mode == GTK_CELL_RENDERER_MODE_EDITABLE)
> >-	    cursor_cell = g_list_index (icon_view->priv->cell_list, info);
> >-	  else
> >-	    cursor_cell = -1;
> >-	  
> >+          if (info != NULL)
> >+            {
> >+	      g_object_get (info->cell, "mode", &mode, NULL);
> >+ 
> >+	      if (mode == GTK_CELL_RENDERER_MODE_ACTIVATABLE ||
> >+	          mode == GTK_CELL_RENDERER_MODE_EDITABLE)
> >+	        cursor_cell = g_list_index (icon_view->priv->cell_list, info);
> >+	    }
> 
> Matthias mentioned that this is probably an independent fix.  I think this is
> caused by the modification of gtk_icon_view_get_item_at_coords() below.  I
> don't think that modification is necessary and therefore these guards for info
> != NULL can also be removed.

The fix isn't independent. It handles the previously impossible case that an item is clicked, but none of the item's cells were hit (i.e., the user clicked on an area in the selection rectangle where is neither the icon nor the label).

> >@@ -2730,6 +2731,7 @@
> >       cell_area->width = item->box[info->position].width + 
> > 	item->before[info->position] + item->after[info->position];
> >       cell_area->height = item->height;
> >+      cell_area->height -= ITEM_PADDING * 2;
> >     }
> >   else
> >     {
> >@@ -2738,7 +2740,11 @@
> >       cell_area->width = item->width;
> >       cell_area->height = item->box[info->position].height + 
> > 	item->before[info->position] + item->after[info->position];
> >+      cell_area->width -= ITEM_PADDING * 2;
> >     }
> >+
> >+  cell_area->x += ITEM_PADDING;
> >+  cell_area->y += ITEM_PADDING; 
> > }
> 
> Just as the height is only corrected for the horizontal case, the y coordinate
> should probably only be corrected in the horizontal case too.  In the vertical
> case, the y coordinate is calculated from the (already corrected for
> item-padding) box coordinates.  Same for the x coordinate in the vertical case.

Correct (with the now-correct "box" assumption). Fixed.

> > static void 
> >@@ -2750,6 +2756,8 @@
> >   g_return_if_fail (info->position < item->n_cells);
> > 
> >   *box = item->box[info->position];
> >+  box->x += ITEM_PADDING;
> >+  box->y += ITEM_PADDING;
> > }
> 
> AFAIK this should be unnecessary because the box coordinates are already
> corrected for item-padding.

Removed.

> >@@ -2854,6 +2864,9 @@
> > 	  item->height += item->box[info->position].height + (info->position > 0 ? spacing : 0);
> > 	}
> >     }
> >+
> >+  item->width += ITEM_PADDING * 2;
> >+  item->height += ITEM_PADDING * 2;
> > }
> 
> Should be fine.

Think so too. ;)

> > static void
> >@@ -2940,7 +2953,12 @@
> > 	  item->box[i].x = item->x + item->width - 
> > 	    (item->box[i].x + item->box[i].width - item->x);
> > 	}      
> >-    }	
> >+    }
> >+
> >+  /* NOTE: increasing item->width by the item padding here
> >+     causes the layouting code to crash, but doesn't seem
> >+     to be needed anyway */  
> >+  item->height += ITEM_PADDING * 2;
> > }
> 
> This is asking for trouble.  In theory, we are only modifying how the items are
> being laid out internally -- the layouting algorithm of the items does not
> change and should not crash due to our modifications.  The modifications to the
> function should be done differently, you want to correct the cell_area that is
> being set up for item-padding.  Next, the corrected cell_area will be used to
> calculate the actual positions of the cells contained in the item with the
> results saved to "box".  The "box" coordinates are now calculated correctly and
> then we should not have a need for the change to gtk_icon_view_get_cell_box()
> above.  item->(width, height) are recalculated in this function and we should
> make sure they (obviously) end up being cell_area.(width, height) + 2 *
> item-padding.

Due to the mofications in this function to get "box" right, this is done correctly now. "item->height += ITEM_PADDING * 2;" is still there, but is semantically correct and causes no problems.

> >@@ -3066,42 +3073,56 @@
> > 				drawable,
> > 				GTK_WIDGET (icon_view),
> > 				&cell_area, &cell_area, area, flags);
> >-      
> >     }
> > 
> >-  /* FIXME where to draw the focus with generic cell renderers ? */
> >   if (draw_focus && 
> >       GTK_WIDGET_HAS_FOCUS (icon_view) &&
> >       item == icon_view->priv->cursor_item)
> >-    for (l = icon_view->priv->cell_list, i = 0; l; l = l->next, i++)
> >-      {
> >-	GtkIconViewCellInfo *info = (GtkIconViewCellInfo *)l->data;
> >-	
> >-	if (!info->cell->visible)
> >-	  continue;
> >-	
> >-	if (icon_view->priv->cursor_cell < 0 &&
> >-	    (info->cell->mode != GTK_CELL_RENDERER_MODE_INERT ||
> >-	     GTK_IS_CELL_RENDERER_TEXT (info->cell)))
> >-	  icon_view->priv->cursor_cell = i;
> >-	
> >-	if (i == icon_view->priv->cursor_cell)
> >-	  {
> >-	    gtk_icon_view_get_cell_box (icon_view, item, info, &box);
> >+    {
> >+      for (l = icon_view->priv->cell_list, i = 0; l; l = l->next, i++)
> >+        {
> >+          GtkIconViewCellInfo *info = (GtkIconViewCellInfo *)l->data;
> >+          
> >+          if (!info->cell->visible)
> >+            continue;
> > 
> >-	    gtk_paint_focus (GTK_WIDGET (icon_view)->style,
> >-			     drawable,
> >-			     GTK_STATE_NORMAL,
> >-			     area,
> >-			     GTK_WIDGET (icon_view),
> >-			     "icon_view",
> >-			     x - item->x + box.x - padding,
> >-			     y - item->y + box.y - padding,
> >-			     box.width + 2 * padding,
> >-			     box.height + 2 * padding);
> >-	    break;
> >-	  }
> >-      }
> >+          /* If found a editable/activatable cell, draw focus on it. */          
> >+          if (icon_view->priv->cursor_cell < 0 &&
> >+              info->cell->mode != GTK_CELL_RENDERER_MODE_INERT)
> >+            icon_view->priv->cursor_cell = i;
> >+
> >+          gtk_icon_view_get_cell_box (icon_view, item, info, &box);
> >+          
> >+          if (i == icon_view->priv->cursor_cell)
> >+            {
> >+              gtk_paint_focus (GTK_WIDGET (icon_view)->style,
> >+          		     drawable,
> >+          		     GTK_STATE_NORMAL,
> >+          		     area,
> >+          		     GTK_WIDGET (icon_view),
> >+          		     "icon_view",
> >+                             x - item->x + box.x - padding,
> >+                             y - item->y + box.y - padding,
> >+                             box.width + 2 * padding,
> >+                             box.height + 2 * padding);
> >+              break;
> >+            }
> >+        }
> >+
> >+      /* If there are no editable/activatable cells, draw focus 
> >+       * around the whole item. */          
> >+      if (icon_view->priv->cursor_cell < 0)
> >+        gtk_paint_focus (GTK_WIDGET (icon_view)->style,
> >+            	   drawable,
> >+            	   GTK_STATE_NORMAL,
> >+            	   area,
> >+            	   GTK_WIDGET (icon_view),
> >+            	   "icon_view",
> >+                   x - padding,
> >+                   y - padding,
> >+                   item->width + 2 * padding,
> >+                   item->height + 2 * padding);
> >+    }
> > }
> 
> I like the idea here.  The indentation and space/tab usage here needs fixing. 
> Interestingly, this works well when clicking with the mouse on different items,
> it breaks as soon as I use the keyboard to navigate.

First, an important attribution: the idea and the code is completely the work of Lucas Rocha. Without his initial patch, I wouldn't have been able to write the patch in its current state. Thanks, Luca! :)

About the problems with keyboard navigation: I didn't know how to fix them yet, but I noticed that they only appear once you navigate from one row to another (using the up/down arrow keys for instance). This might help to find the cause.

> > static void
> >@@ -3331,7 +3352,7 @@
> > 		    }
> > 		}
> > 
> >-	      if (only_in_cell)
> >+	      if (only_in_cell && !item->selected)
> > 		return NULL;
> 
> Why is this change needed?  I think this causes the need for all the info !=
> NULL guards we saw above.

This implements the behaivior I outlined in comment #28. This works pretty good in my opinion, but might be a debatable solution for the problems mentioned in the comment. Another way to archieve predictability (regarding whether clicking would hit an item or allow rubberband selection), it would also be a solution to implement mouse-over item highlighting. If no item is highlighted, this would be a signal to the user that the mouse is over a blank spot from which rubberband selection can be started. (I might implement this as an alternative patch.) This issue is definitely open for discussion.

As mentioned, the updated patch will now be attached.
Comment 58 Denis Washington 2008-09-10 18:48:40 UTC
Created attachment 118458 [details] [review]
GtkIconView patch iteration N

The keyboard navigation issue mentioned in coment #56 remains, but is purely aesthetical. Trying to fix this though. The rest should be correct now. Please review.
Comment 59 Denis Washington 2008-09-11 16:03:43 UTC
Created attachment 118523 [details] [review]
The last patch with keyboard navigation drawing fixed

This version fixes the problem with keyboard navigation mentioned in the last two comments. Previously, if there is no editable item cell to draw keyboard focus around, the focus rectangle was drawn around the text cell. I modified the internal find_cell() function slightly to return -1 in this case instead, causing the focus rectangle to be drawn around the whole item. (If there is an editable cell, like the item text in gtk-demo's "Icon View -> Editing and Drag-and-Drop" test, the focus is still drawn around that cell).
Comment 60 Kristian Rietveld 2008-09-15 19:01:41 UTC
(In reply to comment #57)
> Thank you for your thorough review. :) An updated patch will be attached
> immediately after this reply (made with diff -up, I promise ;)).

Great!

I am going to follow up in two separate comments here.

> "box" is calculated from the return value of gtk_cell_renderer_get_size(),
> which naturally doesn't know about the item padding. So whenever "box" is
> recalculated, the padding needs to additionally be calculated in. I am doing
> this in gtk_icon_view_calculate_item_size[2]() now, so the above part of the
> patch is removed anyway.

This is exactly what I tried to make clear, that the padding should be corrected for in _calculate_item_size2() and not _allocate_children()  :)

> The fix isn't independent. It handles the previously impossible case that an
> item is clicked, but none of the item's cells were hit (i.e., the user clicked
> on an area in the selection rectangle where is neither the icon nor the label).

I agree with you -- but this impossibility has disappeared by the fact that gtk_icon_view_get_item_at_coords() has been patched.  (More on this below).

> > > static void
> > >@@ -3331,7 +3352,7 @@
> > > 		    }
> > > 		}
> > > 
> > >-	      if (only_in_cell)
> > >+	      if (only_in_cell && !item->selected)
> > > 		return NULL;
> > 
> > Why is this change needed?  I think this causes the need for all the info !=
> > NULL guards we saw above.
> 
> This implements the behaivior I outlined in comment #28. This works pretty good
> in my opinion, but might be a debatable solution for the problems mentioned in
> the comment. Another way to archieve predictability (regarding whether clicking

Personally, I like the argumentation in comment #28.  I do not like the implementation of this, gtk_icon_view_get_item_at_coords() is not the place where selection behavior should be changed.  This is due to other functions, not depending on selection behavior, that are also using this function (eg. gtk_icon_view_get_path_at_pos(), gtk_icon_view_get_item_at_pos).  I would advise to move this logic somewhere else, probably to the button-press handler.  When done the "non-independent fix" above ;) needs revision to see whether it is still necessary to have.
Comment 61 Kristian Rietveld 2008-09-15 19:11:59 UTC
(In reply to comment #59)
> Created an attachment (id=118523) [edit]
> The last patch with keyboard navigation drawing fixed
> 
> This version fixes the problem with keyboard navigation mentioned in the last
> two comments. Previously, if there is no editable item cell to draw keyboard
> focus around, the focus rectangle was drawn around the text cell. I modified
> the internal find_cell() function slightly to return -1 in this case instead,
> causing the focus rectangle to be drawn around the whole item. (If there is an
> editable cell, like the item text in gtk-demo's "Icon View -> Editing and
> Drag-and-Drop" test, the focus is still drawn around that cell). 

Functionally, keyboard navigation seems to work well in this patch.  The location of the focus rectangle on individual cells and also the position where the editables of the cells are overlaid are still wrong.  I've done some debugging on this and I think this is being caused the boxes not being calculated 100% correctly yet.

Attached is an updated patch that solves these issues.  I also took the liberty to fix the indentation issues as pointed out in comment 56.  Please try this one out and see if it breaks stuff or not ;)

What's not yet fixed is the gtk_icon_view_get_item_at_coords() issue as outlined above -- we might as well split that out of this patch and handle as a separate issue.  Additionally I found that the focus rectangle is sometimes overlapped with parts of the other cells in an item.  This seems to be present in previous versions of GTK+ also, so can be fixed in another bug.
Comment 62 Kristian Rietveld 2008-09-15 19:12:46 UTC
Created attachment 118778 [details] [review]
updated patch
Comment 63 Denis Washington 2008-09-16 10:01:08 UTC
The updated patch works great, thanks Kristian! :) Based on that I created an updated patch with the selection behaivior moved from gtk_icon_view_get_item_at_coords() to the button-press handler.
Comment 64 Denis Washington 2008-09-16 10:02:38 UTC
Created attachment 118819 [details] [review]
Updated patch with selection behaivior changes moved to gtk_icon_view_button_press()
Comment 65 Matthias Clasen 2008-09-18 16:25:14 UTC
Now that we've branched, this patch can go in as soon as you are both happy with it.
Comment 66 Denis Washington 2008-09-18 16:40:02 UTC
I am happy. As already said in comment #61, the last remaining issue doesn't seem to have anything to do with the changes in this patch and should be addressed separately. Awaiting your feedback, Kristian. :)
Comment 67 Kristian Rietveld 2008-09-23 13:47:28 UTC
(In reply to comment #64)
> Created an attachment (id=118819) [edit]
> Updated patch with selection behaivior changes moved to
> gtk_icon_view_button_press()

This patch looks great to me!  As Matthias seems to trust me on handing out commit permission for icon view patches, I've granted the accepted state.

For the focus rectangle overlapping issue I've filed #553401.

Thank you for your continued work on this patch, Denis.
Comment 68 Denis Washington 2008-09-24 08:06:43 UTC
Commited as revision 21506. Many thanks Lucas Rocha for the initial patch upon which I started the work, and everybody else who reviewed and helped me with the patch!