GNOME Bugzilla – Bug 661711
Sorting keys for GDrive, GVolume and GMount instances
Last modified: 2012-09-07 08:06:03 UTC
Most GIO applications displaying GDrive, GVolume and GMount instances rely on the order of objects as returned by the get_connected_drives(), get_volumes() and get_mounts() methods on GVolumeMonitor. This by itself is fine - the GVolumeMonitor implementation is just supposed to sort the list before returning it to the user. This, however, makes it difficult for implementations where you only call these methods at startup and instead want to rely on ::drive-connected, ::drive-disconnected, ::volume-added etc. signals to maintain a data structure with the objects. You don't know where to insert the added GDrive or GVolume object because you have no way to know the ordering. One specific example of this is the GVfs proxy volume monitor implementation where, for performance reasons, the client (e.g. each GIO-using process) does a single GetAll() D-Bus call and then just listen to D-Bus methods. The problem with this is that the order of the objects returned by the proxy volume monitor is arbitrary. I will attach a patch that introduces get_sort_key() methods on GDrive, GVolume and GMount objects. The default implementation will just return NULL. It is tempting to introduce compare() methods too but I don't actually need that much ... and since introducing compare() methods puts us in a difficult territory (what to do with objects from different backends?), I'd rather not do this. With this in place, I will attach a patch that uses this in the proxy volume monitor by expanding the protocol to include the sort-key over D-Bus. The patch will also modify the proxy volume monitor's implementation of get_connected_drives(), get_volumes(), get_mounts() to sort the entries using the value from the new get_sort_key() method. In the case where the proxied GVolumeMonitor implementation does not support any sort key (e.g. get_sort_key() returns NULL), this will not make any difference since g_list_sort() is a stable sort (see bug 531973). All in all, these two patches will solve the problem where the order of drives, volumes and mounts from a single backend appears in random order. In the future, we could - define a namespace for sorting keys and make all backends implement it (for example, we could say that 10_name_* is for e.g. SMB/NFS shares and 20_detection_time_<nanosecs_since_epoch> is for devices you can plug and unplug) - make applications (e.g. nautilus sidebar) sort all the returned objects using get_sort_key() but I would like to just get get_sort_key() in first. Thanks for considering.
Created attachment 198977 [details] [review] GIO patch
Created attachment 198978 [details] [review] GVfs patch
Note: with these patches applied, nothing will actually change - things will only change once a volume monitor starts implementing get_sort_keys() for its returned objects. One such volume monitor is the upcoming udisks2 volume monitor that I'm working on over in the wip/udisks2 branch. I don't have any plans to change the gdu or hal volume monitors.
Created attachment 198980 [details] [review] Updated GVfs patch Ooops, forgot to sort the volumes returned by g_drive_get_volumes(). This updated patch does that.
I just committed code to use this on the wip/udisks2 branch, see http://git.gnome.org/browse/gvfs/commit/?h=wip/udisks2&id=3347b1f9a4aedb8235542b7b4ccc5306dc8b6650 It obviously depends on the two patches in comment 1 and comment 4. It works as expected insofar that newly inserted media/drives appear at the bottom. The relevant gvfs-mount -oi output is in [1] and a screenshot is here: http://people.freedesktop.org/~david/gvfs-with-sort-keys.png [1] : Drive(0): Floppy Drive Type: GProxyDrive (GProxyVolumeMonitorUDisks2) ids: unix-device: '/dev/fd0' themed icons: [drive-removable-media-floppy] [drive-removable-media] [drive-removable] [drive] is_media_removable=1 has_media=1 is_media_check_automatic=1 can_poll_for_media=0 can_eject=0 can_start=0 can_stop=0 start_stop_type=unknown sort_key=gvfs.time_detected_usec.1318518015525416 Volume(0): Floppy Disk Type: GProxyVolume (GProxyVolumeMonitorUDisks2) ids: unix-device: '/dev/fd0' themed icons: [media-floppy] [media] can_mount=0 can_eject=0 should_automount=0 sort_key=gvfs.time_detected_usec.1318613103789389 Drive(1): 2.0 TB Hard Disk Type: GProxyDrive (GProxyVolumeMonitorUDisks2) ids: unix-device: '/dev/sdc' themed icons: [drive-harddisk-usb] [drive-harddisk] [drive] is_media_removable=0 has_media=1 is_media_check_automatic=1 can_poll_for_media=0 can_eject=0 can_start=0 can_stop=0 start_stop_type=unknown sort_key=gvfs.time_detected_usec.1318613845541318 Volume(0): blah1 Type: GProxyVolume (GProxyVolumeMonitorUDisks2) ids: unix-device: '/dev/sdc1' uuid: 'B029-303C' label: 'blah1' themed icons: [drive-harddisk-usb] [drive-harddisk] [drive] can_mount=0 can_eject=0 should_automount=0 sort_key=gvfs.time_detected_usec.1318613845917542 Drive(2): CD Drive Type: GProxyDrive (GProxyVolumeMonitorUDisks2) ids: unix-device: '/dev/sr1' themed icons: [drive-optical] [drive] is_media_removable=1 has_media=1 is_media_check_automatic=1 can_poll_for_media=0 can_eject=1 can_start=0 can_stop=0 start_stop_type=unknown sort_key=gvfs.time_detected_usec.1318613874198455 Drive(3): 8.2 GB Drive Type: GProxyDrive (GProxyVolumeMonitorUDisks2) ids: unix-device: '/dev/sdb' themed icons: [drive-removable-media-usb] [drive-removable-media] [drive-removable] [drive] is_media_removable=1 has_media=1 is_media_check_automatic=1 can_poll_for_media=0 can_eject=1 can_start=0 can_stop=0 start_stop_type=unknown sort_key=gvfs.time_detected_usec.1318613874264146 Volume(0): 8.2 GB Encrypted Type: GProxyVolume (GProxyVolumeMonitorUDisks2) ids: unix-device: '/dev/sdb1' uuid: 'edb970d9-ae0d-45ef-9485-0de2dc7eeddc' can_mount=0 can_eject=1 should_automount=0 sort_key=gvfs.time_detected_usec.1318613874304854 Drive(4): CD/DVD Drive Type: GProxyDrive (GProxyVolumeMonitorUDisks2) ids: unix-device: '/dev/sr0' themed icons: [drive-optical] [drive] is_media_removable=1 has_media=1 is_media_check_automatic=1 can_poll_for_media=0 can_eject=1 can_start=0 can_stop=0 start_stop_type=unknown sort_key=gvfs.time_detected_usec.1318613895909240 Volume(0): LSI HBA Drivers Type: GProxyVolume (GProxyVolumeMonitorUDisks2) ids: unix-device: '/dev/sr0' label: 'LSI HBA Drivers' themed icons: [media-optical-cd-rom] [media-optical-cd] [media-optical] [media] can_mount=0 can_eject=1 should_automount=0 sort_key=gvfs.time_detected_usec.1318613896013217
Review of attachment 198977 [details] [review]: Looks good to me.
Review of attachment 198980 [details] [review]: Looks good to me. ::: monitor/proxy/gproxydrive.c @@ -136,2 @@ */ -#define DRIVE_STRUCT_TYPE "(sssbbbbbbbbuasa{ss})" Why is this defined here, as well as in gvfsproxyvolumemonitordaemon.c? It doesn't seem used?
Comment on attachment 198977 [details] [review] GIO patch Thanks for the review. Committed on master: http://git.gnome.org/browse/glib/commit/?id=915e2238c478737def2f8919204ee10d06ecb98a
(In reply to comment #8) > Review of attachment 198980 [details] [review]: > > Looks good to me. > > ::: monitor/proxy/gproxydrive.c > @@ -136,2 @@ > */ > -#define DRIVE_STRUCT_TYPE "(sssbbbbbbbbuasa{ss})" > > Why is this defined here, as well as in gvfsproxyvolumemonitordaemon.c? > It doesn't seem used? It's not currently used because of the way things are unpacked using libdbus (for a GDBus implementation it could be used with g_variant_get()).. it's there more as a comment I guess.
Comment on attachment 198980 [details] [review] Updated GVfs patch Thanks for the review. Committed on master: http://git.gnome.org/browse/gvfs/commit/?id=2b5e26a0ca1f9a38fe981f431ee6fc29180d8fff
Closing bug. Tomas: For your gdbus branch, you will need to implement the same changes (e.g. the patch from comment 4).
(In reply to comment #10) > (In reply to comment #8) > > Review of attachment 198980 [details] [review] [details]: > > > > Looks good to me. > > > > ::: monitor/proxy/gproxydrive.c > > @@ -136,2 @@ > > */ > > -#define DRIVE_STRUCT_TYPE "(sssbbbbbbbbuasa{ss})" > > > > Why is this defined here, as well as in gvfsproxyvolumemonitordaemon.c? > > It doesn't seem used? > > It's not currently used because of the way things are unpacked using libdbus > (for a GDBus implementation it could be used with g_variant_get()).. it's there > more as a comment I guess. In my gdbus work-in-progress branch I've slightly modified these defines in gproxy{drive,mount,volume}.c to take advantage of const char * passing: -#define DRIVE_STRUCT_TYPE "(sssbbbbbbbbuasa{ss})" +#define DRIVE_STRUCT_TYPE "(&s&s&sbbbbbbbbuasa{ss})" Unmodified constants are actually used in gvfsproxyvolumemonitordaemon.c for variant type definition so let's keep them there for the moment.
(In reply to comment #12) > Tomas: For your gdbus branch, you will need to implement the same changes (e.g. > the patch from comment 4). Done.
Shouldn't the documentation say what this sort key is used for? At the moment it's rather boiler-plate.