Created attachment 88807 [details] [review]
patch

Maybe I should give a rationale for the change in gtk_tree_view_search_dialog_hide(), when switching to the treeview content with keyboard navigation, search window is shown and hidden immediately, and when it's hidden forces the GTK_HAS_FOCUS flag in the treeview, which makes its rows take focus, even if there wasn't any selectable row, producing strange focus behavior. Given that if the search window is shown legitimately the treeview will already have that flag set, there should be no need to force it to the treeview again.

Great patch, that fixes a longstanding annoyance in comboboxes and in the file chooser !

I'll let Kris review it too, but I'm all for this going in.

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

Let me start off by saying that it should still be possible to move the cursor to insensitive rows.  You want to be able to move left/right on insensitive rows, you only can't toggle its selection state or control any of the interactive cell renderers.  I am renaming the bug to reflect that we are (for now) only talking about disallowing movement to separators.

More comments:

* In gtk_tree_view_button_press() separators rows should be handled via the node == NULL case (ie. we clicked in dead space).

* The patch chunk in gtk_tree_view_bin_expose() is not needed since we assure in the other parts of the code that the cursor can never be on a separator.  This will avoid slowing down gtk_tree_view_bin_expose() too (which can use some performance improvements already).

* The gtk_tree_view_focus() changes should be removed, a tree view with only a separator is an empty tree view and can still receive focus.

* The current code in maemo will indeed crash and burn if you use lists without any "normal" items.  In such a case it's probably better to just return; just like the tree_view->priv->tree == NULL case.  And indeed, the DRAW_KEYFOCUS flag does not have to be set in that case.

* What is the reason for removing "y -= window_y" in gtk_tree_view_move_cursor_p
age_up_down()?  I spent quite a bit of time getting page up/down right a few months ago, and I think that should be all correct in trunk right now.

* For the gtk_tree_view_search_dialog_hide() chunk, I also spent quite a bit of time on getting that right.  Were you seeing any wrong behavior before patching too?  If yes, please report this via a separate bug or rather reopen and comment to #356515.


(Please ask if anything is not clear, it's late here already ;) )

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

(In reply to comment #4)
> Let me start off by saying that it should still be possible to move the cursor
> to insensitive rows.  You want to be able to move left/right on insensitive
> rows, you only can't toggle its selection state or control any of the
> interactive cell renderers.  I am renaming the bug to reflect that we are (for
> now) only talking about disallowing movement to separators.

Ok, I've cooked a new patch to just disallow movement to separators. however it feels a bit strange to me to be able to move focus across seemingly insensitive items in a filechooser in select folder mode.

> 
> More comments:
> 

<snip>

> * The gtk_tree_view_focus() changes should be removed, a tree view with only a
> separator is an empty tree view and can still receive focus.

I wanted to avoid having focus on something unable to show that it has the focus, I've removed these changes in the new patch and if the treeview has only separators (quite a extreme use case :) it won't show focus. We could try to show the "tree empty" focus, but seems to be tricky.

> 
> * The current code in maemo will indeed crash and burn if you use lists without
> any "normal" items.  In such a case it's probably better to just return; just
> like the tree_view->priv->tree == NULL case.  And indeed, the DRAW_KEYFOCUS
> flag does not have to be set in that case.

I already tried hard to allow this use case, seems to work fine.

> 
> * What is the reason for removing "y -= window_y" in
> gtk_tree_view_move_cursor_p
> age_up_down()?  I spent quite a bit of time getting page up/down right a few
> months ago, and I think that should be all correct in trunk right now.

Doh! there was absolutely no reason, it's not removed in the new patch.

> 
> * For the gtk_tree_view_search_dialog_hide() chunk, I also spent quite a bit of
> time on getting that right.  Were you seeing any wrong behavior before patching
> too?  If yes, please report this via a separate bug or rather reopen and
> comment to #356515.

I wasn't getting any big issues in the unpatched treeview, the problem arose when it was forcing focus to the treeview rows when there wasn't selectable rows, but that's not the behavior anymore.

Other than that, the only (just visual) glitch I've found with this happens when you show the search window and then click i.e. on the desktop, the focus is still drawn in the treeview despite of it's window not having focus, but it's a nitpick :)

Created attachment 89120 [details] [review]
updated patch

Created attachment 89121 [details] [review]
another updated patch

this one doesn't select non-selectable rows during rubberbanding, it was right in the first patch

Created attachment 94118 [details] [review]
patch as committed

Committed r18672.