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 694266 - Nautilus should export via DBus a list of opened locations
Nautilus should export via DBus a list of opened locations
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-20 13:39 UTC by Marco Trevisan (Treviño)
Modified: 2013-03-04 23:33 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
0001: NautilusFreedesktopDBus: add public setter for OpenLocations (3.04 KB, patch)
2013-02-27 16:35 UTC, Marco Trevisan (Treviño)
accepted-commit_now Details | Review
0002: NautilusWindowSlot: emit a location change only after setting the new one (1.11 KB, patch)
2013-02-27 16:37 UTC, Marco Trevisan (Treviño)
reviewed Details | Review
0003: NautilusApplication: export currently opened locations to DBus (4.29 KB, patch)
2013-02-27 16:37 UTC, Marco Trevisan (Treviño)
needs-work Details | Review
0002: NautilusWindowSlot: emit a location change only after setting the new one (1.75 KB, patch)
2013-03-01 12:47 UTC, Marco Trevisan (Treviño)
none Details | Review
0003: NautilusApplication: export currently opened locations to DBus (4.41 KB, patch)
2013-03-01 12:48 UTC, Marco Trevisan (Treviño)
reviewed Details | Review
0002: NautilusWindowSlot: emit a location change only after setting the new one (2.29 KB, patch)
2013-03-01 12:57 UTC, Marco Trevisan (Treviño)
needs-work Details | Review
0004: NautilusWindow: don't pass the application on construction (3.87 KB, patch)
2013-03-01 13:10 UTC, Marco Trevisan (Treviño)
reviewed Details | Review
0001: NautilusWindow: don't pass the application on construction (3.04 KB, patch)
2013-03-02 20:21 UTC, Marco Trevisan (Treviño)
accepted-commit_now Details | Review
0002: NautilusFreedesktopDBus: add public setter for OpenLocations (3.04 KB, patch)
2013-03-02 20:23 UTC, Marco Trevisan (Treviño)
accepted-commit_now Details | Review
0003: NautilusWindowSlot: emit a location change only after setting the new one (2.26 KB, patch)
2013-03-02 20:24 UTC, Marco Trevisan (Treviño)
accepted-commit_now Details | Review
0004: NautilusApplication: export currently opened locations to DBus (4.41 KB, patch)
2013-03-02 20:24 UTC, Marco Trevisan (Treviño)
reviewed Details | Review
0004: NautilusApplication: export currently opened locations to DBus (4.08 KB, patch)
2013-03-02 20:56 UTC, Marco Trevisan (Treviño)
accepted-commit_now Details | Review

Description Marco Trevisan (Treviño) 2013-02-20 13:39:19 UTC
Nautilus should add a property in the org.freedesktop.FileManager1 interface that lists all the currently opened locations
Comment 1 Marco Trevisan (Treviño) 2013-02-27 16:35:31 UTC
Created attachment 237530 [details] [review]
0001: NautilusFreedesktopDBus: add public setter for OpenLocations
Comment 2 Marco Trevisan (Treviño) 2013-02-27 16:37:23 UTC
Created attachment 237531 [details] [review]
0002: NautilusWindowSlot: emit a location change only after setting the new one

This patch is not completely related to this change, but it's needed to make everything work as expected. I think it's always a good habit to emit a signal only after that thins have actually been changed internally.
Comment 3 Marco Trevisan (Treviño) 2013-02-27 16:37:51 UTC
Created attachment 237532 [details] [review]
0003: NautilusApplication: export currently opened locations to DBus
Comment 4 Marco Trevisan (Treviño) 2013-02-27 16:43:52 UTC
Review of attachment 237531 [details] [review]:

::: src/nautilus-window-slot.c
@@ +2136,2 @@
 	nautilus_window_slot_set_location (slot, new_location);
+	nautilus_window_slot_emit_location_change (slot, old_location, new_location);

Wouldn't be just better to call nautilus_window_slot_emit_location_change inside nautilus_window_slot_set_location instead?
Comment 5 Marco Trevisan (Treviño) 2013-02-27 16:44:11 UTC
Review of attachment 237532 [details] [review]:

