GNOME Bugzilla – Bug 752034
Delegate permanent devices and connected networks from Places Sidebar
Last modified: 2015-07-16 02:17:43 UTC
From the latest mockups from the Design Team [1], GtkPlacesSidebar delegates the ability to handle permanent devices (such as partitions and hard disks) to other widgets. The mockups are directed to Nautilus but, as discussed with the Design Team, Gtk+ should follow the same pattern in order to keep consistency between GNOME file management tools. As such, GtkFileChooserWidget should also follow this convention. [1] https://raw.githubusercontent.com/gnome-design-team/gnome-mockups/master/nautilus/nautilus-next/other-locations.png
There are some differences between nautilus and the file chooser that would be nice to consider before getting too deep into this: - the file chooser has different modes (actually called actions: open, save, select-folder, create-folder), whereas nautilus basically always operates in 'open' mode. Does this work need adjustments for the other modes ? - the file chooser has a 'local-only' mode in which it only browses local file systems; the design should accommodate this - how does the flow look beyond the 'other locations' page ? clicking on one of those locations gets you a regular file list ? - the mockup shows a pathbar in the header, but in the file chooser we chose not to go that way; where will the pathbar go ?
(In reply to Matthias Clasen from comment #1) > - the file chooser has different modes (actually called actions: open, save, > select-folder, create-folder), whereas nautilus basically always operates in > 'open' mode. Does this work need adjustments for the other modes ? No, this is a simple portal-like widget that redirects users to the desired locations. It isn't aware of the modes, and it shouldn't by design. > - the file chooser has a 'local-only' mode in which it only browses local > file systems; the design should accommodate this Indeed. > - how does the flow look beyond the 'other locations' page ? clicking on one > of those locations gets you a regular file list ? Selecting a location or connecting to a network will "send" the user to that location, just like if he/she just selected another folder. > - the mockup shows a pathbar in the header, but in the file chooser we chose > not to go that way; where will the pathbar go ? The pathbar shall be hidden in file chooser.
(In reply to Georges Basile Stavracas Neto from comment #2) > (In reply to Matthias Clasen from comment #1) > > > - the file chooser has different modes (actually called actions: open, save, > > select-folder, create-folder), whereas nautilus basically always operates in > > 'open' mode. Does this work need adjustments for the other modes ? > > No, this is a simple portal-like widget that redirects users to the desired > locations. It isn't aware of the modes, and it shouldn't by design. > But if a location is read-only (e.g. a cd-rom), maybe we don't want to show it in save mode ?
(In reply to Matthias Clasen from comment #1) > There are some differences between nautilus and the file chooser that would > be nice to consider before getting too deep into this: > > - the file chooser has different modes (actually called actions: open, save, > select-folder, create-folder), whereas nautilus basically always operates in > 'open' mode. Does this work need adjustments for the other modes ? Those actions seem equally applicable to local volumes or network locations. As such, the other locations list should work for all. > - the file chooser has a 'local-only' mode in which it only browses local > file systems; the design should accommodate this In local only mode, "Other Locations" would only show local volumes (and not network locations). Remote locations would also be hidden in the sidebar. > - the mockup shows a pathbar in the header, but in the file chooser we chose > not to go that way; where will the pathbar go ? I assume that it would go in the same position it currently does - above the content list.
Created attachment 307030 [details] [review] add gtkplacesview placesview: add view for fixed drives and networks Places sidebar shows XDG directories, mounted and unmounted devices, connected networks, bookmarks and actions like 'Connect to server' and 'Insert location', which causes the sidebar to grow very quickly and look cluttered. Because of that, new mockups for the sidebar try to simplify it. To make the sidebar simpler, the new mockups propose that it should only handle connected networks and removable devices such as flash drives and USB devices, and delegates other devices for external widgets through the 'Other Locations' item. To handle fixed devices and manage network connections, add a new widget named GtkPlacesView, based on Nautilus mockups to keep consistency between GNOME file management tools - in this case, between Nautilus and the bundled Gtk's file chooser.
Created attachment 307031 [details] [review] placessidebar: add Other Locations item placessidebar: add Other Locations item Places sidebar is a widget that enabled the user to select XDG directories, bookmarks and mounted network locations, as well as manages permanent and removable devices. The new design that aims to look less clutered makes the sidebar display only removable devices, as well as mounted networks, bookmarks and XDG directories, and delegates the management of permanent devices such as hard drive partitions to GtkPlacesView, a newly introduced widget for this specific purpose. To delegate it, add an "Other Locations..." item to notify when the permanent devices manager is required. Besides that, don't show these fixes devices on the sidebar itself, as they not supposed to be handled by the sidebar anymore.
Created attachment 307032 [details] [review] filechooserwidget: use places view to manage fixed devices The preview patch modified places sidebar widget to stop handling fixed devices by adding an "Other Locations..." item. Up to now, however, these changes are isolated from each other since the bundled file manager widgets ignore the sidebar requests for external management of fixed devices and networks. To fix that, make the file chooser widget be aware of the GtkPlacesSidebar::show-other-locations signal and, when requested, show places view to manage the fixed devices and networks.
Small thing I noticed while playing with this for 5 minutes: If I open the devices page, and then enter search, the sidebar keeps 'other locations' selected (and thus, the subtitle says 'Searching in other locations', but the page switches back to the list showing current_folder contents. I think the sidebar selection needs to be updated in this case.
When opening and browsing a network location, the pathbar is not updated.
There should be some feedback while a mount operation is in progress, I end up double-clicking multiple times.
Maybe hide section headings if the sections are entirely empty ?
Should I squash these 3 patches into one single patch?
Might be nice to have some empty state filler for the recent servers popover. Also, the horizontal scrolling in there is somewhat awkward with a non-resizable popover.
Last comment: the filter combo should be insensitive in the places view. See what we do for search, e.g
Review of attachment 307030 [details] [review]: ::: gtk/gtkplacesview.c @@ +30,3 @@ +/** + * SECTION:gtkplacesview + * @Short_description: Widget that displays permanent drives and manages mounted networks Not sure the word permanent is a good one here. Maybe you mean 'persistent' (as opposed to 'transient') here ? Or maybe just 'non-removable' ? @@ +34,3 @@ + * @See_also: #GtkFileChooser + * + * #GtkPlacesView is a stock widget that displays a list permanent drives such list of @@ +39,3 @@ + * + * The places view displays drives and networks, and will automatically mount + * them when the user selects them. Network addresses are stored even if they I think technically, what the user has to do is 'activate' them, not 'select' @@ +40,3 @@ + * The places view displays drives and networks, and will automatically mount + * them when the user selects them. Network addresses are stored even if they + * fail to connect. When the connection is successfull, the connected network successful - only one l @@ +101,3 @@ + "archive", "recent", "localtest", + NULL +}; Hmm, not the most future-proof way to go - I guess there's no better way to determine what you mean by 'permanent' ? @@ +1238,3 @@ + { + activate_row (view, row, GTK_PLACES_OPEN_NORMAL); + } This would seem nicer to split in two separate callbacks, one for each listbox @@ +1349,3 @@ + GTK_TYPE_PLACES_OPEN_FLAGS, + GTK_PLACES_OPEN_NORMAL, + G_PARAM_READWRITE); This property seems to not be implemented at all. ::: gtk/gtkplacesviewprivate.h @@ +26,3 @@ + +#include <gtk/gtkbox.h> + I think you are missing a #include <gtkplacessidebar.h> here (for GtkPlacesOpenFlags)
Review of attachment 307031 [details] [review]: ::: gtk/gtkplacessidebar.c @@ +1194,3 @@ + add_place (sidebar, PLACES_OTHER_LOCATIONS, + SECTION_OTHER_LOCATIONS, + _("Other Locations"), icon, "other-locations://", A bit ugly to use a made-up scheme like that here - is this necessary for something to work ? We seem to add plenty of other places with a NULL uri... @@ +4181,3 @@ + * + * The places sidebar emits this signal when it needs the calling + * application to present an way to show other locations e.g. drives 'a way'. I think it would be helpful to mention GtkPlacesView here as the prime example for how to do this. @@ +4250,3 @@ + g_param_spec_boolean ("show-other-locations", + P_("Show 'Other locations'"), + P_("Whether the sidebar includes an item to show customly external locations"), customly ? Not sure what you are tryin gto say here, I would just strike that word @@ +4644,3 @@ + * view; this is off by default. + * An application may want to turn this on if it implements a way for the user to see and interact + * to drives and network servers directly. 'interact with'. Again, mentioning GtkPlacesView here would be helpful.
Review of attachment 307032 [details] [review]: ::: gtk/gtkfilechooserwidget.c @@ +8315,3 @@ gtk_widget_class_bind_template_callback (widget_class, path_bar_clicked); gtk_widget_class_bind_template_callback (widget_class, places_sidebar_open_location_cb); gtk_widget_class_bind_template_callback (widget_class, places_sidebar_show_error_message_cb); Is places_sidebar_show_error_message_cb defined anywhere ? ::: gtk/ui/gtkfilechooserwidget.ui @@ +246,3 @@ + <property name="visible">True</property> + <property name="can_focus">True</property> + <property name="local_only">True</property> You could bind local-only to the file chooser widget's property of the same name here, using GtkBuilder's binding support.
Created attachment 307184 [details] [review] placesview: add view for fixed drives and networks Proposed changes fixed. I'm sticking with the placeholder text present on mockups. I personally prefer feedback from the design team here.
Created attachment 307186 [details] [review] placessidebar: add Other Locations item Fixed the typos and proposed changes. It sets the URI to 'other-locations://" so that we can select it programatically.
Created attachment 307187 [details] [review] filechooserwidget: use places view to manage fixed devices Fixed the pointed issues. The pathbar not being updated is actually another bug, which will be corrected in future patches.
Hi Gerges, thanks for the patches! Trying them I had a few issues: - As Mathias said, it doesn't show progress for me when opening/mounting owncloud. - Seems single clicks are not always accepted in the owncloud row, only when right click -> open it actually opens. - It doesn't show progress when connecting to a server in the connect to server. - I have no permanent volumes, so I have a "This computer" label with anything below, and then the "Network" label. Empty groups should be hidden. - There is no empty state if there is not permanent volumes and no network. An empty view is odd. - I would say the popover should fit the items inside until it hits a max-height, so we don't have empty place when there is only one item. I know it's not possible with a simple GtkWindow, I will talk with Mathias because I have the same use case. gnome-builder solves it subclassing GTkWindow and adding a max-height property. I will take care of this. - As you said, GtkPathBar doesn't work well. I will take care of this as well. - Search is available in the Other-locations view. Either disable it since it doesn't work or make it work filtering rows and servers. (Disabling search is fine for now) - The autocompletion popup in connect to server doesn't work at all since it resides in the bottom, so the list is just 2 px taller, making impossible to see the items inside. - I connected to ftp://ftp.redhat.com to try how it works. It has a few issues: -The entry is added to the sidebar and added to the "This computer" section on the other-locations. So I think that needs to be in the "Network" section and also removed from the sidebar. - There is no way to remove an entry on the recent servers. We know this will be something to complain about from people that cares about privacy. - The uri displayed for the ftp.redhat.com address is kinda odd. Something like /run/user/etc. Use the web address the user wrote for these cases (look how the GtkPathBar acts in this case, it doesn't show a non-understandable path). I think is all I could see for now :)
(In reply to Carlos Soriano from comment #21) > - As Mathias said, it doesn't show progress for me when opening/mounting > owncloud. > - Seems single clicks are not always accepted in the owncloud row, only when > right click -> open it actually opens. These are already fixed issues. Which patch did you tested?
(In reply to Carlos Soriano from comment #21) > - It doesn't show progress when connecting to a server in the connect to > server. Showing the progress with the entry's pulses seems like the obvious answer here. I'll do that. > - I have no permanent volumes, so I have a "This computer" label with > anything below, and then the "Network" label. Empty groups should be hidden. This was also fixed with the last patch. > - There is no empty state if there is not permanent volumes and no network. > An empty view is odd. I'll fix this too, but some input from the Design Team here is needed. At least for the wording of the empty view. > - Search is available in the Other-locations view. Either disable it since > it doesn't work or make it work filtering rows and servers. (Disabling > search is fine for now) Ok. > - The autocompletion popup in connect to server doesn't work at all since it > resides in the bottom, so the list is just 2 px taller, making impossible to > see the items inside. I also removed the completion popover in the last patch. > > - I connected to ftp://ftp.redhat.com to try how it works. It has a few > issues: > -The entry is added to the sidebar and added to the "This computer" > section on the other-locations. So I think that needs to be in the "Network" > section and also removed from the sidebar. This was discussed with Allan some time ago. It should appear both on the sidebar and on the view too. > - There is no way to remove an entry on the recent servers. We know this > will be something to complain about from people that cares about privacy. I have absolutely no idea how this can be implemented (in terms of interface) without cluterring the view. Again, some input from the Design Team is important here. > - The uri displayed for the ftp.redhat.com address is kinda odd. Something > like /run/user/etc. Use the web address the user wrote for these cases (look > how the GtkPathBar acts in this case, it doesn't show a non-understandable > path). Ok.
(In reply to Carlos Soriano from comment #21) > - I would say the popover should fit the items inside until it hits a > max-height, so we don't have empty place when there is only one item. I know > it's not possible with a simple GtkWindow, I will talk with Mathias because > I have the same use case. gnome-builder solves it subclassing GTkWindow and > adding a max-height property. I will take care of this. GtkWindow is not involved here. You need bug 742281 > - The autocompletion popup in connect to server doesn't work at all since it > resides in the bottom, so the list is just 2 px taller, making impossible to > see the items inside. I've analysed this in another bug: it was a case of missing cell renderer. > - There is no way to remove an entry on the recent servers. We know this > will be something to complain about from people that cares about privacy. Regardless of adding a clear button in nautilus, this should be covered in the privacy panel, in the same place where we clear recent files.
Trying the latest attached patches again: - I see a spinner while connecting to a network location now - good. But I think this should follow the rest of the file chooser and also set a busy cursor. This will need some plumbing to make sure you properly integrate it with the managment of the busy cursor in gtkfilechooserwidget.c. - I didn't see the mounted network location being added to the sidebar, as Carlos seemed to see with entering a server. Is it intentional that these two are being treated differently ? - Another curiosity: After mounting an sftp location (sftp://matthiasc@master.gnome.org), I see it appear under "This Computer", which seems very dubious. I would have expected it either show up under "Network", or in a new "Connected Servers" section. - I would suggest to use the same image button approach for mount/unmount that we use in the places sidebar, instead of the somewhat clumsy context menu. - When I unmount a connected server location from the places sidebar, the file list continues to show it (probably not your bug) - When clicking on a mounted network location in the places view, I see the current contents of the file list (some local directory) for a brief moment before it is emptied and the remote contents appear (also probably not your bug) - The places view seems entirely non-keyboard-navigatable. Tab does not seem to enter the view at all; I can't seem to put keyboard focus into the view sections, and even when I focus the connect to server entry, hitting tab does not move the focus to the connect button.
(In reply to Georges Basile Stavracas Neto from comment #18) > Proposed changes fixed. > > I'm sticking with the placeholder text present on mockups. I personally > prefer feedback from the design team here. In my experience, interaction with the design team is most productive if you don't treat them as the ultimate authority on every minor UI change, but rather as equal partners. In the case of the connect to server row, I would suggest something like this: Connect to _Server [ Address... |^] [ _Connect ] (with _S and _C being mnemonics, and "Address..." the entry placeholder text) instead of the current Connect to Server Address [ For example: smb://foo.ex... |^] [ Connect ] Another observation on the entry: it should be cleared when the view is shown. Currently, I can enter some string there, navigate away from the places view, and when I come back, my old nonsense is still there.
(In reply to Georges Basile Stavracas Neto from comment #23) > I have absolutely no idea how this can be implemented (in terms of > interface) without cluterring the view. Again, some input from the Design > Team is important here. Some suggestions: - Add a delete image button at the right end of each row. Could make it appear only on hover. The horizontal scrolling is still really problematic here, and should go away. - Bind the Delete key to this action - Add a context menu with this action - Add a "Clear list" imae button at the top right
Two observations about the handling of the servers file: - We don't seem to monitor the file - I edited it manually, and the changes did not make it into my running application. That means that long-running applications will overwrite each others additions to this file with the older content they've already in memory... - I had manually added a few locations with titles in the servers file. When mounting them, they loose their title. It appears the code replacing my manually set title with and (empty) display name. Or maybe getting the display name just failed... I observed: (lt-testfilechooser:4444): GLib-GIO-CRITICAL **: _g_file_info_get_attribute_value: assertion 'G_IS_FILE_INFO (info)' failed
(In reply to Georges Basile Stavracas Neto from comment #19) > Created attachment 307186 [details] [review] [review] > placessidebar: add Other Locations item > > Fixed the typos and proposed changes. > > It sets the URI to 'other-locations://" so that we can select it > programatically. Look at "Enter Location" for an example of adding a 'place' without a made-up uri that can still be selected programmatically.
(In reply to Matthias Clasen from comment #25) > - When clicking on a mounted network location in the places view, I see the > current contents of the file list (some local directory) for a brief moment > before it is emptied and the remote contents appear (also probably not your > bug) I briefly looked at this, and it does look like it is at least partially something to address in your file chooser integration: The way loading a uri works is that we start async loading of the folder, then call operation_mode_set (... BROWSE), which makes the file list visible, then clear it and start filling in new contents as the async operation returns. This is fine if you are already in the file list (and say, load a new uri by clicking on a folder in the list). In the case of "Other Location", we should clear the file list from old contents before showing it. I would would suggest that the cleanest way to integrate this in the existing filechooserwidget machinery is to add a OPERATION_MODE_OTHER_LOCATION. Look at how "Enter Location" was integrated for some ideas.
Created attachment 307350 [details] [review] filechooserwidget: use places view to manage fixed devices Previous patch modified places sidebar widget to stop handling fixed devices by adding an "Other Locations..." item. Up to now, however, these changes are isolated from each other since the bundled file manager widgets ignore the sidebar requests for external management of fixed devices and networks. To fix that, make the file chooser widget be aware of the GtkPlacesSidebar::show-other-locations signal and, when requested, show places view to manage the fixed devices and networks.
Created attachment 307351 [details] [review] placessidebar: add Other Locations item Better handle networks and removable devices.
Created attachment 307352 [details] [review] placesview: add view for fixed drives and networks placesview: add view for fixed drives and networks Improve the current code to monitor the server file, update pointer to reflect busy status, fix keyboard navigation, add eject button to rows as well as the context menu (which is now connected to 'popup-menu' signal), pulse address entry while connecting.
Much improved, thanks! I see a few remaining things, behaviour-wise: - The unmount buttons seem to be hard (impossible?) to activate with the keyboard. - The sidebar does not scroll to keep the selected row in view during keynav (not your fault) - The 'recent servers' button does not react to space or enter when it has focus, seems not keyboard activatable - Same goes for the Connect button - not keyboard activatable - Missing mnemonics in 'Connect to _Server' and 'Co_nnect' - We should disable search on the places view or make it work
Running testfilechooser in rtl, you'll notice that the "Network" label is missing a margin-end setting of 12 (the "This Computer" label has it).
Here's a patch that fixed the keyboard activation of the recent server and connect buttons: diff --git a/gtk/gtkplacesview.c b/gtk/gtkplacesview.c index 4f83ac4..58af864 100644 --- a/gtk/gtkplacesview.c +++ b/gtk/gtkplacesview.c @@ -393,7 +393,7 @@ on_key_press_event (GtkWidget *widget, focus_widget = gtk_window_get_focus (toplevel); - if (!focus_widget) + if (!GTK_IS_PLACES_VIEW_ROW (focus_widget)) return FALSE; if ((event->state & modifiers) == GDK_SHIFT_MASK) @@ -401,9 +401,7 @@ on_key_press_event (GtkWidget *widget, else if ((event->state & modifiers) == GDK_CONTROL_MASK) priv->current_open_flags = GTK_PLACES_OPEN_NEW_WINDOW; - if (GTK_IS_PLACES_VIEW_ROW (focus_widget)) - activate_row (view, GTK_PLACES_VIEW_ROW (focus_widget), priv->current_open_flags); - + activate_row (view, GTK_PLACES_VIEW_ROW (focus_widget), priv->current_open_flags); return TRUE; } }
Created attachment 307422 [details] [review] placesview: add view for fixed drives and networks placesview: add view for fixed drives and networks Implemented search, added empty state, fixed focus chain and added mnemonics.
Created attachment 307423 [details] [review] placessidebar: add Other Locations item placessidebar: add Other Locations item Updated
Created attachment 307424 [details] [review] filechooserwidget: use places view to manage fixed devices filechooserwidget: use places view to manage fixed devices Improved handling of search for other locations view
The mnemonic in "Connect to Server" doesn't work, you need to add <property name="mnemonic_widget">address_entry</property> I tried to figure out how to test empty state, it turns out to be really hard to make a goa volume go away. Not your bug, but there's something wrong there... I figured that testfilechooser --local-only gives me an empty state (I have no partitions). But this state is just that: empty. There's nothing there in this case - I think there should be. More concerning: even with --local-only, search brings back that dang goa volume. I don't think that should happen. Either it is a network volume, or it isn't. In the end, I figured I can force-disable the goa volume by mutilating /usr/share/gvfs/remote-volume-monitors/goa.monitor. If I do that, I _do_ get the proper empty state (which does look nice). So, somehow the --local-only case gets confused about whether it is actually empty or not.
Created attachment 307476 [details] [review] placesview: add view for fixed drives and networks Fixed both the wrong mnemonic widget and the strange behavior on --local-only
owncloud account no longer shows up on search with local-only, thats good. Mnemonics work, good too. Still get an "empty empty" view with local-only and no partitions, unlike the proper empty state I get when I disable goa and run without local-only. And here's another small issue: the 'no search results' page sticks around after I hide the search entry. Thats different from the behavour when I search in some other location: when I hide the search entry, I get back the normal view for that location. See bug 752432 for the goa fix, btw.
Created attachment 307506 [details] [review] placesview: add view for fixed drives and networks Now really fixed the issue with --local-only, and the sticky empty search view.
Created attachment 307508 [details] [review] filechooserwidget: use places view to manage fixed devices Update with master.
Created attachment 307509 [details] [review] placesview: add view for fixed drives and networks Fixed a very annoying visual glitch when updating the list of devices.
I still get "empty empty" here. I tracked this down to the list being populated before local-only gets set. And the local_only setter does not update the mode to reflect the new state of affairs. I fixed it locally by calling update_places() from there. Not the most elegant in terms of doing unnecessary work, but it fixes the issue.
Created attachment 307516 [details] [review] placesview: add view for fixed drives and networks Hopefully the issue is truly fixed. Thanks for the many reviews.
Review of attachment 307516 [details] [review]: Great work, really not much to complain here. ::: gtk/gtkplacesview.c @@ +487,3 @@ + return; + + Unnecessary extra line here @@ +651,3 @@ + for (l = children; l; l = l->next) + { + if (gtk_widget_get_child_visible (l->data)) I find these uses of child_visible slightly odd here - child_visible is an implementation detail of some containers, and you never set it - how would a child of one of these lists ever be !child_visible ? @@ +767,3 @@ + "volume", volume, + "mount", mount, + NULL); Indentation slightly off here @@ +810,3 @@ + "volume", NULL, + "mount", mount, + NULL); Indentation slightly off again @@ +904,3 @@ + /* + * Now that all necessary drives and volumes were already added, add mounts + * that has no volume, such as /etc/mtab mounts, ftp, sftp, etc. s/has/have/ @@ +1001,3 @@ + { + emit_open_location (view, location, priv->open_flags); + } Loose the { } here @@ +1063,3 @@ + { + emit_open_location (GTK_PLACES_VIEW (user_data), root, priv->open_flags); + } Loose the { } here @@ +1103,3 @@ +} + + Unnecessary extra linebreak here. @@ +1142,3 @@ + + g_cancellable_cancel (priv->cancellable); + priv->cancellable = g_cancellable_new (); Looks like we're leaking a cancellable here ? @@ +1167,3 @@ + + g_cancellable_cancel (priv->cancellable); + priv->cancellable = g_cancellable_new (); And here @@ +1202,3 @@ + + g_cancellable_cancel (priv->cancellable); + priv->cancellable = g_cancellable_new (); And here @@ +1652,3 @@ + + if (name) + retval |= g_strstr_len (name, -1, priv->search_query) != NULL; why use a len-variant if you then pass -1 ? @@ +1738,3 @@ + * GtkPlacesView::open-location: + * @view: the object which received the signal. + * @location: (type Gio.File): #GFile to which the caller should switch. I think this misses an @flags argument ::: gtk/ui/gtkplacesview.ui @@ +22,3 @@ + <property name="visible">True</property> + <child> + <object class="GtkBox" id="empty_servers_box"> In ui files, I nowadays try to only asssign ids to objects that I'm actually accessing from the code, tends to make things look much cleaner because you don't end up with box21 or label16 @@ +34,3 @@ + <property name="can_focus">False</property> + <property name="pixel-size">48</property> + <property name="icon_name">network-server-symbolic</property> One thing that you could do in a separate patch is to make sure that all the icon names you are using in this work are represented in testsuite/gtk/check-icon-names.c. That test is supposed to check that we have all expected icons in the theme. But that is really for extra credit @@ +49,3 @@ + <property name="visible">True</property> + <property name="can_focus">False</property> + <property name="label" translatable="yes">No recent servers found</property> I think the strings that are talking about servers could do with some translator comments, otherwise the results might be mixed... @@ +224,3 @@ + <property name="margin_bottom">6</property> + <property name="hexpand">False</property> + <property name="label" translatable="yes">Network</property> Same here.
Review of attachment 307508 [details] [review]: Looks good
Review of attachment 307423 [details] [review]: Looks good
Created attachment 307523 [details] [review] placesview: add view for fixed drives and networks Fixed.
Review of attachment 307523 [details] [review]: ::: gtk/gtkplacesview.c @@ +1145,3 @@ + 0, + operation, + NULL, Now I wonder: shouldn't you actually pass that cancellable here ?
Review of attachment 307423 [details] [review]: Commited. Commit 7db399d97.
Review of attachment 307523 [details] [review]: Commited. Commit 7af88d40.
Review of attachment 307508 [details] [review]: Commited. Commit 79f2400c.
Marking this bug as RESOLVED FIXED.