GNOME Bugzilla – Bug 748265
Scroll jumps back to first selected item
Last modified: 2017-08-22 22:58:12 UTC
So in icon view, I select a few files (using Ctrl+click), then scroll down and try to select more, but every time I do that, the view keeps scrolling back to the 'first' (topmost) selection, and I have to scroll down again to select the next file. nautilus 3.16.0 (& git master)
I've investigated this, it's a regression caused by commit bb884cb6 (cc'ing Carlos), when clicking a canvas icon: update_context_menus_timeout_callback() is called, which calls nautilus_canvas_view_update_actions_state () which calls g_action_group_change_action_state() on the "sort" action, so it's telling nautilus that's been a change in the sort order of the view (which is false, the sort order has not changed! that's the bug) so, the nautilus handler for when the sort order is changed is to scroll and reveal the first selected item, which is right behaviour. I'm attaching the gdb backtrace in a following comment.
(gdb) bt
+ Trace 235009
*** Bug 748522 has been marked as a duplicate of this bug. ***
thanks for the debugging, the problem is clear. Will take a look.
Created attachment 302530 [details] [review] canvas-view: only change action states when they change This avoid an undesired change on "sort" state, which causes an invalidation of the canvas and moves the scroll to the first selected item.
Review of attachment 302530 [details] [review]: For the commit message: The title reads odd. Only change it when it changes =) I would say: change the action state only when the value differs Also the comment from Nelson explains well one part. "the nautilus handler for when the sort order is changed is to scroll and reveal the first selected item" In your commit message is not clear if scrolling to the first item is a bug, part of the bug or is the intended behaviour. That sentence from Nelson makes it clearer. So I would do something like: title Context (nautilus scrolls to the first item when the sort state is changed because bla blah, and there fore we need to call it when the sort changes) What generates the bug (currently we change the action state everytime when we update menus which triggers the sort changed handler) What we do to fix it (avoid to change the action state if the new value don't actually differ from the current one) ::: src/nautilus-canvas-view.c @@ +1068,3 @@ + reversed_state = g_action_group_get_action_state (view_action_group, "reversed-order"); + + if (reversed_state && this reads as: If the current action value is NULL, don't change the value. That is not what you probably want. Also these actions are stateful (if they wouldn't we won't bother to get the value etc), you don't need to check for NULL. NULL returning here is only as a way to returning something for not stateful actions. Also, in case the action is no stateful, calling g_action_group_change_action_state in any part of the code for this action will be an error.
(In reply to Carlos Soriano from comment #6) > Review of attachment 302530 [details] [review] [review]: > > For the commit message: > The title reads odd. Only change it when it changes =) > I would say: change the action state only when the value differs > > Also the comment from Nelson explains well one part. > "the nautilus handler for when the sort order is changed is to scroll and > reveal the first selected item" > In your commit message is not clear if scrolling to the first item is a bug, > part of the bug or is the intended behaviour. That sentence from Nelson > makes it clearer. > > So I would do something like: > > title > > Context (nautilus scrolls to the first item when the sort state is changed > because bla blah, and there fore we need to call it when the sort changes) > What generates the bug (currently we change the action state everytime when > we update menus which triggers the sort changed handler) > What we do to fix it (avoid to change the action state if the new value > don't actually differ from the current one) With an empty line between those three things to make it clear
Created attachment 302649 [details] [review] canvas-view: change action states only when the value differs Commit bb884cb6 introduced the faulty behavior, with NautilusCanvasView scrolling back to the first selected item when selecting multiple items. Backtracing it, it was found out that the icon view was invalidating the "sort" state when selecting items, which is obviously false since the ordering of the items does not depend on the selection. The issue is fixed by only changing the "sort" state when it makes sense, i.e. only when the states really change.
Review of attachment 302649 [details] [review]: I'm still not very happy with the commit message. did you see my comments in the previous one? Like explaining why we were doing that. It would be nice to know the reason we are updating the sort order etc. What do you think? Maybe it's not too important for this fix if the comment is good in the code, but looks like something good to follow the commit guidelines just in case. Code looks good. ::: src/nautilus-canvas-view.c @@ +1067,3 @@ + sort_state = g_action_group_get_action_state (view_action_group, "sort"); + reversed_state = g_action_group_get_action_state (view_action_group, "reversed-order"); + Could you add comment in code for what we need this?
Created attachment 302655 [details] [review] canvas-view: change action states only when the value differs Commit bb884cb6 introduced the faulty behavior where NautilusCanvasView scrolling back to the first selected item when selecting multiple items. Backtracing it, it was found out that the icon view was invalidating the "sort" state when selecting items, which is obviously false since the ordering of the items does not depend on the selection. Nautilus automatically scrolls to the first sorted item when that happens. The issue is fixed by only changing the "sort" state when it makes sense, i.e. only when the states really change.
Created attachment 302661 [details] [review] canvas-view: change action states only when the value differs Commit bb884cb6 introduced the faulty behavior where NautilusCanvasView keeps scrolling back to the first selected item when selecting multiple items. Backtracing it, it was found out that the icon view was changing the "sort" state when selecting items, even if nothing actually changed, which triggers the sort action changed handler. The handler by itself was doing the right job by revealing the selection, and the issue was caused by calling it unnecessarily. Since we may have to update the action states for other reasons besidse real sort change, we must change the action state only when it makes sense, i.e. only when the states really change.
Created attachment 302662 [details] [review] canvas-view: change action states only when the value differs Commit bb884cb6 introduced the faulty behavior where NautilusCanvasView keeps scrolling back to the first selected item when selecting multiple items. Backtracing it, it was found out that the icon view was changing the "sort" state when selecting items, even if nothing actually changed, which triggers the sort action changed handler. The handler by itself was doing the right job by revealing the selection, and the issue was caused by calling it unnecessarily. Since we may have to update the action states for other reasons besidse real sort change, we must change the action state only when it makes sense, i.e. only when the states really change.
Review of attachment 302662 [details] [review]: Good enought, thanks! ::: src/nautilus-canvas-view.c @@ +1072,3 @@ + * To achieve this, check if the action state value actually changed before setting + * it. + * the sort action changed handler, which reveals the selection, since we expect And the same for the reversed state. =) just add it and commit
Attachment 302662 [details] pushed as 0ff112a - canvas-view: change action states only when the value differs
a small workaround in the meantime: start selection from the bottom, then the view does not jump (go to bottom, select a few / scroll a page up / select a few / and so on)
*** Bug 749985 has been marked as a duplicate of this bug. ***
*** Bug 753143 has been marked as a duplicate of this bug. ***