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 524401 - Gio Port
Gio Port
Status: RESOLVED FIXED
Product: gnome-control-center
Classification: Core
Component: general
git master
Other Linux
: Normal enhancement
: ---
Assigned To: Control-Center Maintainers
Control-Center Maintainers
Depends on:
Blocks:
 
 
Reported: 2008-03-25 23:05 UTC by Lincoln de Sousa
Modified: 2008-07-18 09:47 UTC
See Also:
GNOME target: ---
GNOME version: 2.21/2.22


Attachments
porting appearance-desktop.c to gio (3.00 KB, patch)
2008-04-08 05:36 UTC, Lincoln de Sousa
needs-work Details | Review
New try to port appearance-desktop.c to gio (3.12 KB, patch)
2008-04-08 17:58 UTC, Lincoln de Sousa
reviewed Details | Review
little improvement (3.12 KB, patch)
2008-04-11 02:48 UTC, Lincoln de Sousa
committed Details | Review
gnome-wp-xml migration (6.22 KB, patch)
2008-04-13 19:48 UTC, Lincoln de Sousa
none Details | Review
new try of gnome-wp-xml port =/ (7.00 KB, patch)
2008-04-13 20:02 UTC, Lincoln de Sousa
needs-work Details | Review
gnome-wp-xml correction (6.07 KB, patch)
2008-04-15 22:20 UTC, Lincoln de Sousa
none Details | Review
little improvments to gnome-wp-xml.c following Jens points (6.03 KB, patch)
2008-04-16 22:29 UTC, Lincoln de Sousa
committed Details | Review
theme-save.c port to gio (4.19 KB, patch)
2008-05-07 13:41 UTC, Lincoln de Sousa
needs-work Details | Review
theme-save.c patch with little fixes (4.25 KB, patch)
2008-05-07 19:28 UTC, Lincoln de Sousa
committed Details | Review
theme-installer.c and appearance-main.c port (7.22 KB, patch)
2008-05-14 19:15 UTC, Lincoln de Sousa
needs-work Details | Review
new try to theme-installer and appearance-main (8.38 KB, patch)
2008-05-22 22:49 UTC, Lincoln de Sousa
needs-work Details | Review
Last gnome_vfs to gio port patch with leak and api corrections (7.94 KB, patch)
2008-05-24 15:33 UTC, Lincoln de Sousa
committed Details | Review

Description Lincoln de Sousa 2008-03-25 23:05:54 UTC
I found gnome-control-center in a list[1] waiting to be ported to gio (instead of using gnome-vfs) and started to write a patch to port it. I started working in appearance but I saw that a big part of theme-manager and vfs-method uses GnomeVFS too.

Another problem I found, is that appearance screen is not installing remote themes as done in http://art.gnome.org for example. I think my patch will solve this problem too.

[1] http://live.gnome.org/GioPort
Comment 1 Jens Granseuer 2008-04-06 13:45:57 UTC
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.
Comment 2 Lincoln de Sousa 2008-04-08 05:35:39 UTC
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!
Comment 3 Lincoln de Sousa 2008-04-08 05:36:52 UTC
Created attachment 108839 [details] [review]
porting appearance-desktop.c to gio
Comment 4 Jens Granseuer 2008-04-08 16:04:28 UTC
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.
Comment 5 Lincoln de Sousa 2008-04-08 17:58:44 UTC
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?
Comment 6 Jens Granseuer 2008-04-08 18:21:53 UTC
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.
Comment 7 Lincoln de Sousa 2008-04-11 02:48:03 UTC
Created attachment 109036 [details] [review]
little improvement

Moving g_object_unref (file) to directly below g_file_query_info.
Comment 8 André Klapper 2008-04-11 12:46:23 UTC
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
Comment 9 Lincoln de Sousa 2008-04-11 13:08:51 UTC
(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!
Comment 10 André Klapper 2008-04-11 13:43:21 UTC
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.
Comment 11 Jens Granseuer 2008-04-11 16:10:00 UTC
(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.
Comment 12 André Klapper 2008-04-13 17:34:16 UTC
(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. :)
Comment 13 Lincoln de Sousa 2008-04-13 19:47:50 UTC
(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 =)
Comment 14 Lincoln de Sousa 2008-04-13 19:48:44 UTC
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
Comment 15 Lincoln de Sousa 2008-04-13 20:02:09 UTC
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 =/
Comment 16 Jens Granseuer 2008-04-13 20:48:38 UTC
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.
Comment 17 Lincoln de Sousa 2008-04-15 22:20:45 UTC
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.
Comment 18 Jens Granseuer 2008-04-16 16:57:01 UTC
> 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.
Comment 19 Lincoln de Sousa 2008-04-16 22:21:52 UTC
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 =)
Comment 20 André Klapper 2008-04-16 22:24:50 UTC
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.
Comment 21 Lincoln de Sousa 2008-04-16 22:29:05 UTC
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 =)
Comment 22 Jens Granseuer 2008-04-17 16:33:39 UTC
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)
Comment 23 Jens Granseuer 2008-05-01 14:13:24 UTC
I have committed the other patch, too, and ported a few more parts myself. common/gnome-theme-info is also done.
Comment 24 Lincoln de Sousa 2008-05-07 13:41:49 UTC
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.
Comment 25 Lincoln de Sousa 2008-05-07 14:19:33 UTC
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.
Comment 26 Jens Granseuer 2008-05-07 18:12:25 UTC
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.
Comment 27 Lincoln de Sousa 2008-05-07 19:28:11 UTC
Created attachment 110537 [details] [review]
theme-save.c patch with little fixes
Comment 28 Jens Granseuer 2008-05-07 19:55:43 UTC
Thanks! (still missing the -p, though...)
Comment 29 Lincoln de Sousa 2008-05-14 19:15:44 UTC
Created attachment 110925 [details] [review]
theme-installer.c and appearance-main.c port
Comment 30 Jens Granseuer 2008-05-15 18:19:28 UTC
+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.
Comment 31 Lincoln de Sousa 2008-05-22 22:49:07 UTC
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.
Comment 32 Jens Granseuer 2008-05-24 12:56:18 UTC
-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. ;-)
Comment 33 Lincoln de Sousa 2008-05-24 15:05:20 UTC
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?
Comment 34 Lincoln de Sousa 2008-05-24 15:33:36 UTC
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.
Comment 35 Lincoln de Sousa 2008-07-11 22:54:34 UTC
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 =)
Comment 36 André Klapper 2008-07-13 10:00:30 UTC
I don't think we want to have fonts:// ever returning. People should use gnome-specimen instead.
Comment 37 Jens Granseuer 2008-07-14 17:56:05 UTC
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.
Comment 38 Rodrigo Moya 2008-07-17 11:02:25 UTC
I also agree with dropping fonts:, so should we do that now?
Comment 39 Jens Granseuer 2008-07-17 17:36:56 UTC
Go ahead. We should remove that completely, though, instead of just taking it out of the build.
Comment 40 Rodrigo Moya 2008-07-18 08:55:12 UTC
Ok, both themus and fontilus removed from SVN, so I guess this can be closed now
Comment 41 André Klapper 2008-07-18 09:47:00 UTC
updated http://live.gnome.org/GioPort