After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 507152 - Drop gnome-vfs dependency
Drop gnome-vfs dependency
Product: epiphany
Classification: Core
Component: General
git master
Other Linux
: Normal enhancement
: ---
Assigned To: Epiphany Maintainers
Epiphany Maintainers
Depends on:
Blocks: 588322
Reported: 2008-01-04 01:05 UTC by Cosimo Cecchi
Modified: 2009-07-11 16:02 UTC
See Also:
GNOME target: ---
GNOME version: 2.21/2.22

all-inclusive patch (115.53 KB, patch)
2008-01-07 12:13 UTC, Cosimo Cecchi
reviewed Details | Review
fix roundup (6.60 KB, patch)
2008-01-07 14:41 UTC, Cosimo Cecchi
none Details | Review
fix roundup v2 (6.03 KB, patch)
2008-01-08 00:38 UTC, Cosimo Cecchi
none Details | Review
fix roundup v3 (894 bytes, patch)
2008-01-08 12:22 UTC, Cosimo Cecchi
none Details | Review
fix roundup v4 (542 bytes, patch)
2008-01-08 14:02 UTC, Cosimo Cecchi
none Details | Review
fix roundup v5 (10.71 KB, patch)
2008-01-09 01:17 UTC, Cosimo Cecchi
none Details | Review
avahi migration (15.42 KB, patch)
2008-01-09 01:19 UTC, Cosimo Cecchi
none Details | Review
all-inclusive patch v2 (134.47 KB, patch)
2008-01-11 20:16 UTC, Cosimo Cecchi
accepted-commit_now Details | Review
committed patch (134.12 KB, patch)
2008-01-13 20:44 UTC, Cosimo Cecchi
committed Details | Review

Description Cosimo Cecchi 2008-01-04 01:05:48 UTC
I'm writing a patch to drop gnome-vfs dependency and use gio from glib 2.15...will attach here once complete and working.
Comment 1 Cosimo Cecchi 2008-01-07 12:13:49 UTC
Created attachment 102315 [details] [review]
all-inclusive patch

Attached patch removes almost all dependencies on gnome-vfs, introducing dep on glib >= 2.15.0.
There are two places which are not ported yet: ephy-main.c, due to gnome_vfs_shutdown () (which I guess will be the last thing to remove) and ephy-bookmarks.c, due to the Avahi/Zeroconf stuff, which I had no time for now to look at.
Also some other comments: I had to write a quite simple ephy_string_get_host_name (url) to replace gnome_vfs_uri_get_host_name (). If it's not optimal, feel free to improve it; in my testing everything works fine as without patching, but there may be some corner cases I did not take into accout.
Also, if you want a splitted per-file patch (or per-git-commit) just ask :)
Comment 2 Cosimo Cecchi 2008-01-07 12:24:16 UTC
I forgot to say I also marked for translatable a string in ephy-download.c, which wasn't before.
Comment 3 Christian Persch 2008-01-07 13:35:24 UTC
Thanks, this looks great!

Some comments:

+  if (host != NULL) {
+    g_free (host);

g_free is NULL-safe.

+    if (G_IS_FILE_MONITOR (priv->monitor))
+    {
+      g_file_monitor_cancel (G_FILE_MONITOR (priv->monitor));
+    }
+    if (G_IS_DIRECTORY_MONITOR (priv->monitor))

|else if| ?

ephy_base_embed_directory_monitor_cb (GDirectoryMonitor *monitor,
+                                      GFile *child,
+                                      GFile *other_child,
+                                      gint event_type,
+                                      EphyBaseEmbed *embed)

glong event_type, since it's an enum (or GFileMonitorEvent). Same below.

+  }
+  if (file_type == G_FILE_TYPE_REGULAR)

|else if| ?

