Great. How far along is your patch? Are you still working on it? Do you need help? Note that Thomas Wood is currently working on rewriting the theme info parts in capplets/common/ so don't waste any time on that.

Hi! I spent some time reading gnome-control-center code to understand how it works. Today I wrote the first patch porting a little piece of appearance capplet to gio. The patch will be attached.

I have a little question about appearance capplet. Some functions using uri are able to receive paths as parameter or returning paths (as gnome_theme_install_from_uri), should I care about this and take as a standard that only valid uris should be used?

In general, the biggest problem will be rewrite themes:// and fonts:// but I want to finish appearance capplet before thinking in that.


It's my first patch for gnome-control-center and one of few patches I send to gnome, so all help will be welcome. Thanks =)

ps.: Thanks for advising about capplets/common!

Created attachment 108839 [details] [review]
porting appearance-desktop.c to gio

Looks pretty good for a first, but please be consistent with indentation. Don't mix spaces and tabs.

-      for (; uris != NULL; uris = uris->next)
+      i = 0;
+      while ((uri = uris[i++]) != NULL)
       {

A for loop seems more natural here.

for (i = 0; uris[i]; ++i) ...

+                f = g_file_new_for_uri (uri);
         realuris = g_slist_append (realuris,
-            g_strdup (gnome_vfs_uri_get_path (uris->data)));
+ g_strdup (g_file_get_path (f)));

g_file_get_path already returns a newly allocated string. if you g_strdup that again, you'll leak memory.

In general, I think we should invest a bit more effort here and make the app work exclusively with URIs (or maybe even GFiles if that makes sense) internally and never use paths (except for display). There are several subtle bugs currently because URIs and paths are not used consistently. It's a bit more work, but it'll be worth it.

       wp_add_images (data, realuris);
       gdk_window_set_cursor (window, NULL);
     }
-    gnome_vfs_uri_list_free (uris);
+
+    g_strfreev (uris);

You can pull that inside the if branch.

+      g_warning (_("Unable to get mime_type from file %s"), uri);

We don't need to translate warning messages to the console.

+    if (mime_type && error != NULL)
+    {
       pixbuf = gnome_thumbnail_factory_generate_thumbnail 
(data->thumb_factory,
                                                            uri,
                                                            mime_type);

I suspect you wanted "error == NULL", but it would be easier to initialise mime_type = NULL at the beginning.

Created attachment 108873 [details] [review]
New try to port appearance-desktop.c to gio

Hi Jens,

Thank you very much for help improving the patch. As I said it's my first try helping gnome with code and I think this is very important for people starting to contribute.

I followed your suggestions in this new patch. If you have some time, please take a look again =)

About the exclusive use of URI's and GFiles. I did not include in this patch because some things I think we should convert for this depends on parts that were not ported yet. For example gnome_wp_item_new. Or port base libraries is before a better idea?

Looks good. The only change I'd make is to move this line

+    g_object_unref (file);

up to directly after the g_file_query_info, since you don't use it after that.

Regarding the URIs I can see two possible approaches. One is to try to identify places that aren't affected by switching to URIs and can be converted to gio without any major modifications and do those first. The danger here is that when finally switching to URIs you might realise that some of the places you thought were unaffected, weren't after all, and you'll have to convert them twice.

The second approach would be to switch to URIs right away and then go through and fix all occurences of paths and convert to gio in the same process. The advantage here is you only have to do things once, the disadvantage is that you can't really do it piecemeal and a big patch is a bit harder to review.

Either way would be fine with me. The worst thing you could do is probably converting everything to gio first, and then switching to URIs afterwards. That way you'd have to do all the work twice, basically.

Created attachment 109036 [details] [review]
little improvement

Moving g_object_unref (file) to directly below g_file_query_info.

complete list is:

