GNOME Bugzilla – Bug 696860
filesystem: Add support for non-file URIs
Last modified: 2013-11-21 09:07:31 UTC
Unfinished, as per commit log.
Created attachment 240126 [details] [review] filesystem: Add support for non-file URIs UNFINISHED: If somebody wants to finish this off during the long week-end. Use URIs all across the plugin, instead of paths. This also fixes a number of memory leaks, and uses convenience functions such as g_file_equal() when available. This enhancement will be used in Totem to show recent videos (available through recent://) and remote filesystems where applicable.
Created attachment 240515 [details] [review] filesystem: Add support for non-file URIs Use URIs all across the plugin, instead of paths. This also fixes a number of memory leaks, and uses convenience functions such as g_file_equal() when available. This enhancement will be used in Totem to show recent videos (available through recent://) and remote filesystems where applicable.
Created attachment 240533 [details] [review] test-ui: Update for filesystem plugin changes Update for filesystem plugin changes (base-path -> base-uri), and add recent:/// to the list, as an example.
Review of attachment 240515 [details] [review]: NOTE 1: In Grilo, the root container is identifier because it has a NULL identifier. So, if our base-uri is "file:///", the URL returned in grl_filesystem_source_resolve(FS, NULL) should be that URL. So summing up, while we can use the URL as the identifier, we need to handle the case of root container, which should have the ID as NULL, even when the URL is well-known defined. ::: src/filesystem/grl-filesystem.c @@ +502,3 @@ if (!media) { media = grl_media_new (); + grl_media_set_id_from_file (media, file); See note 1 @@ +968,3 @@ GFile *directory; + home = g_get_home_dir (); I think we should use what devhelp suggests: const char *homedir = g_getenv ("HOME"); if (!homedir) homedir = g_get_home_dir (); Reason is, there are projects that changes the HOME envvar to set up the new home depending on several conditions. As g_get_home_dir() doesn't check this envvar, I suggest to check first and if not available, then use g_get_home_dir(). @@ +1295,3 @@ id = grl_media_get_id (rs->media); + if (!id) + return; We can't just return without invoking the callback. Otherwise, the caller will be blocked waiting for the callback, which will never arrive. Also, see note 1.
(In reply to comment #4) <snip> > @@ +968,3 @@ > GFile *directory; > > + home = g_get_home_dir (); > > I think we should use what devhelp suggests: > > const char *homedir = g_getenv ("HOME"); > if (!homedir) > homedir = g_get_home_dir (); > > Reason is, there are projects that changes the HOME envvar to set up the new > home depending on several conditions. As g_get_home_dir() doesn't check this > envvar, I suggest to check first and if not available, then use > g_get_home_dir(). glib/gutils.c in g_get_home_dir(): 887 /* We first check HOME and use it if it is set */• 888 tmp = g_strdup (g_getenv ("HOME"));• Best read current API docs :) https://developer.gnome.org/glib/stable/glib-Miscellaneous-Utility-Functions.html#g-get-home-dir
Created attachment 241043 [details] [review] filesystem: Add support for non-file URIs Use URIs all across the plugin, instead of paths. This also fixes a number of memory leaks, and uses convenience functions such as g_file_equal() when available. This enhancement will be used in Totem to show recent videos (available through recent://) and remote filesystems where applicable.
Comment on attachment 240533 [details] [review] test-ui: Update for filesystem plugin changes commit 46eab031ff7c871f16fbbf67d9cddddae7b6cf85 Author: Bastien Nocera <hadess@hadess.net> Date: Wed Apr 3 20:25:28 2013 +0200 test-ui: Update for filesystem plugin changes Update for filesystem plugin changes (base-path -> base-uri), and add recent:/// to the list, as an example. https://bugzilla.gnome.org/show_bug.cgi?id=696860 tools/grilo-test-ui/main.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) Attachment 240533 [details] pushed as 46eab03 - test-ui: Update for filesystem plugin changes
commit 0b5b0aee7e13697faea4e6e9eb2475f595378a63 Author: Bastien Nocera <hadess@hadess.net> Date: Fri Mar 29 15:30:22 2013 +0100 filesystem: Add support for non-file URIs Use URIs all across the plugin, instead of paths. This also fixes a number of memory leaks, and uses convenience functions such as g_file_equal() when available. This enhancement will be used in Totem to show recent videos (available through recent://) and remote filesystems where applicable. https://bugzilla.gnome.org/show_bug.cgi?id=696860 src/filesystem/grl-filesystem.c | 411 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++---------------------------------------------------- src/filesystem/grl-filesystem.h | 2 +- 2 files changed, 215 insertions(+), 198 deletions(-) Attachment 241043 [details] pushed as 0b5b0ae - filesystem: Add support for non-file URIs
This introduces an API break. I have a unit test that use "base-path" to browse a specific test directory, and now it started browsing my root filesystem because grilo only supports "base-uri". This has been even more difficult to catch because the config key is a string instead of a C symbol so it was not catched at build time. And even at runtime grl_config_set_string() does not verify if the config actually exists, it should at least g_warning. I don't know how it's implemented so I don't know if it's easy/possible to do though. I suggestion adding back "base-path" and if that config is set, the filesystem could use base_uri = g_filename_to_uri(base_path, NULL); Reopening.
(In reply to comment #9) > This introduces an API break. I have a unit test that use "base-path" to browse > a specific test directory, and now it started browsing my root filesystem > because grilo only supports "base-uri". Can't you replace your "base-path" with "base-uri"? If you have base-path defined as "/one/directory", now it would be a base-uri defined as "file:///one/directory"
Sure I can, but I think grilo should be backward compatible. It though me ages to figure out why the test were broken. But if you consider your API not stable, then feel free to close wontfix ;-)
We try to be as stable as possible at least in the core. On the other hand, probably we can do better about how to communicate such breaks. For instance, we could include this changes somewhere in the NEWS, so people can quickly see any change that could affect the applications. Something I'll take in account for next releases. Apologies for inconvenience.