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 694034 - Nautilus should optionally reuse already opened views before opening a new window
Nautilus should optionally reuse already opened views before opening a new wi...
Status: RESOLVED FIXED
Product: nautilus
Classification: Core
Component: general
unspecified
Other Linux
: Normal normal
: ---
Assigned To: Marco Trevisan (Treviño)
Nautilus Maintainers
Depends on:
Blocks:
 
 
Reported: 2013-02-17 18:18 UTC by Marco Trevisan (Treviño)
Modified: 2013-03-04 09:55 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
0001: NautilusWindow: present a window when grabbing focus (1.45 KB, patch)
2013-02-17 18:20 UTC, Marco Trevisan (Treviño)
reviewed Details | Review
0002: NautilusApplication: don't open a new window if the location is already open in a slot (4.01 KB, patch)
2013-02-17 18:23 UTC, Marco Trevisan (Treviño)
needs-work Details | Review
0003: NautilusApplication: add --new-window commandline to force the old behavior (4.73 KB, patch)
2013-02-17 18:23 UTC, Marco Trevisan (Treviño)
needs-work Details | Review
0004: NautilusWindowSlot: allow the location change if selection has been changed (2.36 KB, patch)
2013-02-17 18:24 UTC, Marco Trevisan (Treviño)
needs-work Details | Review
0002: NautilusApplication: don't open a new window if the location is already open in a slot (3.62 KB, patch)
2013-02-19 01:46 UTC, Marco Trevisan (Treviño)
none Details | Review
0003: NautilusApplication: add --new-window commandline to force the old behavior (4.57 KB, patch)
2013-02-19 01:47 UTC, Marco Trevisan (Treviño)
none Details | Review
0004: NautilusWindowSlot: allow the location change if selection has been changed (3.51 KB, patch)
2013-02-19 02:28 UTC, Marco Trevisan (Treviño)
none Details | Review
0001: NautilusWindow: present a window when grabbing focus (2.46 KB, patch)
2013-02-19 02:41 UTC, Marco Trevisan (Treviño)
reviewed Details | Review
0002: NautilusApplication: don't open a new window if the location is already open in a slot (3.61 KB, patch)
2013-02-19 02:42 UTC, Marco Trevisan (Treviño)
none Details | Review
0003: NautilusApplication: add --new-window commandline to force the old behavior (4.57 KB, patch)
2013-02-19 02:43 UTC, Marco Trevisan (Treviño)
accepted-commit_now Details | Review
0004: NautilusWindowSlot: allow the location change if selection has been changed (3.51 KB, patch)
2013-02-19 02:43 UTC, Marco Trevisan (Treviño)
none Details | Review
0002: NautilusApplication: don't open a new window if the location is already open in a slot (3.88 KB, patch)
2013-02-19 03:08 UTC, Marco Trevisan (Treviño)
reviewed Details | Review
0004: NautilusWindowSlot: allow the location change if selection has been changed (3.59 KB, patch)
2013-02-19 03:25 UTC, Marco Trevisan (Treviño)
none Details | Review
0004: NautilusWindowSlot: allow the location change if selection has been changed (3.59 KB, patch)
2013-02-19 04:05 UTC, Marco Trevisan (Treviño)
needs-work Details | Review
0001: NautilusApplication: don't open a new window if the location is already open in a slot (3.89 KB, patch)
2013-02-27 13:45 UTC, Marco Trevisan (Treviño)
accepted-commit_now Details | Review
0002: NautilusApplication: add --new-window commandline to force the old behavior (4.28 KB, patch)
2013-02-27 13:46 UTC, Marco Trevisan (Treviño)
accepted-commit_now Details | Review
0003: NautilusWindowSlot: allow the location change if selection has been changed (3.62 KB, patch)
2013-02-27 13:46 UTC, Marco Trevisan (Treviño)
needs-work Details | Review
0003: NautilusWindowSlot: allow the location change if selection has been changed (3.64 KB, patch)
2013-03-02 23:31 UTC, Marco Trevisan (Treviño)
reviewed Details | Review
0003: NautilusWindowSlot: allow the location change if selection has been changed (3.55 KB, patch)
2013-03-03 18:34 UTC, Marco Trevisan (Treviño)
accepted-commit_now Details | Review

