GNOME Bugzilla – Bug 555740
gicon serialization
Last modified: 2008-10-21 11:58:20 UTC
GIcon is a convenient way of representing what an icon looks like (support themed icons, file based icons, emblems). For applications needing to store an icon reference on persistent storage (such as the connected-server feature) or pass a GIcon reference around processes (such as gvfs), it would be nice to be able to serialize a GIcon and construct a new GIcon from it. The API would be something like char *g_icon_serialize (GIcon *icon); on the GIcon interface. We'd also add a constructor GIcon *g_icon_new_from_data (const char *serialized_data, GError *error); which is allowed to fail. The storage format should satisfy the following properties 1. Should return a NUL-terminated string. 2. For GFileIcon pointing to a native URI, the storage format should be just the path, e.g. "/usr/share/pixmaps/apple-red.png" The former requirement means it's easy to use with existing API's (GConf, DBus), the latter means it can be used with older storage systems. It's worth noting we already do something like this in the proxy volume monitor. The code should be able to be reused. It's probably fine to just assume all GIcon subclasses will be in libgio. If we don't want this assumption we need to add some API to the GIcon interface so subclasses can participate in the serialization game.
There is also some code in the daemon communications code (for passing GFileInfos). I've wanted a more generic way to do this, and in fact had some ideas for a generic serialization API, but never got there.
Created attachment 120449 [details] [review] proposed patch This patch should do the trick including making similar guarantees for GThemedIcon instances with a single name. There's also tests for all this. Credit goes to Matthias Clasen for writing this for gvfs.
If we move this to gio, I think it would be much cleaner to do it via a serialize vfunc in GIcon.
(In reply to comment #3) > If we move this to gio, I think it would be much cleaner to do it via a > serialize vfunc in GIcon. I think it's more complicated than that - you need to handle both writing and reading. Either way, it's something we can add later (and I'll queue it up on my TODO list if you want) - but I think it's sorta academic at this point; am not aware of any GIcon implementations outside libgio. I think it's more important to get this in ASAP as other features depend on it.
Ok, I guess you are right that the the code is good enough as is. You need to add <para></para> around the content of the <listitem> elements, and please add the new functions to gio.symbols and gio-sections.txt. Thanks for adding the testcases, btw.
A more flexible implementation could have a GIcon method for to-string and a from-string method on the class, and then the serialization form could always start with the GType name so we could look up the class (by name).
Created attachment 120950 [details] [review] updated patch OK, here's a patch that uses vfuncs - turns out we need this since we want something like GVfsPreviewIcon as discussed here http://mail.gnome.org/archives/gvfs-list/2008-October/msg00055.html It would probably also be good to version the encoded data since - otherwise we can't extend existing icons - in a star trek future we might standardize the encoding on fd.o Versioning exists in two places - In the main string - For each icon type I've left some debug code (#if'ed out) in on purpose since that's useful when adding new GIcon implementations. Here's the output of the debug code when running the test suite (it also shows the (simpler, but bulkier) encoding used now: $ ./g-icon /g-icon/serialize: serialized icon with hash 0x9213fe60 into '/some/native/path/to/an/icon.png' deserialized icon with hash 0x9213fe60 from '/some/native/path/to/an/icon.png' serialized icon with hash 0x2d510c07 into '/some/native/path/to/an/icon with spaces.png' deserialized icon with hash 0x2d510c07 from '/some/native/path/to/an/icon with spaces.png' serialized icon with hash 0xb526a310 into 'sftp:///some/non-native/path/to/an/icon.png' deserialized icon with hash 0xb526a310 from 'sftp:///some/non-native/path/to/an/icon.png' serialized icon with hash 0x9986d0f7 into 'sftp:///some/non-native/path/to/an/icon%20with%20spaces.png' deserialized icon with hash 0x9986d0f7 from 'sftp:///some/non-native/path/to/an/icon%20with%20spaces.png' serialized icon with hash 0x4db60fc2 into 'network-server' deserialized icon with hash 0x4db60fc2 from 'network-server' deserialized icon with hash 0x4db60fc2 from 'network-server' deserialized icon with hash 0x80f05975 from '/path/to/somewhere.png' deserialized icon with hash 0x4ce473ec from '/path/to/somewhere with whitespace.png' serialized icon with hash 0x4ce473ec into '/path/to/somewhere with whitespace.png' deserialized icon with hash 0x80661ec5 from 'sftp:///path/to/somewhere.png' serialized icon with hash 0x80661ec5 into 'sftp:///path/to/somewhere.png' deserialized icon with hash 0x4c72212c from 'sftp:///path/to/somewhere with whitespace.png' serialized icon with hash 0x4c72212c into 'sftp:///path/to/somewhere%20with%20whitespace.png' serialized icon with hash 0x91ff70d9 into 'GIconSerialization0 GThemedIcon 0 network-server computer' deserialized icon with hash 0x91ff70d9 from 'GIconSerialization0 GThemedIcon 0 network-server computer' serialized icon with hash 0x72c7b0d2 into 'GIconSerialization0 GThemedIcon 0 icon%20name%20with%20whitespace computer' deserialized icon with hash 0x72c7b0d2 from 'GIconSerialization0 GThemedIcon 0 icon%20name%20with%20whitespace computer' serialized icon with hash 0x9cc48ff9 into 'GIconSerialization0 GThemedIcon 0 network-server-xyz network-server network computer' deserialized icon with hash 0x9cc48ff9 from 'GIconSerialization0 GThemedIcon 0 network-server-xyz network-server network computer' serialized icon with hash 0xe16ca458 into 'face-smirk' serialized icon with hash 0x95420757 into 'GIconSerialization0 GThemedIcon 0 emblem-important emblem-shared' serialized icon with hash 0x95420756 into 'GIconSerialization0 GEmblem 0 GIconSerialization0%20GThemedIcon%200%20emblem-important%20emblem-shared 1' serialized icon with hash 0x357cb9b0 into '/some/path/somewhere.png' serialized icon with hash 0x357cb9b2 into 'GIconSerialization0 GEmblem 0 %2Fsome%2Fpath%2Fsomewhere.png 2' serialized icon with hash 0x41521abc into 'GIconSerialization0 GEmblemedIcon 0 face-smirk GIconSerialization0%20GEmblem%200%20GIconSerialization0%2520GThemedIcon%25200%2520emblem-important%2520emblem-shared%201 GIconSerialization0%20GEmblem%200%20%252Fsome%252Fpath%252Fsomewhere.png%202' deserialized icon with hash 0xe16ca458 from 'face-smirk' deserialized icon with hash 0x95420757 from 'GIconSerialization0 GThemedIcon 0 emblem-important emblem-shared' deserialized icon with hash 0x95420756 from 'GIconSerialization0 GEmblem 0 GIconSerialization0%20GThemedIcon%200%20emblem-important%20emblem-shared 1' deserialized icon with hash 0x357cb9b0 from '/some/path/somewhere.png' deserialized icon with hash 0x357cb9b2 from 'GIconSerialization0 GEmblem 0 %2Fsome%2Fpath%2Fsomewhere.png 2' deserialized icon with hash 0x41521abc from 'GIconSerialization0 GEmblemedIcon 0 face-smirk GIconSerialization0%20GEmblem%200%20GIconSerialization0%2520GThemedIcon%25200%2520emblem-important%2520emblem-shared%201 GIconSerialization0%20GEmblem%200%20%252Fsome%252Fpath%252Fsomewhere.png%202' OK That's it.
docs/reference/gio/gio-sections.txt | 2 gio/gemblemedicon.c | 115 +++++++++++++ gio/gfileicon.c | 64 +++++++ gio/gicon.c | 300 +++++++++++++++++++++++++++++++++++- gio/gicon.h | 29 ++- gio/gio.symbols | 2 gio/gthemedicon.c | 64 +++++++ gio/tests/Makefile.am | 6 gio/tests/g-icon.c | 241 ++++++++++++++++++++++++++++ 9 files changed, 812 insertions(+), 11 deletions(-)
Note that we're not storing the use-default-fallbacks property on GThemedIcon since it's not used in the equal() method. It's probably also a bug that it's a readable property; any implementation relying on it is wrong as it should _only_ look at the names.
Hmm. We also need to ensure that all GIcon types are registered. The naive approach is to call g_themed_icon_get_type (); g_file_icon_get_type (); g_emblemed_icon_get_type (); g_emblem_get_type (); in the beginning of g_icon_new_for_string(); any better place for this? We should also mention in the GIcon docs that implementers (such as gvfs) need to do the same. Yucky. Hmm. Is there a facility in GObject for automatically registering types at startup? I believe C++ relies on some magic for running some kind of constructors before main(); we should be able to use the same facility.
maybe even s/startup/g_type_init() time/.
Created attachment 120982 [details] [review] new patch Here's a patch that a) ensures the built-in types are registered; and b) specifies that implementations need to register the types before calling g_icon_new_for_string().
Looks generally ok to me. Some details: + if (!G_IS_EMBLEM (emblem)) + { + g_set_error_literal (error, + G_IO_ERROR, + G_IO_ERROR_INVALID_ARGUMENT, + _("Expected a GEmblem for GEmblemedIcon")); + goto fail; + } Looks like we are leaking emblem here, should it ever be != NULL, but not an emblem. +static char * +_g_icon_to_string_tokenized (GIcon *icon) No need to _-prefix static functions. (More instances of this further down) + if (icon_iface->to_tokens == NULL) + { + goto out; + } glib coding style omits {} around single statements. (More instances of this further down) + g_type_name (G_TYPE_FROM_INSTANCE (icon)), More succinct: g_type_name_from_instance (icon); + if (names != NULL) + { + if (g_strv_length (names) == 1) No need to count here, I'd just say if (names != NULL && names[1] == NULL) Of course, then you have to move the strfreev out of the body of the if. s = g_strdup (names[0]); + ret = g_uri_escape_string (s, NULL, TRUE); + g_free (s); That strdup looks pointless to me ? + s = _g_icon_to_string_tokenized (icon); + ret = g_strdup_printf (G_ICON_SERIALIZATION_MAGIC0 "%s", s); + g_free (s); I think you want to check s for NULL here, else you may return "GIconSerialization0 NULL" Please remove the #if0ed debug prints before committing. I think the docs for g_icon_new_for_string() should also contain the warning to register custom icon types before calling it. Or maybe only do the warning in the function docs.
I fixed the issues pointed out by mclasen, and commited with some further changes: To make the encoding less verbose i changed G_ICON_SERIALIZATION_MAGIC0 to "." (icon names should not generally start with a dot, and if they do we force full serialization). I also made the versions implicit in the encoding if it is zero. I ensure that raw filenames and themed icon names are valid utf8 (or we fall back to full serialization) which guarantees that the full serialized string is utf8 (useful when e.g. passing the string via dbus). I moved all escaping and unescaping into the general code, simplifying all the per-icon implementations. I also changed the to_token API to take a GPtrArray to further simplify them. I allow more characters when escaping the tokens, as we only really need to escape spaces and non-utf8 text. GEmblem serialization was missing in the patch, so I added it. to_tokens can fail (if some inner GIcon doesn't support to_tokens), so allow that.