$:andre\> grep -l -R gnome_vfs .
./libslab/document-tile.c
./libslab/app-shell.c
./libslab/slab-gnome-util.c
./libslab/bookmark-agent.c
./libslab/directory-tile.c
./libslab/egg-recent-model.c
./libslab/egg-recent-item.c
./libslab/recent-files.c
./vfs-methods/fontilus/font-view.c
./vfs-methods/fontilus/ftstream-vfs.c
./vfs-methods/fontilus/thumbnailer.c
./vfs-methods/fontilus/fontilus-context-menu.c
./vfs-methods/fontilus/font-method.c
./vfs-methods/themus/theme-method.c
./vfs-methods/themus/themus-theme-applier.c
./vfs-methods/themus/theme-thumbnailer.c
./vfs-methods/themus/themus-properties-view.c
./capplets/network/gnome-network-preferences.c
./capplets/common/gnome-theme-test.c
./capplets/common/gnome-theme-info.c
./capplets/appearance/gnome-wp-item.c
./capplets/appearance/appearance-themes.c
./capplets/appearance/appearance-main.c
./capplets/appearance/theme-save.c
./capplets/appearance/theme-util.c
./capplets/appearance/theme-installer.c
./capplets/appearance/gnome-wp-info.c
./capplets/appearance/appearance-desktop.c
./capplets/appearance/gnome-wp-xml.c

(In reply to comment #6)
> Looks good. The only change I'd make is to move this line
> 
> +    g_object_unref (file);
> 
> up to directly after the g_file_query_info, since you don't use it after that.
Done!

> Regarding the URIs I can see two possible approaches. One is to try to identify
> (...)
> converting everything to gio first, and then switching to URIs afterwards. That
> way you'd have to do all the work twice, basically.
> 

Thanks to answer again =)

I'm very excited with this work and I'd like to follow the second approach, to do this without give you big patches hard to review, I'll try to make little patches for each functionality using GnomeVFS, so we can migrate pieces of code instead of the whole in a single patch. I already ported gnome-wp-xml.c removing all gnome_vfs symbols, I'll make my revision and today I'll upload it here!

lincoln: thanks for your work in advance, i should say. :-)
tracking page for entire gnome is located at http://live.gnome.org/GioPort
if there's any technical questions, #nautilus on irc is a good place to ask.