+		path = g_strconcat (g_file_get_path (dir), G_DIR_SEPARATOR_S,
+				    g_file_info_get_edit_name (file_info),

Why the edit-name, not the name of the file ?

+					       "standard::type,standard::edit-name,"

same question.

+	file_info = g_file_enumerator_next_file (file_enum, NULL, NULL);

Does this return a reference? The docs aren't clear on that, perhaps file a gio bug for that.

+	if (scheme != NULL)
-		gnome_vfs_uri_unref (vfs_uri);
+		g_free (scheme);
+	}
+	if (host_name != NULL)
+	{
+		g_free (host_name);

g_free is NULL-safe.

+user_css_file_monitor_changed_cb (GFileMonitor *file_monitor,
+				  GFile *file,
+				  GFile *other_file,
+				  gint event_type,
+				  MozillaEmbedSingle *single)

see above.

+#include <gio/gdesktopappinfo.h>
+#include <gio/gio.h>

gdesktopappinfo.h comes from gio-unix, right? Is there no global header like gio/gio.h for that? Maybe worth filing a gio bug...

-	expanded = gnome_vfs_expand_initial_tilde (download_dir);
-	g_free (download_dir);
-	return expanded;
+	return download_dir;

That's a behaviour change; return ephy_file_expand_initial_tilde here ?

+	app = g_desktop_app_info_new (filename);
+	file = g_file_new_for_path (parameter);
+	list = g_list_append (list, file);
+	return ephy_file_launch_application (G_APP_INFO (app), list, user_time, widget);

Doesn't this leak |app| ?

+		parent = g_file_get_parent (file);
 		/* TODO find a way to make nautilus scroll to the actual file */
 		ret = ephy_file_launch_handler ("x-directory/normal", 
-						gnome_vfs_uri_get_path (parent_uri), 
+						parent, 

Opening the file manager at a given file is a common operation; gio should really offet API for it. File a gio bug ?

-	g_free (tmp_file);
+	g_free (tmp_file_path);
 	return ret;

Don't you leak the 2 GFile objects here ?

+		address = g_strconcat (
+				       scheme,
+				       "://",
+				       host_name,
+				       NULL);

No need for (\n here, just put the 1st arg on the same line.

+	handler = eel_gconf_get_string ("/desktop/gnome/url-handlers/mailto/command");

Need to check the enabled key in the same dir too before using the command.
(And the same below in window-commands.c.)


The code for reading/writing with libxml is ok, but there really should be common code somewhere in gnome (libgnome?) or for c&p, to make libxml read/write from a GFile or GInput/OutputStream...
Comment 4 Cosimo Cecchi 2008-01-07 14:41:15 UTC
Created attachment 102328 [details] [review]
fix roundup

Christian: attached patch applies on top of the previous one and is a roundup of fixes for your comments.
Some notes:
- you were right about GFileEnumerator returning refs, I will file a bug about that. Also I was missing a call to g_file_enumerator_close () when finish.
- I used GFileMonitorEvent instead of gint, though Devhelp documentation of glib 2.15.0 has the prototype of the "changed" callback with gint. I will file a bug about that too, if it's not modified in glib trunk.
- There's no unique header to include for the unix part of GIO.
Comment 5 Christian Persch 2008-01-07 17:42:32 UTC
gio change from today:
+  - GDirectoryMonitor has been removed; GFileMonitor can monitor
+    files and directories now
Comment 6 Cosimo Cecchi 2008-01-08 00:38:27 UTC
Created attachment 102361 [details] [review]
fix roundup v2

Attached patch applies on top of the other two and:
- updates to new GFileMonitor API
- bump dep on glib >= 2.15.1.
- fix some other minor flaws I found around.
Comment 7 Cosimo Cecchi 2008-01-08 09:24:25 UTC
(In reply to comment #3)
> Why the edit-name, not the name of the file ?
> +                                             
> "standard::type,standard::edit-name,"
> same question.

(I missed this question yesterday)
I used it because it seemed to me the safest option (as the "name" attribute could be in an unknown encoding according to gio docs), but I think we might use "copy-name" instead, as it is said to be always UTF-8. What do you think?
Comment 8 Christian Persch 2008-01-08 09:58:10 UTC
You're using the filename not for display but for working with the file (deleting it), so you should use the real name.
Comment 9 Cosimo Cecchi 2008-01-08 12:22:02 UTC
Created attachment 102388 [details] [review]
fix roundup v3

Use name instead of edit-name (applies on top of the other patches).
Comment 10 Cosimo Cecchi 2008-01-08 12:26:52 UTC
(In reply to comment #3)

> The code for reading/writing with libxml is ok, but there really should be
> common code somewhere in gnome (libgnome?) or for c&p, to make libxml
> read/write from a GFile or GInput/OutputStream...

I thought the same thing when reading that part of code. Is there a bug filed somewhere/anyone you know working on such a thing?

Comment 11 Christian Persch 2008-01-08 12:33:53 UTC
I think libgsf can do that (libgsf module in gnome svn), and it just got gio support.... However I'm not sure depending on that huge lib is the best thing to do...
Comment 12 Christian Persch 2008-01-08 12:50:52 UTC
Looks good so far.

libgsf is listed on the external dependencies page, but in version 1.14.5 while the gio support is only in .6 and probably needs trunk for gio trunk...

One other thing: since bookmarks im/export uses GFile now, we should be able to allow remote files (set local-only = FALSE) for their filechoosers, and use get_uri instead of get_filename.
Comment 13 Christian Persch 2008-01-08 13:46:58 UTC
 		path = g_strconcat (g_file_get_path (dir), G_DIR_SEPARATOR_S,
-				    g_file_info_get_edit_name (file_info),

Use g_build_filename.

The further changes (libgsf?, and allowing non-local files which probably depends on that) can be done when they're ready, shouldn't block this.

Xan, could you take a look over the patches too?
Comment 14 Cosimo Cecchi 2008-01-08 14:02:00 UTC
Created attachment 102392 [details] [review]
fix roundup v4

Use g_build_filename ().

So, a summary of what's missing:
- to remove completely the dep on gnome-vfs we need to port all the Avahi stuff in ephy-bookmarks.c (IMHO this has priority so we can stop linking to gnome-vfs).
- replace the libxml parts with libgsf using their gsf-gvfs backend.
Comment 15 Christian Persch 2008-01-08 14:14:11 UTC
(In reply to comment #14)
> - to remove completely the dep on gnome-vfs we need to port all the Avahi stuff
> in ephy-bookmarks.c (IMHO this has priority so we can stop linking to
> gnome-vfs).

Definitely has priority, yes.

Comment 16 Xan Lopez 2008-01-08 20:19:49 UTC
Some minor nits:

+  if (g_str_has_prefix (address, "file://"))
+  {
+    return g_strdup (address + 7);
+  }
+  return ephy_string_get_host_name (address);

K&R braces (happens in more places in the same file), maybe an else?

+	if (mime_type == NULL || type != G_FILE_TYPE_REGULAR)
+	{
+		return;
+	}
+	else
+	{

Mmm, that is really strange, can you invert the logic and get rid of the else?

-	mime_description = gnome_vfs_mime_get_description (mMimeType.get());
+	mime_description = g_content_type_get_description (mMimeType.get());

you need to free mime_description now I think.

         char *filename;
-        filename = gnome_vfs_unescape_string (default_name, NULL);
+        filename = g_uri_unescape_string (default_name, NULL);
Can we assign in the declaration? :)

-		uri = gnome_vfs_get_uri_from_local_path (priv->user_css_file);
+		file = g_file_new_for_path (priv->user_css_file);

Missing g_object_unref for file. (I think you are missing this in most places. Either that or I'm missing something :))
Comment 17 Cosimo Cecchi 2008-01-09 01:17:56 UTC
Created attachment 102430 [details] [review]
fix roundup v5

Attached patch plugs mem leaks and style issues as for Xan comment.
Comment 18 Cosimo Cecchi 2008-01-09 01:19:22 UTC
Created attachment 102431 [details] [review]
avahi migration

Attached patch migrates ephy-bookmarks.c to just fine in my (quite limited) testing.
Comment 19 Cosimo Cecchi 2008-01-09 01:22:53 UTC
Note that if you get a warning about a wrong GaProtocol when running Ephy with the avahi patch, it should be a bug in avahi-gobject using a wrong enum type for a ParamSpec (I'll report the bug to Sjoerd).
Comment 20 Cosimo Cecchi 2008-01-09 12:48:04 UTC
I talked with Sjoerd and the warning is fixed in Avahi SVN now (though the warning is non fatal, so it works fine also with 0.6.22).
Comment 21 Cosimo Cecchi 2008-01-11 20:16:53 UTC
Created attachment 102620 [details] [review]
all-inclusive patch v2

All-in-one patch.
With this I removed last bits of gnome-vfs from and ephy-main.c, making Ephy completely gnome-vfs free. I also removed HAVE_GFVS checks for "gio-standalone" in, as that module does not exist anymore.
Comment 22 Christian Persch 2008-01-13 17:51:43 UTC
+static char*
 get_title_from_address (const char *address)
-  GnomeVFSURI *uri;
-  char *title;
-  if (address == NULL) return NULL;

Not sure if that NULL-check was ever needed... it's probably ok to remove it.

Does ephy_base_embed_update_file_monitor leak the |file| ?

+	type = g_file_info_get_file_type (file_info);
+	mime_type = g_file_info_get_content_type (file_info);
+	if (mime_type != NULL && type == G_FILE_TYPE_REGULAR)

Why do we need to get the mime type here?

+	file_enum = g_file_enumerate_children (dir,
+					       "standard::type,standard::name,"
+					       "standard::content-type",
+					       0, NULL, NULL);

Let's use the defines here. I.e.:

+	/* FIXME: better use g_app_info_get_default_for_uri_scheme () when it is
+	 * implemented.
+	 */

File a ephy bug so we don't forget about this :)

With those nits fixed, please commit. Thanks very much for the patches! 
Comment 23 Cosimo Cecchi 2008-01-13 20:44:10 UTC
Created attachment 102764 [details] [review]
committed patch

Attaching here the patch that was committed.
Comment 24 Cosimo Cecchi 2008-01-13 20:44:37 UTC
Closing as FIXED.

r7858 | cosimoc | 2008-01-13 21:42:01 +0100 (dom, 13 gen 2008) | 4 lines

Drop gnome-vfs dependency. Now Epiphany depends on glib >= 2.15.1.
Also, optional Zeroconf support depends on Avahi >= 0.6.22.
Bug #507152.
Comment 25 Diego Escalante Urrelo (not reading bugmail) 2008-01-13 20:48:18 UTC
*diego fanboys cosimo