Description Marco Trevisan (Treviño) 2013-02-17 18:18:11 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.
Comment 1 Marco Trevisan (Treviño) 2013-02-17 18:20:30 UTC
Created attachment 236470 [details] [review]
0001: NautilusWindow: present a window when grabbing focus
Comment 2 Marco Trevisan (Treviño) 2013-02-17 18:23:03 UTC
Created attachment 236471 [details] [review]
0002: NautilusApplication: don't open a new window if the location is already open in a slot
Comment 3 Marco Trevisan (Treviño) 2013-02-17 18:23:46 UTC
Created attachment 236472 [details] [review]
0003: NautilusApplication: add --new-window commandline to force the old behavior
Comment 4 Marco Trevisan (Treviño) 2013-02-17 18:24:10 UTC
Created attachment 236473 [details] [review]
0004: NautilusWindowSlot: allow the location change if selection has been changed
Comment 5 Marco Trevisan (Treviño) 2013-02-17 18:26:54 UTC
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.
Comment 6 Cosimo Cecchi 2013-02-18 15:42:18 UTC
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
Comment 7 Cosimo Cecchi 2013-02-18 15:42:32 UTC
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
Comment 8 Cosimo Cecchi 2013-02-18 15:42:49 UTC
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.
Comment 9 Cosimo Cecchi 2013-02-18 15:45:50 UTC
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.
Comment 10 Marco Trevisan (Treviño) 2013-02-19 01:13:58 UTC
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...
Comment 11 Marco Trevisan (Treviño) 2013-02-19 01:26:58 UTC
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!
Comment 12 Marco Trevisan (Treviño) 2013-02-19 01:40:06 UTC
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?
Comment 13 Marco Trevisan (Treviño) 2013-02-19 01:46:08 UTC
Created attachment 236693 [details] [review]
0002: NautilusApplication: don't open a new window if the location is already open in a slot
Comment 14 Marco Trevisan (Treviño) 2013-02-19 01:47:39 UTC
Created attachment 236694 [details] [review]
0003: NautilusApplication: add --new-window commandline to force the old behavior
Comment 15 Cosimo Cecchi 2013-02-19 01:57:57 UTC
(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.
Comment 16 Cosimo Cecchi 2013-02-19 02:02:30 UTC
(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.
Comment 17 Marco Trevisan (Treviño) 2013-02-19 02:07:45 UTC
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.
Comment 18 Marco Trevisan (Treviño) 2013-02-19 02:28:11 UTC
Created attachment 236701 [details] [review]
0004: NautilusWindowSlot: allow the location change if selection has been changed
Comment 19 Cosimo Cecchi 2013-02-19 02:41:20 UTC
(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.
Comment 20 Marco Trevisan (Treviño) 2013-02-19 02:41:49 UTC
Created attachment 236702 [details] [review]
0001: NautilusWindow: present a window when grabbing focus
Comment 21 Marco Trevisan (Treviño) 2013-02-19 02:42:19 UTC
Created attachment 236703 [details] [review]
0002: NautilusApplication: don't open a new window if the location is already open in a slot
Comment 22 Marco Trevisan (Treviño) 2013-02-19 02:43:02 UTC
Created attachment 236704 [details] [review]
0003: NautilusApplication: add --new-window commandline to force the old behavior
Comment 23 Marco Trevisan (Treviño) 2013-02-19 02:43:23 UTC
Created attachment 236705 [details] [review]
0004: NautilusWindowSlot: allow the location change if selection has been changed
Comment 24 Marco Trevisan (Treviño) 2013-02-19 02:47:48 UTC
(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
Comment 25 Marco Trevisan (Treviño) 2013-02-19 03:08:09 UTC
Created attachment 236706 [details] [review]
0002: NautilusApplication: don't open a new window if the location is already open in a slot
Comment 26 Marco Trevisan (Treviño) 2013-02-19 03:25:19 UTC
Created attachment 236707 [details] [review]
0004: NautilusWindowSlot: allow the location change if selection has been changed
Comment 27 Marco Trevisan (Treviño) 2013-02-19 04:05:26 UTC
Created attachment 236717 [details] [review]
0004: NautilusWindowSlot: allow the location change if selection has been changed
Comment 28 Cosimo Cecchi 2013-02-25 23:44:16 UTC
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
Comment 29 Cosimo Cecchi 2013-02-25 23:45:18 UTC
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.
Comment 30 Cosimo Cecchi 2013-02-25 23:46:19 UTC
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.
Comment 31 Cosimo Cecchi 2013-02-25 23:46:46 UTC
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
Comment 32 Marco Trevisan (Treviño) 2013-02-27 13:43:07 UTC
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...
Comment 33 Marco Trevisan (Treviño) 2013-02-27 13:43:16 UTC
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.
Comment 34 Marco Trevisan (Treviño) 2013-02-27 13:43:30 UTC
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.
Comment 35 Marco Trevisan (Treviño) 2013-02-27 13:45:25 UTC
Created attachment 237521 [details] [review]
0001: NautilusApplication: don't open a new window if the location is already open in a slot
Comment 36 Marco Trevisan (Treviño) 2013-02-27 13:46:20 UTC
Created attachment 237522 [details] [review]
0002: NautilusApplication: add --new-window commandline to force the old behavior
Comment 37 Marco Trevisan (Treviño) 2013-02-27 13:46:56 UTC
Created attachment 237523 [details] [review]
0003: NautilusWindowSlot: allow the location change if selection has been changed
Comment 38 Cosimo Cecchi 2013-03-01 16:04:29 UTC
Review of attachment 237521 [details] [review]:

Looks good now.
Comment 39 Cosimo Cecchi 2013-03-01 16:04:39 UTC
Review of attachment 237522 [details] [review]:

OK
Comment 40 Cosimo Cecchi 2013-03-01 16:04:54 UTC
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 41 Cosimo Cecchi 2013-03-01 16:21:10 UTC
Comment on attachment 237523 [details] [review]
0003: NautilusWindowSlot: allow the location change if selection has been changed

Moving this out of the patch queue
Comment 42 Marco Trevisan (Treviño) 2013-03-02 20:35:42 UTC
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... :)
Comment 43 Cosimo Cecchi 2013-03-02 22:17:15 UTC
(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).
Comment 44 Marco Trevisan (Treviño) 2013-03-02 23:31:50 UTC
Created attachment 237856 [details] [review]
0003: NautilusWindowSlot: allow the location change if selection has been changed
Comment 45 Marco Trevisan (Treviño) 2013-03-02 23:32:37 UTC
(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.
Comment 46 Cosimo Cecchi 2013-03-03 16:15:03 UTC
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.
Comment 47 Marco Trevisan (Treviño) 2013-03-03 18:26:43 UTC
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.
Comment 48 Marco Trevisan (Treviño) 2013-03-03 18:34:49 UTC
Created attachment 237887 [details] [review]
0003: NautilusWindowSlot: allow the location change if selection has been changed
Comment 49 Cosimo Cecchi 2013-03-03 20:45:45 UTC
Review of attachment 237887 [details] [review]:

Thanks, looks good now.
Comment 50 Marco Trevisan (Treviño) 2013-03-04 09:55:34 UTC
Patches pushed to master, thanks for your reviews! :)