(In reply to comment #8)
> ./libslab/...

libslab is part of gnome-main-menu, so out of scope for this bug.

> ./vfs-methods/fontilus/...
> ./vfs-methods/themus/...

Before blindly converting these we need to discuss whether we actually want to keep them.

> ./capplets/common/...

Being worked on by Thomas as mentioned earlier. Don't touch.

That leaves just

> ./capplets/network/...
> ./capplets/appearance/...

for now.

> if there's any technical questions, #nautilus on irc is a good place to ask.

I'd argue that #control-center should go first.

(In reply to comment #11)
> libslab is part of gnome-main-menu, so out of scope for this bug.

filed bug 527903.

> > if there's any technical questions, #nautilus on irc is a good place to ask.
> I'd argue that #control-center should go first.

well... i refered to gio questions. in general, you're of course right. :)

(In reply to comment #10)
> lincoln: thanks for your work in advance, i should say. :-)
> tracking page for entire gnome is located at http://live.gnome.org/GioPort
> if there's any technical questions, #nautilus on irc is a good place to ask.
> 

Thanks, I'll need to visit #nautilus channel many times while working on this issue =)

Created attachment 109190 [details] [review]
gnome-wp-xml migration

Sorry for delay, too many work these times =)

Ihu!

lincoln@indigente:~/Work/contrib/gnome-control-center-trunk/capplets/appearance$ find . -name '*.c' | xargs grep gnome_vfs| wc -l
55

Created attachment 109191 [details] [review]
new try of gnome-wp-xml port =/

Sorry, I forgot to include ChangeLog diff and I found a little bug freeing a wrong pointer in the last patch =/

One general remark regarding error checking. You always pass a GError to functions which take them, even if you're not interested in the result and just put out a message and continue, or if the function also has a return code that indicates success and that's all you need. Full error handling with GError is pretty tedious and makes the code harder to read. So, in cases where you don't really need the error, just pass NULL.

+static void gnome_wp_file_changed (GFileMonitor *monitor,
+				   GFile *file,
+				   GFile *other_file,
+                                   GFileMonitorEvent event_type,
+                                   AppearanceData *data) {

Indentation is off here. Please don't mix tabs and spaces. In a few other places, too.

+  /* No gio backend give us this file, so if someday it works, please
+   * fix it. */
+  g_assert (other_file == NULL);

Why? Apparently we don't need it to work properly. So just ignore it instead of breaking when some backends decide to pass it along.

+static void gnome_wp_xml_add_monitor (GFile *directory,
...
+    path = g_file_get_path (directory);
+    g_warning ("Unable to monitor directory %s: %s",
+               path, error->message);

You should use g_file_get_parse_name when showing file names to the user instead of _get_path

+static void gnome_wp_xml_load_from_dir (const gchar *path,
...
+  directory = g_file_new_for_path (path);
+  info = g_file_query_info (directory,
+                            "standard::*",
+                            G_FILE_QUERY_INFO_NONE,
+                            NULL,
+                            &error);

Just use g_file_test here. It's much easier, you have a path already, and the XML file is a local file, anyway.

+  while ((info = g_file_enumerator_next_file (enumerator, NULL, &error))) {
+    const gchar *filename;
+    gchar *fullpath;
+
+    if (error != NULL) {
+      /* I can't get the file name if info == NULL */
+      g_warning ("Unable to load a file: %s", error->message);

If info is NULL, you won't even enter the loop, so checking for error here is unnecessary. You leak that error after the loop, though.

Created attachment 109340 [details] [review]
gnome-wp-xml correction

Hi Jens,

I've fixed all points you shown me in your last comment. But about errors, I got only one situation that it's result is no important (when calling g_file_enumerator_close) the others helped me find bugs and debug problems. Can you see other unuseful error handling in this new patch?

ps.: I saw some tabs in the last patch, but actually, the current code has some tabs mixed with spaces. All new code I'm writting I'm removing tabs and replacing by spaces.

> I've fixed all points you shown me in your last comment. But about errors, I
> got only one situation that it's result is no important (when calling
> g_file_enumerator_close) the others helped me find bugs and debug problems.

Well, they sure are useful during development, just sometimes not so much once the functionality is stable.

> Can you see other unuseful error handling in this new patch?

No, looks fine now.

> ps.: I saw some tabs in the last patch, but actually, the current code has some
> tabs mixed with spaces. All new code I'm writting I'm removing tabs and
> replacing by spaces.

Ok, great.

Just a few small things left, then this will be ready for prime-time.

+static void gnome_wp_xml_add_monitor (GFile *directory,
...
+  g_signal_connect (G_OBJECT (monitor), "changed",

The cast is unnecessary.

+static void gnome_wp_xml_load_from_dir (const gchar *path,
+                                        AppearanceData *data) {
...
+  directory = g_file_new_for_path (path);
+
+  if (!g_file_test (path, G_FILE_TEST_IS_DIR)) {
+    g_warning ("Skipping path %s, its not a directory", path);
+    g_object_unref (directory);
+    return;
+  }

If you only created the directory after checking it's fine, you'd not have to clean up.

How can I add a new patch to fix points above? I tried to add a new attachement but the patch is not in obsoletes list. Should I add a new patch forgetting the patch marked as needs work?

ps.: me 0 X 10 bugzilla =)

just add a new patch here and add a comment that it obsoletes the former one, anybody else can set the status if it does not work for you.

Created attachment 109391 [details] [review]
little improvments to gnome-wp-xml.c following Jens points

This patch makes gnome-wp-xml correction obsolete.

Thanks Andre =)

Thanks.

2008-04-17  Jens Granseuer  <jensgr@gmx.net>
        
        Patch by: Lincoln de Sousa <lincoln@minaslivre.org>
        
        * gnome-wp-xml.c (gnome_wp_file_changed), (gnome_wp_xml_add_monitor),
        (gnome_wp_xml_load_from_dir), (gnome_wp_xml_load_list): replace gnome-vfs by
        gio (part of bug #524401)

I have committed the other patch, too, and ported a few more parts myself. common/gnome-theme-info is also done.

Created attachment 110522 [details] [review]
theme-save.c port to gio

As in some other places, I used glib stuff mixed with gio instead of using pure gio layer. I really don't know if this is the better idea. But currently the program uses only local resources and receives and sends too many paths instead of using uris.

At this moment I'm working on the theme-installer.c file. I think this is the last file with gnome_vfs references (appearance-main.c has only gnome_vfs_init =). Currently it in that file, remains about 14 references.

Please use the -p parameter when generating diffs. That makes reviews a lot easier.

   dir = g_build_filename (g_get_home_dir (), ".themes", theme_name_dir, "index.theme", NULL);
   g_free (theme_name_dir);
-  uri = gnome_vfs_uri_new (dir);
-  g_free (dir);
-
-  if (gnome_vfs_uri_exists (uri)) {
+  if (g_file_test (dir, G_FILE_TEST_EXISTS)) {
     GtkDialog *dialog;

Seems like you're leaking dir here.

Otherwise looks pretty good.

Created attachment 110537 [details] [review]
theme-save.c patch with little fixes

Thanks! (still missing the -p, though...)

Created attachment 110925 [details] [review]
theme-installer.c and appearance-main.c port

+cleanup_tmp_dir (GIOSchedulerJob *job,
+		 GCancellable *cancellable,
+		 gpointer user_data)

theme-util.c already has code to recursively delete a directory.

@@ -76,14 +106,31 @@ file_theme_type (const gchar *dir)
...
+		while (TRUE) {
+                        GError *error = NULL;
+
+			file_contents = g_realloc (file_contents, total_file_size + CHUNK_SIZE);

You may want to take a look at g_file_get_contents.

@@ -307,13 +358,25 @@ gnome_theme_install_real (gint filetype,

+			if (!g_file_move (src_file, new_file, G_FILE_COPY_NONE,
+					  NULL, NULL, NULL, NULL)) {
+				/* is this noise enough? */
+				g_warning ("Error while coping file");
+			}

You're not trying to copy a file. Plus, you should at least mention the source and destination.

@@ -326,17 +389,12 @@ gnome_theme_install_real (gint filetype,

+ 	if (!g_file_move (theme_source_dir, theme_dest_dir,
+			  G_FILE_COPY_OVERWRITE, NULL, NULL,
+			  NULL, NULL)) {
 		gchar *str;
 
 		str = g_strdup_printf (_("Installation for theme \"%s\" failed."), theme_name);

In this case, we could use the actual error message from g_file_move and show it as secondary text in the error dialog.

@@ -495,13 +553,21 @@ transfer_done_cb (GtkWidget *dlg, gchar 
-		gnome_vfs_unlink (path);
+		{
+			GFile *todelete;
+
+			todelete = g_file_new_for_path (path);
+			g_file_delete (todelete, NULL, NULL);
+			g_object_unref (todelete);
+		}

Please don't add sub-blocks without need.

Created attachment 111370 [details] [review]
new try to theme-installer and appearance-main

ps.: gnome-wp-info.c was included in this patch to allow compile appearance-properties with -Wall.

-static gboolean
+gboolean
 directory_delete_recursive (GFile *directory, GError **error)

I think it would be better to leave this private and make file_delete_recursive public since that's the generic solution.

+/* user_data should receive a (gchar *) and it can't be freed after calling
+ * g_io_scheduler_push_job, because it will be executed in a new thread or
+ * when main loop is idle. */

g_io_scheduler_push_job has a GDestroyNotify parameter which you can (and should) use to free the user_data instead of relying on the called function. It helps prevent leaks like the one you have in there. ;-)

Okay!

I'm going to fix it right now! =)

But about your first tip, I saw other functions in theme-util.[ch] using the `theme_' namespace protection, should I rename file_delete_recursive to theme_file_delete_recursive too?

Created attachment 111460 [details] [review]
Last gnome_vfs to gio port patch with leak and api corrections

theme_dir was not being free and now g_io_scheduler_push_job receives a copy of tmp_dir and a g_free in the GDestroyNotify param.

Hi Jens,

Is there anything else waiting to be ported to gio in control-center? Are there any news about themes:// or fonts://

If something is missing I would like to help =)

I don't think we want to have fonts:// ever returning. People should use gnome-specimen instead.

themes:// is already gone. The code's still in svn, but it's not distributed or built anymore. I still want to move the theme thumbnailer over to capplets/common as a simple example and test app, but after that it will be dropped.

With regards to fonts://, I basically agree with Andre, although afaik specimen doesn't support font installation. I believe it would be better to add this capability to an app like gnome-specimen than to port the fonts method, though. So, I guess that will go away as well.

You're welcome to move over the theme thumbnailer if you like. I'm not going to find the time for that any time soon.

I also agree with dropping fonts:, so should we do that now?

Go ahead. We should remove that completely, though, instead of just taking it out of the build.

Ok, both themus and fontilus removed from SVN, so I guess this can be closed now

updated http://live.gnome.org/GioPort