Created attachment 66517 [details] [review]
the patch file

We probably need some comparative analysis how similar widgets on other platforms behave keynav-wise. 

Windows Explorer Icon View doesn't do any of these actions, and simply maintains the selection.

The arrow navigation in the icon view is very similar to the tree view. 
I think that is an advantage.

(In reply to comment #3)
> Windows Explorer Icon View doesn't do any of these actions, and simply
> maintains the selection.

But it does have one little feature that I think would be useful. When you are on the last icon on the last row of the IconView, and when that icon is not a leftmost icon, and you press the right arrow key, then the leftmost icon of the previous line is focused. 

Also you can "jump diagonally" to the last item from the item one step above and to the right of it by pressing down.

this patch can enable the behaviours desired:

--- /data/zzz/gtk-2.6/gtk+-2.6.10/gtk/gtkiconview.c     2005-08-18 22:10:58.000000000 +0800
+++ gtk/gtkiconview.c   2006-08-30 17:29:15.797308964 +0800
@@ -2678,10 +2678,85 @@ find_item (GtkIconView     *icon_view,

   /* FIXME: this could be more efficient
    */
-  row = current->row + row_ofs;
-  col = current->col + col_ofs;
+  int columns;
+  if (icon_view->priv->columns > 0) //the column number was explicitly set by gtk_icon_view_set_columns()
+  {
+    if (g_list_length (icon_view->priv->items) >= icon_view->priv->columns)
+    {
+      columns = icon_view->priv->columns;
+    }
+    else
+    {
+      columns = g_list_length (icon_view->priv->items);
+    }
+  }
+  else
+    columns = (icon_view->priv->width - icon_view->priv->margin * 2 + icon_view->priv->column_spacing) / (icon_view->priv->column_spacing + current->width);

-  for (items = icon_view->priv->items; items; items = items->next)
+  int rows = g_list_length (icon_view->priv->items) / columns;
+  if (g_list_length (icon_view->priv->items) % columns > 0)
+    rows++;
+  items = g_list_last(icon_view->priv->items);
+  item = items->data;
+  if (col_ofs == 1) //right is pressed
+  {
+    if (current->col == item->col && current->row == (rows - 1)) //the current item is the last one, wrap to the first item
+    {
+      row = 0;
+      col = 0;
+    }
+    else if (current->col == (columns - 1)) //the current item is the rightmost one
+    {
+      row = current->row + row_ofs + 1;
+      col = 0;
+    }
+    else
+    {
+      row = current->row + row_ofs;
+      col = current->col + col_ofs;
+    }
+  }
+  else if (col_ofs == -1) //left is pressed
+  {
+    if (current->col == 0) //the current item is the leftmost one
+    {
+      if (current->row == 0) //the current item is the first one, wrap to the last item
+      {
+        row = rows - 1;
+        col = item->col;
+      }
+      else
+      {
+        row = current->row + row_ofs - 1;
+        col = columns - 1;
+      }
+    }
+    else
+    {
+      row = current->row + row_ofs;
+      col = current->col + col_ofs;
+    }
+  }
+  else if (row_ofs == 1) //down is pressed
+  {
+    if (current->row == (rows - 2) && item->col < current->col)// at the second last row
+    {
+      row = current->row + row_ofs;
+      col = 0;
+    }
+    else
+    {
+      row = current->row + row_ofs;
+      col = current->col + col_ofs;
+    }
+  }
+  else //up is pressed
+  {
+    row = current->row + row_ofs;
+    col = current->col + col_ofs;
+  }
+
+   for (items = icon_view->priv->items; items; items = items->next)
     {
       item = items->data;
       if (item->row == row && item->col == col)
@@ -2819,8 +2894,10 @@ gtk_icon_view_move_cursor_up_down (GtkIc
                      count, 0);
   if (!item)
+  {
+    gtk_widget_child_focus (gtk_widget_get_toplevel (GTK_WIDGET(icon_view)), count > 0 ? GTK_DIR_TAB_FORWARD : GTK_DIR_TAB_BACKWARD);
     return;
-
+  }
   if (icon_view->priv->ctrl_pressed ||
       !icon_view->priv->shift_pressed ||
       !icon_view->priv->anchor_item ||

Created attachment 88911 [details] [review]
Cleaned up patch

I've cleaned up the patch to adhere to the coding standards, and I've also removed the code to focus the widget below/above the iconview if you use the down/up arrow on the top or bottom rows (respectively), as I don't think this is intuitive --- people can use the tab key to do that, and arrow navigation should stay inside one widget.

I think this is a suitable candidate for inclusion into GTK+, and there's not much to lose by putting it in anyway.

Thanks, Philip!

But as to arrow navigation staying inside GtkIconView, I think on a mobile phone, a up/down press bringing the focus switch is quite handy and necessary since we don't have tab keys.

Comment on attachment 88911 [details] [review]
Cleaned up patch

>Index: gtk/gtkiconview.c

>   /* FIXME: this could be more efficient 
>    */
>-  row = current->row + row_ofs;
>-  col = current->col + col_ofs;
>+  int columns = (icon_view->priv->width - icon_view->priv->margin * 2 + icon_view->priv->column_spacing) / (icon_view->priv->column_spacing + current->width);
>+  int rows = g_list_length (icon_view->priv->items) / columns;

Declare the variables at the beginning of the function, next to the others.

Try to keep the width down, ideally below 79 characters.

>+  if (g_list_length (icon_view->priv->items) % columns > 0)	
>+    rows++;

g_list_length() is O(N), so it would be better if you could call it only once.

>+  items = g_list_last (icon_view->priv->items);
>+  item = items->data;  

>+          row = current->row + row_ofs;
>+          col = current->col + col_ofs;      

This is repeated 4 times, is that possible to avoid?

>+  else if (col_ofs == -1) /* Left pressed */

Put comments on the line before, not on the same line.

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

(In reply to comment #2)
> We probably need some comparative analysis how similar widgets on other
> platforms behave keynav-wise.

the Nautilus icon view works this way, at least for the first two points described above.

So does thunar icon view, for the first two points as well.

For the two latter, the tab key is probably best suited. The mobile phone issue could be treated in another bug if there is no obvious solution to it.

As far as I understand, proposed patch (attachment 88911 [details] [review]) does more than points 1 and 2 described above.

/* The current item is the last one; wrap to the first item */

This is not what is described in the bug description. And I am not sure it is what we want here. If I keep the arrow key pressed, I expect the cursor to reach the last item, ultimately, not to loop forever. Besides it is not thunar's default behaviour, for instance.

I might be mistaken, but function find_item() is designed to handle all kinds of offests, not only (1 row, 0 column) or (0 row, 1 column) ones. I feel weird about patching it to treat specifically the case we're dealing with here. I mean : shouldn't all offsets be wrapped around ?

As far as I understand, the original code just ignores an offset request that is out of bounds. (Like asking for an offset of (0 rows, 5 columns) when only 3 columns exist, for instance.)

While trying to clean the patch as per Comment 9, I did the modifications to handle all kinds of offsets. I'm attaching the patch.

It is meant to wrap around columns on any offset, and return first or last element if the result is out of bounds.

Please note that :
- if my understanding is correct, this patch modifies current behaviour on big offsets, which is out of the scope of this bug
- it is not tested, not even compiled (I don't have any setup here to do that, but I kinda which I had the time)
- it may contain stupid errors (it's getting late)
- I don't know the coding standards, nor do I know how to submit a proper patch...

Anyway, I'm not sure it is worth spending more time coding before agreeing on the design : what is the expected behaviour 
- as regards to 1 step offset wrapping as described in the bug description ?
- as regards to bigger offsets ?

Also, my proposition shouldn't be completely useless since at least I improved the calculation of the number of rows to make it more efficient, as requested by Johan Dalin in Comment 9. This could be imported in attachment 88911 [details] [review].

Created attachment 151979 [details] [review]
Patch to implement column wrapping for any offset size

Just realized what I did does something more, that was not written in the bug description. Here's an example :

1 2 3 4 
5 6 7 8
9

When 7 is selected and down arrow is pressed, current behaviour is "do nothing". If my code works as I expect it to, 9 will be selected, which I think is not that bad. I would say it's better, but unfortunately, it does not match thunar's behaviour...

Created attachment 152008 [details] [review]
Patch to implement column wrapping for any offset size, v2

I checked in thunar, and in case of big row offset yielding a row beyond the last row (pressing page down on the row just before the last row) or before the first, the row is bounded to last or first but the column is kept, therefore, it is not the last of first element that is selected but the lower or upper element in the same column.

Example :

1 2 3 4 
5 6 7 8
9 a b c
d e f g

f is selected. I press page up, 7 is selected. I press page up again, 3 is selected, not 1.

I assume this is what we want to happen in GtkIconView as well.

I can't think of any use case involving a big column offset (like a "page right" key would do). There are two ways of dealing with such a case. We can either wrap around columns and move to next rows or stop at last column, just like we do with page down offsets. On the same example, assuming 5 is selected, a 2-step offset on the right would move to 7 and then either 9 (first way) or 8 (second way). Again, I don't know if this use case actually exists and I couldn't find any documentation on fonction find_item(), so I don't know what the caller expects from it.

I assumed we want it first way, which is wrapping around columns.

This leads me to a new version of my former patch, which is actually even simpler.

This bug has been opened almost 4 years ago. It does not seem to be actively treated. I'd like to push it but I don't know what to do about it.

Is it a matter of requirement ? Don't we agree on the behaviour to expect from GtkIconView ?

Or is it a matter of coding/testing, therefore a matter of time (and skills) ?

We're moving to gitlab! As part of this move, we are closing bugs that haven't seen activity in more than 5 years. If this issue is still imporant to you and
still relevant with GTK+ 3.22 or master, please consider creating a gitlab issue
for it.