::: src/nautilus-application.c
@@ +1634,3 @@
+
+	nautilus_freedesktop_dbus_set_open_locations (app->priv->fdb_manager,
+		                                      (const gchar**) locations_array);

I've considered if passing a list to this function and doing the conversion between types on it, however doing it here allows us to same some iterations.

@@ +1690,3 @@
+		NautilusWindowSlot *slot = NAUTILUS_WINDOW_SLOT (sl->data);
+		on_slot_added (win, slot, NAUTILUS_APPLICATION (app));
+	}

This is not that bad, but I'm wondering if it could be better to have the GtkApplication set for each window as soon as we create it... Forcing this in NautilusWindow is not a so nice thing, though
Comment 6 Cosimo Cecchi 2013-02-28 04:25:35 UTC
Review of attachment 237532 [details] [review]:

::: src/nautilus-application.c
@@ +1634,3 @@
+
+	nautilus_freedesktop_dbus_set_open_locations (app->priv->fdb_manager,
+				GList *found = g_list_find_custom (locations, uri, (GCompareFunc) g_strcmp0);

Yeah I think it's fine.

@@ +1655,3 @@
+{
+	if (nautilus_window_slot_get_location (slot))
+	}

I don't think this check is needed, since update_dbus_opened_locations() already discards slots that don't have a location URI.

@@ +1667,3 @@
+	update_dbus_opened_locations (application);
+
+		locations_array[i] = l->data;

This will also possibly disconnect other signals that pass the application singleton as user data. You should use g_signal_handlers_disconnect_by_func.

@@ +1690,3 @@
+		NautilusWindowSlot *slot = NAUTILUS_WINDOW_SLOT (sl->data);
+		on_slot_added (win, slot, NAUTILUS_APPLICATION (app));
+static void

That's what should happen already - the "application" property is set when the window is created, and there should be no slots until someone creates one, unless somehow these notifications are defered to an idle.
Where is slot-added being emitted from before this signal?

@@ +1708,3 @@
 	}
+
+	g_signal_handlers_disconnect_by_data (window, app);

Same comment as above for both handlers you install
Comment 7 Cosimo Cecchi 2013-02-28 04:25:57 UTC
Review of attachment 237531 [details] [review]:

::: src/nautilus-window-slot.c
@@ +2136,2 @@
 	nautilus_window_slot_set_location (slot, new_location);
+	nautilus_window_slot_emit_location_change (slot, old_location, new_location);

Yeah, good idea.
Comment 8 Cosimo Cecchi 2013-02-28 04:26:16 UTC
Review of attachment 237530 [details] [review]:

Patch looks good.
The only thing here is this is a spec listed on freedesktop.org [1]. Honestly I don't know if any other file manager implement the spec yet, but we should tell Federico and update it on the website there too.

[1] http://www.freedesktop.org/wiki/Specifications/file-manager-interface
Comment 9 Cosimo Cecchi 2013-02-28 04:26:47 UTC
Federico, what do you think?
Comment 10 Marco Trevisan (Treviño) 2013-02-28 14:59:10 UTC
Review of attachment 237532 [details] [review]:

::: src/nautilus-application.c
@@ +1655,3 @@
+{
+	if (nautilus_window_slot_get_location (slot))
+		update_dbus_opened_locations (application);

Yes, sure, but not adding a simple check like this saves us from a lot of unneeded computation: iterating over the windows, over the slots, allocating memory, and calling the gdbus-generated function that would do other checks to see if something has changed, while it's not.

And considering that this the "get_location" is actually is G_UNLIKELY to return a non-null value, I'd prefer to keep it here.

@@ +1667,3 @@
+	update_dbus_opened_locations (application);
+
+	g_signal_handlers_disconnect_by_data (slot, application);

Mh, Yeah I thought to that but it seemed to me quite safe since that slot won't exist for a long time and at this point we should disconnect from it... However, I'll do what you suggested.

@@ +1690,3 @@
+		NautilusWindowSlot *slot = NAUTILUS_WINDOW_SLOT (sl->data);
+		on_slot_added (win, slot, NAUTILUS_APPLICATION (app));
+	}

