GNOME Bugzilla – Bug 762874
Drives and locations list: improve sort and layout
Last modified: 2021-05-26 09:26:38 UTC
Created attachment 322664 [details] screenshot There are various issues with the drives and locations list. The result is that it is hard to understand and doesn't guide the user. It could look better too. A screenshot is attached. Issues I can see here: * Cloud (Google and ownCloud) accounts are shown twice. * Home is almost always going to be the location the user is interested in - why isn't it at the top? * Showing / is unhelpful: users can't do anything with it unless they have a lot of technical knowledge. Options here: - Don't show / - Show it in the list but don't allow it to be opened (make it insensitive) - Show it and allow it to be opened, but only show the parts that are relevant to the user and which can be safely removed (cache and so on) * Doesn't show the amount of space on the home partition, or allow the whole partition to be scanned. * No clear indication that unmounted volumes aren't mounted. * Isn't clear which locations are local devices and which are remote. * "Devices and locations" should use title case: "Devices & Locations". * The size bars are all different lengths. This looks messy, and it's not clear whether the length is significant. * The list doesn't look very good when it is very wide - everything is spread out to the edges. Apologies for mentioning so many issues in one report. I'd be happy to file separate bugs where appropriate.
Created attachment 322665 [details] mockup Attaching a mockup for an improved list.
Created attachment 325056 [details] [review] wireframe implementation I've made a first pass at this. I took a stab at the problem of duplicated cloud drives, but the problem appears to be firmly rooted in GIO: I tried rewriting the already_exists function in LocationList checking against a variety of different properties, but everything on the remote GMount returns null where ideally get_volume() should return the volume we've already built a LocationListRow for. I've also avoided trying to correct the icons displayed for cloud volumes for the same reason, really this is something that should be handled properly in GIO. This is a bit of a first for me, so please be gentle :)
Created attachment 325059 [details] [review] patch fixup This is a cleaner implementation, and fixes two things I missed in the last patch: * The remote locations frame is no longer displayed when there are no remote volumes * When remote volumes are added (eg when the 'Files' switch in the Google account section of gnome-settings is enabled) the volume will only appear in the remote volumes list
Created attachment 325062 [details] [review] patch mk. III I was experiencing very odd behaviour on device mount/unmount before, and while I was trying to sound out all the relevant GVolumeMonitor quirks I found out that creating two instances of a Location object causes troubles. So this patch splits the management of the locations GList into a single object that is shared between the LocalLocationsList and RemoteLocationsList instances. It also fixes any remote volume duplication oddities I was able to encounter (hooray!)
Looks great to me! Some things I noticed with the patch: * I'd suggest adding 6px margin above and below the level bars - they're a bit cramped at the moment. * It would be good to make the list rows a bit narrower - the gap between the information on either side is a bit big, making the list hard to read. * Trying it here, I see my ownCloud account repeated twice - once in the "This Computer" section, and once in "Remote Locations". * I wonder if the home directory should be pinned to the top of the list?
Could you upload a screenshot? I'm not sure what to make of the ownCloud repetition.
Created attachment 325109 [details] screenshot of the latest patch Here you go.
Created attachment 325114 [details] [review] patch - take four I've moved some styling bits around as per your comment, please let me know if there's anything further that needs touching up. I've been playing with this for a couple of hours and the only way I can get remote mounts to not appear duplicated on application startup is to just not enumerate the mounts when the application begins. This is unfortunately a bug, however any mounts that don't get enumerated on startup will happily be discovered on remount thanks to some tweaks in the GVolumeMonitor signal handler. Another thing new in this patch is now the fetching of mount data is asynchronous, which helps with ownCloud since the GMount doesn't always contain all the information we want when the mount signal is fired. I've also added a bit of a dirty hack which helps to identify some GMounts as remote: some mounts return null for almost every attribute, but I've found that remote mounts that do so almost always have the GThemedIcon "folder-remote". So in cases where we don't have a volume, fill_from_mount() will examine the icon and set is_remote accordingly.
Created attachment 325117 [details] [review] patch 5 This is just fixing stuff I broke with the last patch :P I again have spent a couple of hours messing around with stuff to try and make volume/mount detection and location processing more robust, but the combination of signals firing out of order and mounts not being parented to their volumes mean anything that can be done on the baobab side is either going to be racy or extremely slow. I ended up with a couple of async functions that polled the volumes for information on a timeout so that LocationList could have all volume data up-to-date when mount events came in. But it ended up taking 5-10 seconds for baobab to show all the devices because there is no way to know whether a GVolume returns null for get_mount() because the field isn't populated or because it isn't mounted, nor can you know how long it will take for the field to be maybe not null. Particularly with ownCloud, the time it took was quite variable. The current patch mixes a bit of both: I've used GLib.Timeout to poll state in a couple of places and removing the mount enumeration from startup has improved stuff a lot, but it still remains quite racy. I've done a lot of unmounting/remounting of my test Google drive and ownCloud instance over the past few days, and off the top of my head I guess maybe one in thirty mounts I ended up with a dupe. So it's an improvement, but not a perfect one and not without cost. Whilst writing this I noticed that Nautilus seems to handle it fine, so it might be worth investigating what they're doing. Also, is seems as though GLib.File.equal() doesn't always return true when dealing with remote mounts, so I changed it to a string equality test comparing paths.
Created attachment 325179 [details] [review] last one (hopefully) Okay, foot firmly in mouth: whatever ramblings in the above walls of text about this part of GIO being broken was written in complete ignorance of shadow mounts and the get_activation_root() method of GVolume. These two API features are how Nautilus nails this perfectly, and with this patch baobab does too! I've reinstated the initial mount enumeration, the crazy mount_added() buffering is gone, and I've added some additional bits so that GMounts which really /are/ broken still get parented to an appropriate Location. So now this patch (finally) implements Allan Day's complete wireframe, plus fixes some relevant blocker bugs and contains no regressions.
Review of attachment 325179 [details] [review]: Hi George, thanks for your work! This patch is mixing UI rearrangement with several other bugfixes. Could you please split it in such a way that each patch addresses a single issue? At least one patch for UI changes and one for the bugs.
Sure! Two questions though: * Should subsidiary patches be logged against the relevant tickets, or are they okay here? * Is it appropriate for one patch to depend on others? Thanks
(In reply to bob from comment #12) > Sure! Two questions though: > > * Should subsidiary patches be logged against the relevant tickets, or are > they okay here? > * Is it appropriate for one patch to depend on others? > * If there's already a bug open, you can attach the patch there, otherwise here is fine. * Of course, but in an incremental way. Roughly, patches shouldn't break the build.
Created attachment 325195 [details] [review] 0001-Bug-fix-volume-duplication.patch (1 of 3) (sorry, I thought there were other open tickets that pertained to bugs that got fixed here, but I couldn't find them again. So I'm just dumping the patches here) This implements shadow mount and activation root testing to try and remove any double-up volumes/mounts
Created attachment 325196 [details] [review] 0002-Bug-fix-asynchronously-poll-mounts-for-data.patch (2 of 3) This patch polls new mounts for missing data until we've got it or 10 seconds are up
Created attachment 325197 [details] [review] 0003-Rejig-devices-and-locations-UI.patch (3 of 3) And this patch actually implements the new design
Review of attachment 325195 [details] [review]: Thanks for looking into the duplication issue, my comments below. I'm asking a lot questions but that's not intended to be harsh, it's just that the code is already a bit spaghetti so I'd rather be careful before introducing workarounds :) ::: src/baobab-location-list.vala @@ +103,3 @@ bool already_present (File file) { foreach (var l in locations) { + if (l.file != null && l.file.get_parse_name () == file.get_parse_name ()) { Can you provide an example for this change? Are we sure it's not a bug in Gio? @@ +154,3 @@ + location.update (); + } else { + locations.remove (location); What does it mean that remote filesystems fire mount signals? Anyway, when a volume is unmounted, the volume_change signal is also fire, and in the callback we take care of updating the location, so why do we need to do it here again? @@ +198,3 @@ var location = new Location.from_mount (mount); if (!location.is_home) { + mount_added (mount); mount_added() checks if volume==null. The same check is also here. Moreover, mount_added() calls update() each time and that's in contrast with the logic used in populate(). So this need to be refactored a bit. ::: src/baobab-location.vala @@ -32,3 @@ public File? file { get; private set; } public FileInfo? info { get; private set; } - public bool is_volume { get; private set; default = true; } Here and in the following you remove the boolean property logic. The property is used to decide if we need to show usage bars. Basically, you are saying that checking volume != null is enough. But this misses at least the Main Volume case. It's true that we could handle it separately (as you do in the third patch), but are we sure that the Main Volume is the only "Volume" which has volume == null ? @@ +173,3 @@ name = volume.get_name (); icon = volume.get_icon (); + file = volume.get_activation_root (); This is strange. My understanding is that the activation root is the path that will be used once the volume is mounted, but in that case there must be a GMount associated with the volume. Nautilus uses get_activation_root() somewhere, but only for opening a location, never for deduplicating.
>Can you provide an example for this change? Are we sure it's not a bug in Gio? I noticed the File.equal comparison fails with Google drive and ownCloud. Now that I think about it, it might be that the comparison fails because one is an activation root and the other is an actual mount. Either way, the patch doesn't work without that line. You can test this for yourself, add in a message() call to print out the two parse names and you can see that two parse names can be equal without File.equal returning true. >What does it mean that remote filesystems fire mount signals? >Anyway, when a volume is unmounted, the volume_change signal is also fire, and in the callback we take care of updating the location, so why do we need to do it here again? That's the problem, the remote filesystems I was testing with (again, Google drive and ownCloud) don't fire the volume_change signal. >Moreover, mount_added() calls update() each time and that's in contrast with the logic used in populate(). So this need to be refactored a bit. I'm afraid I don't understand what you mean. >but are we sure that the Main Volume is the only "Volume" which has volume == null ? Looking through the code, I can't imagine any other case where that would be true. Then again, that isn't saying much :) I've been thinking about this too. What I really wanted to do is try and make Location a little bit less spaghetti, but my concern is that either my idea of "not spaghetti" is exactly your idea of spaghetti or that the changes would be altogether too superfluous. >My understanding is that the activation root is the path that will be used once the volume is mounted, but in that case there must be a GMount associated with the volume. I have no idea what is meant to happen, but for sure GVolumes have activation roots and then don't have the GMount associated with them. You can test this for yourself, remove that line and throw in a couple of message calls around mount_added() and you'll see what I mean. Again, without this line the patch no worko Thanks
Looks really good! The duplication issue is gone here now. The list is perhaps on the narrow side with the latest patches - maybe go for around 700px width?
Created attachment 325375 [details] [review] 0003-Rejig-devices-and-locations-UI.patch (w/ 700px wide list) Here you go!
Hey, I was wondering if there was any progress re review? Let me know if there's anything else that needs touching up. Thanks
Hello! It's been a while now and I’m still willing to work on this. When could we pick this back up?
(In reply to bob from comment #22) > Hello! It's been a while now and I’m still willing to work on this. When > could we pick this back up? Hi! I think that the duplication issue is not entirely baobab's fault, there's at least one bug in gvfs (see bug 764934) which should be solved first. So, it would be great if you could prepare a patch on top of master, just implementing the new design. In this way we could commit it soon during this cycle. Later we'll deal with other bugs such as duplications etc...
Created attachment 332286 [details] [review] 0001-Rejig-devices-and-locations-UI.patch (based on master) Roger, let me know if this is more what you had in mind. It's worth noting, however, that because of this missing bug fix patches the scope of the design implementation patch has shrunk a little: the first and second patches also implemented the "is this Location mounted?" logic, so this has been removed from this patch as well (I don't know why that wasn't made more clear in the commit messages, sorry about that).
GNOME is going to shut down bugzilla.gnome.org in favor of gitlab.gnome.org. As part of that, we are mass-closing older open tickets in bugzilla.gnome.org which have not seen updates for a longer time (resources are unfortunately quite limited so not every ticket can get handled). If you can still reproduce the situation described in this ticket in a recent and supported software version of Baobab, then please follow https://wiki.gnome.org/GettingInTouch/BugReportingGuidelines and create a new enhancement request ticket at https://gitlab.gnome.org/GNOME/baobab/-/issues/ Thank you for your understanding and your help.