GNOME Bugzilla – Bug 681458
Use symbolic icons for drives
Last modified: 2012-12-16 12:21:38 UTC
Nautilus has switched to using symbolic icons for drives, media, and bookmarks. A number of these are provided by gvfs. Apple File Connection (AFC) multimedia-player-apple-ipod-touch -> multimedia-player-apple-ipod-touch-symbolic computer-apple-ipad -> computer-apple-ipad-symbolic phone-apple-iphone -> phone-apple-iphone-symbolic Apple Filing Protocol (AFP) folder-remote-afp -> folder-remove-symbolic network-server-afp -> folder-remove-symbolic Archive drive-removable-media -> drive-removable-media-symbolic CDDA media-optical-audio -> media-optical-symbolic DAV folder-remote -> folder-remote-symbolic DNSSD network-workgroup -> network-workgroup-symbolic FTP folder-remote -> folder-remote-symbolic GPhoto multimedia-player -> multimedia-player-symbolic camera-photo -> camera-photo-symbolic Network network-workgroup -> network-workgroup-symbolic OBEX FTP drive-removable-media-usb -> folder-remote-symbolic? bluetooth -> bluetooth-symbolic
Created attachment 222711 [details] [review] Add support for getting symbolic icons
Created attachment 222712 [details] [review] Add symbolics support to the backends
Review of attachment 222711 [details] [review]: Looks mostly good to me ::: monitor/proxy/gproxydrive.c @@ +140,3 @@ * dict:string->string identifiers * string sort_key + * string symbolic_gicon_data I think it would be clearer to move this close to gicon_data in the drive struct. ::: monitor/proxy/gproxymount.c @@ +146,3 @@ * array:string x-content-types + * string sort_key + * string gicon_data This should read symbolic_gicon_data, but I think it would be better immediately after gicon_data ::: monitor/proxy/gproxyvolume.c @@ +354,3 @@ * dict:string->string identifiers * string sort_key + * string symbolic_gicon_data Same here. ::: programs/gvfs-mount.c @@ +482,3 @@ + { + if (G_IS_THEMED_ICON (icon)) + show_themed_icon_names (G_THEMED_ICON (icon), indent + 2); Maybe the symbolic icon names should be shown with a different heading in the output? (i.e. showing "symbolic themed icons: ...")
Review of attachment 222712 [details] [review]: Looks mostly good to me ::: daemon/gvfsbackendafc.c @@ +1517,3 @@ + { + g_themed_icon_append_name (G_THEMED_ICON(icon), "text-x-generic"); + g_themed_icon_append_name (G_THEMED_ICON(symbolic_icon), "text-x-generic-symbolic"); I think this needs to be wrapped in a separate if (G_IS_THEMED_ICON (symbolic_icon)) block ::: daemon/gvfsbackenddav.c @@ +1080,3 @@ + { + g_themed_icon_append_name (G_THEMED_ICON (icon), "text-x-generic"); + g_themed_icon_append_name (G_THEMED_ICON (symbolic_icon), "text-x-generic-symbolic"); Same here.
Review of attachment 222711 [details] [review]: ::: monitor/proxy/gproxydrive.c @@ +140,3 @@ * dict:string->string identifiers * string sort_key + * string symbolic_gicon_data Normally would agree but I was trying to append to the structure in order to try to not break ABI as I was interpreting what David said in: http://git.gnome.org/browse/gvfs/commit/?id=2b5e26a0ca1f9a38fe981f431ee6fc29180d8fff Though it is entirely possible I'm not doing what he intended.
Created attachment 222822 [details] [review] Add support for getting symbolic icons
Created attachment 222823 [details] [review] Add symbolics support to the backends
Patches look good to me, but I don't really have an answer for what you mention in comment #5. Tomas, Alex, any opinion?
Review of attachment 222822 [details] [review]: Okay
Review of attachment 222823 [details] [review]: Okay
These should be fine to commit together with the GLib API addition; if the modification I suggested in the order of arguments is acceptable, we can always do it later in a subsequent commit.
Created attachment 222893 [details] [review] Add support for getting symbolic icons
Created attachment 222894 [details] [review] Add symbolics support to the backends Requires the fix for https://bugzilla.gnome.org/show_bug.cgi?id=682892
Review of attachment 222893 [details] [review]: ++
Review of attachment 222894 [details] [review]: ++
(In reply to comment #5) > Normally would agree but I was trying to append to the structure in order to > try to not break ABI as I was interpreting what David said in: > http://git.gnome.org/browse/gvfs/commit/?id=2b5e26a0ca1f9a38fe981f431ee6fc29180d8fff > > Though it is entirely possible I'm not doing what he intended. Feel free to break d-bus protocol as needed, we did it several times in this cycle anyway. GDBus is also more strict when it comes to transferred arguments, it was inevitable to break it for the gdbus port. (In reply to comment #8) > Patches look good to me, but I don't really have an answer for what you mention > in comment #5. > Tomas, Alex, any opinion? Looks good to me too, it's a nice addition. ACK
Just FYI I recently added support in udisks2 to provide symbolic icons and I just added support for using this in the udisks2 GVfs volume monitor, see http://git.gnome.org/browse/gvfs/commit/?id=d2273d404cb3e895ba67052dde4a3e85b1c084ba http://people.freedesktop.org/~david/gvfs-udisks2-symbolic-icons.png
*** Bug 690288 has been marked as a duplicate of this bug. ***