GNOME Bugzilla – Bug 694266
Nautilus should export via DBus a list of opened locations
Last modified: 2013-03-04 23:33:15 UTC
Nautilus should add a property in the org.freedesktop.FileManager1 interface that lists all the currently opened locations
Created attachment 237530 [details] [review] 0001: NautilusFreedesktopDBus: add public setter for OpenLocations
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.
Created attachment 237532 [details] [review] 0003: NautilusApplication: export currently opened locations to DBus
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?
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
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
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.
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
Federico, what do you think?
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.
(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.
Created attachment 237704 [details] [review] 0002: NautilusWindowSlot: emit a location change only after setting the new one
Created attachment 237705 [details] [review] 0003: NautilusApplication: export currently opened locations to DBus
Created attachment 237706 [details] [review] 0002: NautilusWindowSlot: emit a location change only after setting the new one Ops, fixed compilation issue...
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.
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).
Review of attachment 237707 [details] [review]: OK, but move this patch before in the stack, and avoid adding and removing the slot iteration code.
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.
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.
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...
Created attachment 237845 [details] [review] 0001: NautilusWindow: don't pass the application on construction
Created attachment 237846 [details] [review] 0002: NautilusFreedesktopDBus: add public setter for OpenLocations
Created attachment 237847 [details] [review] 0003: NautilusWindowSlot: emit a location change only after setting the new one
Created attachment 237848 [details] [review] 0004: NautilusApplication: export currently opened locations to DBus
Review of attachment 237845 [details] [review]: Looks good
Review of attachment 237847 [details] [review]: OK
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?
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.
Created attachment 237852 [details] [review] 0004: NautilusApplication: export currently opened locations to DBus
Review of attachment 237852 [details] [review]: Looks good.
Patches pushed to master, thanks for your reviews! :)
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?)
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.