GNOME Bugzilla – Bug 694034
Nautilus should optionally reuse already opened views before opening a new window
Last modified: 2013-03-04 09:55:34 UTC
When nautilus is invoked to open a location that is already opened in a window or tab, it should preferentially focus the available view before opening a new one. Calling nautilus with the "-w" or "--new-window" parameter should also force the standard behavior.
Created attachment 236470 [details] [review] 0001: NautilusWindow: present a window when grabbing focus
Created attachment 236471 [details] [review] 0002: NautilusApplication: don't open a new window if the location is already open in a slot
Created attachment 236472 [details] [review] 0003: NautilusApplication: add --new-window commandline to force the old behavior
Created attachment 236473 [details] [review] 0004: NautilusWindowSlot: allow the location change if selection has been changed
Review of attachment 236472 [details] [review]: ::: src/nautilus-application.c @@ +1347,3 @@ } else { /* Invoke "Open" to create new windows */ + g_application_open (application, files, len, open_new_window ? "new" : ""); This is not properly the best way to do this, but it seems legal and the faster way to me.
Review of attachment 236470 [details] [review]: ::: src/nautilus-window.c @@ +1338,3 @@ gtk_widget_show (GTK_WIDGET (window)); + + nautilus_window_grab_focus (window); Is there a reason you can't just call gtk_window_present() here instead of gtk_widget_show() and avoid modifying nautilus_window_grab_focus()? @@ +1724,3 @@ window = NAUTILUS_WINDOW (widget); + GTK_WIDGET_CLASS (nautilus_window_parent_class)->show (widget); No whitespace-only changes
Review of attachment 236471 [details] [review]: ::: src/nautilus-application.c @@ +567,3 @@ + + g_return_val_if_fail (NAUTILUS_IS_APPLICATION (application), NULL); + NautilusWindowSlot *slot; You can remove these. @@ +575,3 @@ + location = g_file_get_parent (location); + is_file = TRUE; + g_return_val_if_fail (NAUTILUS_IS_APPLICATION (application), NULL); Doing sync I/O here is not the best, but maybe you can avoid it by checking for g_file_has_parent (slot_location, location) in addition to g_file_equal. @@ +651,3 @@ + open_window (application, files[i], screen, geometry); + } else { + // We open the location again to update any possible selection No C++ comments
Review of attachment 236472 [details] [review]: ::: src/nautilus-application.c @@ +709,3 @@ const gchar *hint) { + G_APPLICATION_CLASS (nautilus_application_parent_class)->open (app, files, n_files, hint); No need to chain up here. @@ +1347,3 @@ } else { /* Invoke "Open" to create new windows */ + g_application_open (application, files, len, open_new_window ? "new" : ""); I think it's fine, but use "new-window" instead.
Review of attachment 236473 [details] [review]: Why is this patch needed at all? In which code path exactly is this condition hit? Code seems a bit messy, I added some comments. ::: src/nautilus-window-slot.c @@ +796,2 @@ old_location && g_file_equal (old_location, location) && !is_desktop) { Generally I think you should factor this out into a separate function. @@ +797,3 @@ !is_desktop) { + GList *old_selection = nautilus_view_get_selection (slot->details->content_view); + gboolean selection_matches = FALSE; If you initialize this to TRUE here you can remove the if below. @@ +802,3 @@ + selection_matches = (old_selection == new_selection); + + if (!selection_matches) { Without the previous if, this would be if (new_selection != NULL && old_selection != NULL), which you can also remove, since the cycles won't run for a NULL list, and g_list_length(NULL) should return 0. @@ +807,3 @@ + + for (ol = old_selection; ol && selection_matches; ol = ol->next) { + const char *old_uri = nautilus_file_get_uri (NAUTILUS_FILE (ol->data)); nautilus_file_get_uri doesn't return a const char *, but it allocates a new string. I'd use nautilus_file_get_location and g_file_equal @@ +812,3 @@ + for (nl = new_selection; nl && !found; nl = nl->next) { + const char *new_uri = nautilus_file_get_uri (NAUTILUS_FILE (nl->data)); + You should break when found is TRUE @@ +815,3 @@ + } + + const char *old_uri = nautilus_file_get_uri (NAUTILUS_FILE (ol->data)); And break here when found is FALSE @@ +819,3 @@ + + if (selection_matches) + I don't understand this...if you need the check do it before the cycle and skip the cycle altogether if the length are different.
Review of attachment 236470 [details] [review]: ::: src/nautilus-window.c @@ +1338,3 @@ gtk_widget_show (GTK_WIDGET (window)); + + nautilus_window_grab_focus (window); Well, I thought that _grab_focus was designed for this and also I'll need to call this in the other parts. @@ +1724,3 @@ window = NAUTILUS_WINDOW (widget); + GTK_WIDGET_CLASS (nautilus_window_parent_class)->show (widget); Sorry, I've just seen a bunch of trailing whitespaces and I cleaned up few of them...
Review of attachment 236471 [details] [review]: ::: src/nautilus-application.c @@ +567,3 @@ + + g_return_val_if_fail (NAUTILUS_IS_APPLICATION (application), NULL); + g_return_val_if_fail (G_IS_FILE (location), NULL); Fine... @@ +575,3 @@ + location = g_file_get_parent (location); + is_file = TRUE; + } Yeah, you're right... Using g_file_has_parent (location, slot_location) did it!
Review of attachment 236472 [details] [review]: ::: src/nautilus-application.c @@ +709,3 @@ const gchar *hint) { + G_APPLICATION_CLASS (nautilus_application_parent_class)->open (app, files, n_files, hint); Ah, I thought it was missed there, why not?
Created attachment 236693 [details] [review] 0002: NautilusApplication: don't open a new window if the location is already open in a slot
Created attachment 236694 [details] [review] 0003: NautilusApplication: add --new-window commandline to force the old behavior
(In reply to comment #10) > Review of attachment 236470 [details] [review]: > > ::: src/nautilus-window.c > @@ +1338,3 @@ > gtk_widget_show (GTK_WIDGET (window)); > + > + nautilus_window_grab_focus (window); > > Well, I thought that _grab_focus was designed for this and also I'll need to > call this in the other parts. No, nautilus_window_grab_focus is about keyboard focus, and it will call gtk_widget_grab_focus on the view. If you need to call this in other parts, just call it after you presented the window. > @@ +1724,3 @@ > window = NAUTILUS_WINDOW (widget); > > + GTK_WIDGET_CLASS (nautilus_window_parent_class)->show (widget); > > Sorry, I've just seen a bunch of trailing whitespaces and I cleaned up few of > them... No problem :) The reason I don't want whitespace cleanups mixed with actual changes is they make git blame less useful and more cumbersome to use.
(In reply to comment #12) > Review of attachment 236472 [details] [review]: > > ::: src/nautilus-application.c > @@ +709,3 @@ > const gchar *hint) > { > + G_APPLICATION_CLASS (nautilus_application_parent_class)->open (app, files, > n_files, hint); > > Ah, I thought it was missed there, why not? GApplication implementations are not meant to chain up to open, the default handler doesn't do anything else than checking if you're either connecting to the signal or implementing the method.
Review of attachment 236473 [details] [review]: ::: src/nautilus-window-slot.c @@ +796,2 @@ old_location && g_file_equal (old_location, location) && !is_desktop) { Yes, I was thinking the same... I just forgot to do that at the end @@ +802,3 @@ + selection_matches = (old_selection == new_selection); + + if (!selection_matches) { Well, in case that the old_selection is filled and new_selection is not, the first for would run, even if only for one iteration. It's not that much, but that's why I did this tricky checks :) @@ +807,3 @@ + + for (ol = old_selection; ol && selection_matches; ol = ol->next) { + const char *old_uri = nautilus_file_get_uri (NAUTILUS_FILE (ol->data)); Oh, sorry... I probably misread the header. And I also was looking for the GFile, but probably I was too tired when writing this :/ @@ +812,3 @@ + for (nl = new_selection; nl && !found; nl = nl->next) { + const char *new_uri = nautilus_file_get_uri (NAUTILUS_FILE (nl->data)); + found = (g_strcmp0 (old_uri, new_uri) == 0); Well, the for guard (nl && !found) makes this unneeded, but if you want to mark to the reader that we're stopping the loop, I can add it. I just didn't want to include an extra if into the body. @@ +815,3 @@ + } + + selection_matches = found; As before, this is already prevented by "ol && selection_matches", but just let me know which style you prefer. @@ +819,3 @@ + + if (selection_matches) + selection_matches = (g_list_length (old_selection) == g_list_length (new_selection)); Yeah, I know, but I wanted to avoid these extra iterations until we are in the remote case of having two selections differing only for few additions. Of course we can test this immediately, but we can loose some cycles.
Created attachment 236701 [details] [review] 0004: NautilusWindowSlot: allow the location change if selection has been changed
(In reply to comment #17) > Well, in case that the old_selection is filled and new_selection is not, the > first for would run, even if only for one iteration. It's not that much, but > that's why I did this tricky checks :) I meant to say (old_selection != NULL || new_selection != NULL) in my previous comment, sorry. That way it would still have the same effect as far as I can see. > Well, the for guard (nl && !found) makes this unneeded, but if you want to mark > to the reader that we're stopping the loop, I can add it. I just didn't want to > include an extra if into the body. Yeah, I missed those guards...I prefer the explicit break syntax though, it's much easier to read. > @@ +819,3 @@ > + > + if (selection_matches) > + selection_matches = (g_list_length (old_selection) == > g_list_length (new_selection)); > > Yeah, I know, but I wanted to avoid these extra iterations until we are in the > remote case of having two selections differing only for few additions. > Of course we can test this immediately, but we can loose some cycles. The common case here should be where selections length differ - e.g. I select some files on a window with no files or other files selected, so I don't think checking for length before is a bad thing...for large selections I think it'd be faster. I'll review the new patchset.
Created attachment 236702 [details] [review] 0001: NautilusWindow: present a window when grabbing focus
Created attachment 236703 [details] [review] 0002: NautilusApplication: don't open a new window if the location is already open in a slot
Created attachment 236704 [details] [review] 0003: NautilusApplication: add --new-window commandline to force the old behavior
Created attachment 236705 [details] [review] 0004: NautilusWindowSlot: allow the location change if selection has been changed
(In reply to comment #11) > Review of attachment 236471 [details] [review]: > > ::: src/nautilus-application.c > @@ +567,3 @@ > + > + g_return_val_if_fail (NAUTILUS_IS_APPLICATION (application), NULL); > + g_return_val_if_fail (G_IS_FILE (location), NULL); > > Fine... > > @@ +575,3 @@ > + location = g_file_get_parent (location); > + is_file = TRUE; > + } > > Yeah, you're right... Using g_file_has_parent (location, slot_location) did it! Ops... I was wrong, this won't work if you try to open a subfolder of an open path. At that point we need to check for the type... I don't think we have other ways
Created attachment 236706 [details] [review] 0002: NautilusApplication: don't open a new window if the location is already open in a slot
Created attachment 236707 [details] [review] 0004: NautilusWindowSlot: allow the location change if selection has been changed
Created attachment 236717 [details] [review] 0004: NautilusWindowSlot: allow the location change if selection has been changed
Review of attachment 236702 [details] [review]: I like this a lot better - some comments below. ::: src/nautilus-window.c @@ +224,3 @@ + + if (!NAUTILUS_IS_DESKTOP_WINDOW (window)) { + gtk_window_present (GTK_WINDOW (window)); Is this ever called on the desktop window? @@ +1344,3 @@ gtk_widget_show (GTK_WIDGET (window)); + + nautilus_window_present (window); Why is this change needed here? Don't you call nautilus_window_present() manually in the code paths you add? @@ +1730,3 @@ window = NAUTILUS_WINDOW (widget); + GTK_WIDGET_CLASS (nautilus_window_parent_class)->show (widget); Whitespace
Review of attachment 236706 [details] [review]: OK I guess we'll have to live with the sync I/O in that code path for now. ::: src/nautilus-application.c @@ +571,3 @@ + if (g_file_query_file_type (location, G_FILE_QUERY_INFO_NONE, NULL) != G_FILE_TYPE_DIRECTORY) { + location = g_file_get_parent (location); + gboolean is_file; I'd like it better if you called g_object_ref(location) in the else block here, removed this is_file variable and unconditionally unreffed location at the end of the function. @@ +574,3 @@ + } + + slot = NULL; Please use if (slot) { break; } in the outer cycle instead of merging the condition in the loop guard.
Review of attachment 236704 [details] [review]: This looks good to land together with the rest of the patchset, once these nits are fixed ::: src/nautilus-application.c @@ -689,3 @@ { NautilusApplication *self = NAUTILUS_APPLICATION (app); - Whitespace change @@ +1340,3 @@ nautilus_profile_end (NULL); + return TRUE; Whitespace change.
Review of attachment 236717 [details] [review]: ::: libnautilus-private/nautilus-file-utilities.c @@ +1297,3 @@ } +gboolean nautilus_file_selection_equal (GList *selection_a, GList *selection_b) Our coding style prefers this form for function bodies (we're not always consistent with indenting the arguments, but code looks better that way): void foo_do_bar (Foo *foo, Bar *bar) Also, we always use curly braces, even if the block only has a single line (you don't use curly bracese for if blocks below). @@ +1303,3 @@ + + if (!selection_a || !selection_b) + return (selection_a == selection_b); Isn't this equivalent to the (more readable) if (selection_a == NULL && selection_b == NULL) return TRUE; ::: libnautilus-private/nautilus-file-utilities.h @@ +96,3 @@ gpointer user_data); +gboolean nautilus_file_selection_equal (GList *selection_a, GList *selection_b); As far as I can see there's nothing special about selections in this function. It could as well be nautilus_file_list_equal and live together with the other nautilus_file_list_* functions in libnautilus-private/nautilus-file.c maybe? ::: src/nautilus-window-slot.c @@ +793,3 @@ } + if (target_window == window && target_slot == slot && Whitespace change @@ +800,1 @@ + if (nautilus_file_selection_equal (old_selection, new_selection)) { You should get old_selection (or the result of nautilus_file_selection_equal()) before the if block, and inline the additional condition in the same block. @@ +809,2 @@ slot->details->pending_use_default_location = ((flags & NAUTILUS_WINDOW_OPEN_FLAG_USE_DEFAULT_LOCATION) != 0); + begin_location_change (target_slot, location, old_location, new_selection, Whitespace change
Review of attachment 236702 [details] [review]: ::: src/nautilus-window.c @@ +224,3 @@ + + if (!NAUTILUS_IS_DESKTOP_WINDOW (window)) { + gtk_window_present (GTK_WINDOW (window)); It shouldn't but, i've just added a safety check. Anyway, at this point I'll just remove this patch, just using gtk_window_present (GTK_WINDOW (window)); when needed @@ +1344,3 @@ gtk_widget_show (GTK_WIDGET (window)); + + nautilus_window_present (window); Mh, I would have preferred to making this done automatically, not to include an extra call in the new code I added, but I'll move to it...
Review of attachment 236706 [details] [review]: ::: src/nautilus-application.c @@ +571,3 @@ + if (g_file_query_file_type (location, G_FILE_QUERY_INFO_NONE, NULL) != G_FILE_TYPE_DIRECTORY) { + location = g_file_get_parent (location); + is_file = TRUE; Yeah, that was my idea too... Then thinking to the fact that reffing involves some atomic ops, I just used the boolean way. However, everything is cleaner in that way, I agree.
Review of attachment 236717 [details] [review]: ::: libnautilus-private/nautilus-file-utilities.c @@ +1303,3 @@ + + if (!selection_a || !selection_b) + return (selection_a == selection_b); No, this includes also the cases where selection_a or selection_b are alternatively NULL i.e.: if (selection_a == NULL && selection_b == NULL) return TRUE; if (selection_a == NULL && selection_b != NULL) return FALSE; if (selection_a != NULL && selection_b == NULL) return FALSE; And considering that after we'll do g_list_length causing an iteration over the lists, if one of them is not-null we'd do an useless iteration over it anyway. So, I preferred my solution to avoid any waste, but just let me know the form you prefer.
Created attachment 237521 [details] [review] 0001: NautilusApplication: don't open a new window if the location is already open in a slot
Created attachment 237522 [details] [review] 0002: NautilusApplication: add --new-window commandline to force the old behavior
Created attachment 237523 [details] [review] 0003: NautilusWindowSlot: allow the location change if selection has been changed
Review of attachment 237521 [details] [review]: Looks good now.
Review of attachment 237522 [details] [review]: OK
Review of attachment 237523 [details] [review]: You should still address the comments from the previous review in this patch. ::: libnautilus-private/nautilus-file-utilities.c @@ +1306,3 @@ + if (selection_a == NULL || selection_b == NULL) { + return (selection_a == selection_b); + GList *al, *bl; I won't nitpick about this anymore - I just don't like sacrificing readability for micro-optimization :) My point was just the additional checks are already covered later in the function, though you're right, it requires at least an iteration of the list. If you feel like this is worth optimizing for, let's leave it this way.
Comment on attachment 237523 [details] [review] 0003: NautilusWindowSlot: allow the location change if selection has been changed Moving this out of the patch queue
Review of attachment 237523 [details] [review]: ::: libnautilus-private/nautilus-file-utilities.c @@ +1306,3 @@ + if (selection_a == NULL || selection_b == NULL) { + return (selection_a == selection_b); + } Well, I consider this quite readable, while I don't like to waste iterations :P It's really not a great gain, but it can happen to be... So why not? Anyway... If your comment it's not blocking as you're saying I'd push it this way... :)
(In reply to comment #42) > Anyway... If your comment it's not blocking as you're saying I'd push it this > way... :) It's fine to keep it that way (there are still a couple of other things left to clean up in the patch though, see comment #31).
Created attachment 237856 [details] [review] 0003: NautilusWindowSlot: allow the location change if selection has been changed
(In reply to comment #43) > (In reply to comment #42) > > > Anyway... If your comment it's not blocking as you're saying I'd push it this > > way... :) > > It's fine to keep it that way (there are still a couple of other things left to > clean up in the patch though, see comment #31). Ah, I thought I already fixed them... Mh, maybe I missed few braces, now it should be ok style side.
Review of attachment 237856 [details] [review]: ::: src/nautilus-window-slot.c @@ +796,3 @@ old_location && g_file_equal (old_location, location) && !is_desktop) { + GList *old_selection = nautilus_view_get_selection (slot->details->content_view); What I was referring to were the comments I had about this section - you can merge this new condition into the existing if instead of nesting another one.
Review of attachment 237856 [details] [review]: ::: src/nautilus-window-slot.c @@ +796,3 @@ old_location && g_file_equal (old_location, location) && !is_desktop) { + GList *old_selection = nautilus_view_get_selection (slot->details->content_view); Ah... I missed this request, sorry. I was nesting it as it could lead to some extra computation, but I'll put everything together.
Created attachment 237887 [details] [review] 0003: NautilusWindowSlot: allow the location change if selection has been changed
Review of attachment 237887 [details] [review]: Thanks, looks good now.
Patches pushed to master, thanks for your reviews! :)