GNOME Bugzilla – Bug 546189
Volumes in 'Places' sidebar should have a "Properties" item in context menu.
Last modified: 2011-10-18 15:46:25 UTC
Forwarded from: https://bugs.launchpad.net/ubuntu/+source/nautilus/+bug/230098 I find it pretty confusing that the context menu of (removable or non-removable) media is so short, as compared to what you get when right-clicking the same drive after a click on the 'Computer' toolbar icon. Especially from removable media (USB-Sticks, SD-Cards...) I want to right-click the places bar to select "Properties", in order to learn about the remaining disk space on the drive, which is very useful when drag-dropping files and folders into a drive listed in the places side bar. Of course, I don't want to navigate away from my current folder (because I might have files selected, or the view scrolled down by a specific amount...), neither would I want to open a new window (from the side-bar list's drive's context menu), then right-click into this new window again and select 'Properties' from there... For me, this is mostly about disk-space, but I'm sure there is a lot more cases when people would need a not-so-crippled context menu in the places list. Using screenshots for clarification: This context menu: http://launchpadlibrarian.net/14818633/Bildschirmfoto-Rechner%20-%20Datei-Browser-1.png should be the same as - or more similar to - that one: http://launchpadlibrarian.net/14818628/Bildschirmfoto-Rechner%20-%20Datei-Browser.png .
I agree with you on "Properties", but that menu *definitely* shouldn't have all the other items. Changing summary.
Having "Properties" in the menu would at least be better than nothing, but what's the reasoning for not having the rest in there as well? Wouldn't it make work easier? What are the cons?
> what's the reasoning for not having the rest in there as well? I agree that the inconsistency between computer:/// and the side pane is unfortunate, but we also have to consider UI clutter, and the way people use the side pane UI elements. For instance, I wouldn't right-click a volume in the side pane for opening its root location in an application. I also think that it doesn't make sense to offer the properties dialog unless there is something really useful [besides the disk space calculation]. That said, most of the property dialog contents of volume and drive items in computer:/// is useless as well. Regarding your original issue: Maybe the remaining disk space should be displayed in a tool tip?
Good points, there. > Maybe the remaining disk space should be > displayed in a tool tip? For my case, that should be enough, yes. Actually, spontaneously, I can't think of any case where I'd desperately need any of the other menu entries. If someone else knows one, should a new bug be opened? Of course, rather than a tooltip I'd like to see a visual representation like a small bar on the right to the volume name, but of course I know there is no such thing as a developer with too much free time.
> Of course, rather than a tooltip I'd like to see a visual representation like a small bar on the right to the volume name For N volumes, this would mean that we have N new visual elements (i.e. strings) displayed all the time, in addition to the recently added eject button. I'm a bit curious why it is importat to see the free disk space all the time. What storage media are you using? Note that instead of a tool tip, we could also display volume information in the status when hovering over the side bar, which may fit better into your work flow.
> I'm a bit curious why it is importat to see the > free disk space all the time. You're right - it is not. It'd be rather cluttering, actually. > What storage media are you using? Nothing special. This is mostly about external hard drives for backups and USB sticks for any kind of use. I think, my problem here was that I was thinking from a "bling" point of view. > Note that instead of a tool tip, we could also display volume > information in the status when hovering over the side bar Would that tooltip still be instant even though it's displaying more information? I am asking because the properties dialog can take quite some time to load, and I can't tell which element of it causes that. So if such a tooltip could appear within a few ms, this might be a good way to present useful information faster than with an extra dialog and not clutter the interface the same time. For extra information, I'm thinking about e.g. the mount point and the /dev name - which is not even shown in the properties dialog. (Neither in the Drive, nor in the Volume tab.)
I just wanted to open a bug to add 'properties' to the places pane, because of a few other concerns. 1. i don't see any reason why the properties should be available for the tree pane but not for the place pane, as most users work with either one, but don't switch. 2. the properties dialog accessed via the tree pane is different than the one accessed from the desktop context menu, what reasoning is behind that? the desktop one lets you set mount options, the other one does not. i used to have the gconf option show_desktop=0 and was never aware that you can actually change the mount point.
Tentative patch to add a properties item to the context menu attached. I am not an expert of all the ins and outs of nautilus internals, so there may be something totally wrong with the patch. Comments are warmly welcome.
Created attachment 138277 [details] [review] Tentative patch.
Created attachment 138278 [details] [review] Tentative patch.
*** Bug 513838 has been marked as a duplicate of this bug. ***
Comment on attachment 138278 [details] [review] Tentative patch. Hi Stefano, first of all sorry for the late review. Could you please build your patches with `git diff` or `git format-patch`? It makes them much easier to read and apply. As for the patch contents, the basic idea looks good, but unfortunately the current Nautilus architecture would not allow clients outside src/file-manager to access methods in that directory (such as fm_properties_window_present()). It would probably be a good thing to move the properties dialogue outside of src/file-manager/, but that would require a bit more work. Also, some of the places sidebar code has been recently rewritten, so your patch might need updating too.
Created attachment 181013 [details] [review] same patch updated to current git master Cosimo: don't worry, I had completely forgotten about that patch anyway. :-) Here's the very same patch updated for git master. Possible variants: - add a separator just before the properties menu item - show the properties menu item only for bookmarks and volumes
Created attachment 181454 [details] [review] updated patch, adds a separator menu item I went ahead and added the separator The properties entry is shown unconditionally.
Comment on attachment 181454 [details] [review] updated patch, adds a separator menu item Thanks for the patch, looks almost good; I think though you should disable the item at least for 'Browse Network'.
Created attachment 181512 [details] [review] patch Ok, properties entry is not shown for the Browse network item.
(In reply to comment #15) > (From update of attachment 181454 [details] [review]) > Thanks for the patch, looks almost good; I think though you should disable the > item at least for 'Browse Network'. Stefano, with "at least" here I meant this report originally asked for the context menu item on volume entries of the sidebar. It kind of makes sense to me, now that we don't have the desktop (and thus, 'Computer'), but I wonder it makes sense to show the item for all folders. I'd rather wait for a designer input before moving on, although your patch looks fine code-wise.
A properties item seems like a good addition to the context menu. Wanting to find out remaining disk space is a common enough occurrence, and it shouldn't have any negative consequences.
(In reply to comment #18) > A properties item seems like a good addition to the context menu. Wanting to > find out remaining disk space is a common enough occurrence, and it shouldn't > have any negative consequences. Properties should be just available for the volumes: the other items are bookmarks of varying descriptions, whereas the volume items have a more literal relationship with the object that they represent. Plus, the other items can be accessed relatively easily.
Created attachment 182001 [details] [review] udpated patch Okay, patch updated to only show the properties entry for local mounts only, since the properties window only shows volume usage for them. Hopefully I got it right :-)
Any update on this? :-)
Review of attachment 182001 [details] [review]: I inlined some comments below. ::: src/nautilus-places-sidebar.c @@ +1693,3 @@ + show_properties = (g_mount_can_unmount (mount) || + g_mount_can_eject (mount)) && + nautilus_directory_is_local (directory); I don't really get the rationale of this snippet. Why are you checking for can_unmount and can_eject? If it's to avoid adding the item to unmounted volumes, I think the (mount != NULL) already covers that, as a GMount is a mounted volume by definition (you would only have a GVolume otherwise). Also, I think we should show properties for every mount, not only for local ones. @@ +2465,3 @@ + gtk_tree_view_get_cursor (sidebar->tree_view, &path, NULL); + + if (!gtk_tree_model_get_iter (model, &iter, path)) { I think you're leaking |path| in this case here. You should also set path = NULL before passing it to gtk_tree_view_get_cursor() and return early before calling gtk_tree_model_get_iter() if the path was not set (this might not happen in reality because we are usually here after a key/button event callback, but it's better to be safe). @@ +2651,3 @@ + /* Properties menu item */ + Please remove this empty line here. @@ +2655,3 @@ + GTK_WIDGET (eel_gtk_menu_append_separator (GTK_MENU (sidebar->popup_menu))); + + item = gtk_image_menu_item_new_from_stock (GTK_STOCK_PROPERTIES, NULL); Does this force an icon in the menu item? We shouldn't display an icon there. @@ +2658,3 @@ + sidebar->popup_menu_properties_item = item; + g_signal_connect (item, "activate", + G_CALLBACK (properties_cb), sidebar); Wrong indentation?
(In reply to comment #22) > Review of attachment 182001 [details] [review]: > > I inlined some comments below. Thanks for the lightning fast reply, Cosimo. > ::: src/nautilus-places-sidebar.c > @@ +1693,3 @@ > + show_properties = (g_mount_can_unmount (mount) || > + g_mount_can_eject (mount)) && > + nautilus_directory_is_local (directory); > > I don't really get the rationale of this snippet. Why are you checking for > can_unmount and can_eject? If it's to avoid adding the item to unmounted > volumes, I think the (mount != NULL) already covers that, as a GMount is a > mounted volume by definition (you would only have a GVolume otherwise). I'm not really well learned on the actual taxonomy of volumes, mounts, and drives, and how to determine what is what depending on whether a mount/volume is ejectable/unmountable/local/etc. Do you have any reference handy? > Also, I think we should show properties for every mount, not only for local > ones. The rationale behind the additional properties menu entry was to enable the user to check the free space of a mount -- and it doesn't work for remote mounts in nautilus? Unless I'm confusing them with remote mounts... > @@ +2465,3 @@ > + gtk_tree_view_get_cursor (sidebar->tree_view, &path, NULL); > + > + if (!gtk_tree_model_get_iter (model, &iter, path)) { > > I think you're leaking |path| in this case here. Fixed. > You should also set path = NULL before passing it to gtk_tree_view_get_cursor() > and return early before calling gtk_tree_model_get_iter() if the path was not > set (this might not happen in reality because we are usually here after a > key/button event callback, but it's better to be safe). Fixed as well. > @@ +2651,3 @@ > > + /* Properties menu item */ > + > > Please remove this empty line here. I left the empty line here, it's in line with other comments in the same function. Those can be cleaned up (if needed) with a follow-up patch. > @@ +2655,3 @@ > + GTK_WIDGET (eel_gtk_menu_append_separator (GTK_MENU > (sidebar->popup_menu))); > + > + item = gtk_image_menu_item_new_from_stock (GTK_STOCK_PROPERTIES, NULL); > > Does this force an icon in the menu item? We shouldn't display an icon there. Indeed, fixed. > @@ +2658,3 @@ > + sidebar->popup_menu_properties_item = item; > + g_signal_connect (item, "activate", > + G_CALLBACK (properties_cb), sidebar); > > Wrong indentation? Fixed. Please let me know if there's anything else that needs fixing. Thanks!
> > Also, I think we should show properties for every mount, not only for local > > ones. > > The rationale behind the additional properties menu entry was to enable the > user to check the free space of a mount -- and it doesn't work for remote > mounts in nautilus? Unless I'm confusing them with remote mounts... I'm unable to test it here (because my 3G connection is so slow that the properties window call times out), but this enables the properties menu item for ssh mounts. Does it make sense? Anyway, patch attached.
Created attachment 186752 [details] [review] updated patch
Review of attachment 186752 [details] [review]: Looks good to commit after we branch for 3.2.
I pushed this to master now that we branched for gnome-3-2.