You can try it... "slot-added" is emitted when opening the slot in nautilus_window_constructed, and this happens before than "window-added" on GtkApplication.

Forcing a call to:
  application = NAUTILUS_APPLICATION (g_application_get_default ());
  gtk_window_set_application (GTK_WINDOW (window), GTK_APPLICATION (application));

At the beginning of that function will change the things, but that's something I don't like as I supposed that the application was already set on it, while it was not.
Comment 11 Cosimo Cecchi 2013-02-28 15:49:35 UTC
(In reply to comment #10)

> Yes, sure, but not adding a simple check like this saves us from a lot of
> unneeded computation: iterating over the windows, over the slots, allocating
> memory, and calling the gdbus-generated function that would do other checks to
> see if something has changed, while it's not.
> 
> And considering that this the "get_location" is actually is G_UNLIKELY to
> return a non-null value, I'd prefer to keep it here.

OK, fair enough.

> You can try it... "slot-added" is emitted when opening the slot in
> nautilus_window_constructed, and this happens before than "window-added" on
> GtkApplication.

Oh right! I missed we are actually calling nautilus_window_open_slot() in constructed.

> Forcing a call to:
>   application = NAUTILUS_APPLICATION (g_application_get_default ());
>   gtk_window_set_application (GTK_WINDOW (window), GTK_APPLICATION
> (application));
> 
> At the beginning of that function will change the things, but that's something
> I don't like as I supposed that the application was already set on it, while it
> was not.

The application is guaranteed to be a singleton, so it doesn't really matter. This is only a problem because "application" is not a construct property of GtkWindow, so it won't be set after we chain up in _constructed() - it will be set by GObject after we return from there.
If we go for explicitly setting the application on the GtkWindow (which I think is fine), we should just remove the GtkApplication parameter from nautilus_window_new(), and just call the setter manually in nautilus_window_init or nautilus_window_constructed.
Comment 12 Marco Trevisan (Treviño) 2013-03-01 12:47:32 UTC
Created attachment 237704 [details] [review]
0002: NautilusWindowSlot: emit a location change only after setting the new one
Comment 13 Marco Trevisan (Treviño) 2013-03-01 12:48:02 UTC
Created attachment 237705 [details] [review]
0003: NautilusApplication: export currently opened locations to DBus
Comment 14 Marco Trevisan (Treviño) 2013-03-01 12:57:07 UTC
Created attachment 237706 [details] [review]
0002: NautilusWindowSlot: emit a location change only after setting the new one

Ops, fixed compilation issue...
Comment 15 Marco Trevisan (Treviño) 2013-03-01 13:10:53 UTC
Created attachment 237707 [details] [review]
0004: NautilusWindow: don't pass the application on construction

I've also made an extra patch to set the GtkApplication on constructed so that it's set as soon as we create the window.

Removed the extra code needed to have proper dbus paths exported, due to this issue.
Comment 16 Cosimo Cecchi 2013-03-01 16:10:24 UTC
Review of attachment 237706 [details] [review]:

::: src/nautilus-window-slot.c
@@ +975,3 @@
 	}
+	else if (!slot->details->location && !location) {
+		return;

I don't think this is needed - the function assumed the new location was != NULL before (it unconditionally reffed it to slot->details->location).

You should also check for (old_location != NULL) before unfeffing it later (or use g_clear_object instead).
Comment 17 Cosimo Cecchi 2013-03-01 16:11:54 UTC
Review of attachment 237707 [details] [review]:

OK, but move this patch before in the stack, and avoid adding and removing the slot iteration code.
Comment 18 Cosimo Cecchi 2013-03-01 16:14:23 UTC
Review of attachment 237705 [details] [review]:

This also needs some changes for the previous comment

::: src/nautilus-application.c
@@ +1654,3 @@
+	       NautilusApplication *application)
+{
+	}

I think the G_UNLIKELY here is a bit too much, and I don't like using it in applications' code. Also, missing braces around the block.
Comment 19 Marco Trevisan (Treviño) 2013-03-02 20:06:58 UTC
Review of attachment 237706 [details] [review]:

::: src/nautilus-window-slot.c
@@ +975,3 @@
 	}
+	else if (!slot->details->location && !location) {
+		return;

Yes indeed... For some reason I was confused that g_object_unref was just skipping NULL objects. Sorry :/

Ok for removing the check... I just added it since it looked something that we didn't care about.
Comment 20 Marco Trevisan (Treviño) 2013-03-02 20:19:34 UTC
Review of attachment 237705 [details] [review]:

::: src/nautilus-application.c
@@ +1654,3 @@
+	       NautilusApplication *application)
+{
+	if (G_UNLIKELY (nautilus_window_slot_get_location (slot)))

Ok, I added this only to underline this, as we discussed this fact (also it should lead to no optimization since we don't have any else statement here).

Reverting back...
Comment 21 Marco Trevisan (Treviño) 2013-03-02 20:21:13 UTC
Created attachment 237845 [details] [review]
0001: NautilusWindow: don't pass the application on construction
Comment 22 Marco Trevisan (Treviño) 2013-03-02 20:23:29 UTC
Created attachment 237846 [details] [review]
0002: NautilusFreedesktopDBus: add public setter for OpenLocations
Comment 23 Marco Trevisan (Treviño) 2013-03-02 20:24:08 UTC
Created attachment 237847 [details] [review]
0003: NautilusWindowSlot: emit a location change only after setting the new one
Comment 24 Marco Trevisan (Treviño) 2013-03-02 20:24:44 UTC
Created attachment 237848 [details] [review]
0004: NautilusApplication: export currently opened locations to DBus
Comment 25 Cosimo Cecchi 2013-03-02 20:39:28 UTC
Review of attachment 237845 [details] [review]:

Looks good
Comment 26 Cosimo Cecchi 2013-03-02 20:39:46 UTC
Review of attachment 237847 [details] [review]:

OK
Comment 27 Cosimo Cecchi 2013-03-02 20:40:57 UTC
Review of attachment 237848 [details] [review]:

::: src/nautilus-application.c
@@ +1688,3 @@
+
+	/* We need to manually do this on window-added, because the 'slot-added'
+}

We can remove this now, right?
Comment 28 Marco Trevisan (Treviño) 2013-03-02 20:54:12 UTC
Review of attachment 237848 [details] [review]:

::: src/nautilus-application.c
@@ +1688,3 @@
+
+	/* We need to manually do this on window-added, because the 'slot-added'
+	 * signal for a newly-opened window is emitted before of this call */

Ups, yes... Sorry I forgot about that when restacking... Doing it now.
Comment 29 Marco Trevisan (Treviño) 2013-03-02 20:56:03 UTC
Created attachment 237852 [details] [review]
0004: NautilusApplication: export currently opened locations to DBus
Comment 30 Cosimo Cecchi 2013-03-02 22:12:03 UTC
Review of attachment 237852 [details] [review]:

Looks good.
Comment 31 Marco Trevisan (Treviño) 2013-03-04 09:54:57 UTC
Patches pushed to master, thanks for your reviews! :)
Comment 32 Federico Mena Quintero 2013-03-04 23:20:23 UTC
This is great!  Thanks for all the work, Marco.

Indeed, http://www.freedesktop.org/wiki/Specifications/file-manager-interface needs updating to have the new OpenLocations property.

What is the convention for adding stuff to DBus interfaces?  Do new things simply get tacked onto an existing interface, or would we use a FileManager2 interface with the new property?

(I.e. how would an application figure out if the file manager supports the new property?)
Comment 33 Marco Trevisan (Treviño) 2013-03-04 23:33:15 UTC
I think an application can just try to get the property to see if it's supported, and this can happen without troubles, but I don't know what is the policy here in Gnome, I guess Cosimo has a better answer.