Created attachment 108393 [details] [review]
Current port state

This is just the current patch, i have no hope to this be useful, anyway here is the patch.

Created attachment 108452 [details] [review]
Patch update. src/rotation-utils.c ported to GIO

Updating the patch, while this report is not confirmed, i will post the patches here, to keep updated.
New ChangeLog entry:
2008-04-02  Gabriel Falcão <gabriel@nacaolivre.org>

	* src/rotation-utils.c: g_file_query_info and
	g_file_set_attributes_from_info used,
	instead of gnome_vfs_get_file_info and gnome_vfs_set_file_info.
	It removes the GnomeVFS dependency from thid file.

Hi Gabriel, 

gThumb definitely needs to be ported to gio/gvfs, so thanks for taking a look at it!

I'm still learning gio/gvfs myself, so I'm not am expert on it. However, here are a few comments based on reading on your most recent patch:

1) Something is wrong with your tabs/spacing. You've only added one new line to configure.in, but look how many +/- lines are in the diff file. Please tidy that up. 

2) You might as well add a "$GIO_REQUIRED" variable, for consistency with the rest of configure.in.

3) Remove your "g_print" debugging lines.

4) Follow the existing code style. Use

if (foo) {
        bar;
}

not

if (foo)
{
  bar;
}

That means 8-space or 1-tab indent, not two spaces. Use the bracketing style shown above. This is very important for readability. You'll need to change your editor's settings.

5) I don't think we need GCancellable *cancel. We don't use the cancel feature, so you can just supply NULL to g_file_query_info.


- Mike

Created attachment 108508 [details] [review]
Fixing

Hi Michael,
i was fixed the typos that you said.
Smething really weird was happened to my vim (or with me xD), but now i will follow the gthumb code guidelines by a very strict way.
Should i keep putting all the modifications that i made as a patch here on bugzilla or sending to another place ?

Thanks about your patience and time, i am very excited and proud to contribute with gthumb.

Some more comments:

1) Your Changelog entries are not up to date - they do not include all affected files.

2) Formatting issue:

+        GFile *file_gfile;
+        GFileInfo *info;
+        GError *newerror = NULL;

should be aligned like

+        GFile     *file_gfile;
+        GFileInfo *info;
+        GError    *newerror = NULL;

3) I would shorten "file_gfile" to just "gfile", personally.

4) I wonder if the GFile should be made a part of the FileData struct, so that we only call g_file_new_for_uri from file_data_new. What do you think?

- Mike

Also: Yes, all patches should be attached to this bug report.

- Mike

Hi Michael,

thanks for the new issues, i will take a look at it.
About the file_gfile, i agree to it.

I am currently working on some parts of libgthumb (file-utils and file-data, to be more specific), and i've added GFile as part of PathListData.
I haven't saw all use of FileData, but i think that a GFile will be welcome.
Because i'm working on something that needs a lot of care, my next patch maybe take a little late.
In next patch i hope that i can fill all issues about formating, code style and so.

Thanks a lot!

One thing that should be ported as a priority is the trash handling, since that causes an actual regression. Nautilus now uses the trash spec location (~/.local/share/Trash, whereas gthumb still uses the pre-trash-spec gnome-vfs location 
of ~/.Trash). This means that images that are trashed in gthumb don't show up in the trash window of nautilus.

Thankfully, gio has a g_file_trash() function that should make this pretty easy.

Downstream bug about this issue: 
https://bugzilla.redhat.com/show_bug.cgi?id=445125

Gabriel,

I've committed part of your patch, mostly the gnome_vfs_format_file_size_for_display -> g_format_size_for_display stuff.

Numerous variables also had to be changed from GnomeVFSFileSize -> goffset.

Are you doing further porting? If so, please follow the code used in file-roller, which is Paolo's other big project. It has already been ported.

- Mike

Created attachment 111031 [details] [review]
GFileInfo and GAppInfo related ports

Mike,
as you asked me by mail, here follows the port of src/dlg-image-prop.c, src/dlg-open-with.c and resolving the problem at src/gth-viewer.c that caused a reversion in revision [2331].

Please check the ChangeLog for more details.

Regards,
Gabriel Falcão

The dlg-image-prop.c stuff has been committed and copied to gth-exif-data-viewer.c as well.

I'm still reviewing the other bits.

- Mike

Created attachment 111170 [details] [review]
sub-patch for open-width functions

OK, focusing on the open-with parts of the patch next (stripped-down patch attached), I get these warnings:

(gthumb:18372): GLib-GObject-WARNING **: IA__g_object_new_valist: object class `GThemedIcon' has no property named `names'

(gthumb:18372): GLib-GIO-CRITICAL **: g_themed_icon_constructed: assertion `themed->names != NULL && themed->names[0] != NULL' failed

Can that be fixed, Gabriel?

- Mike

Hi Mike,

it seems to be a problem with GIO, i am using the version 2.16.3 packaged in Debian sid, but checking the svn log of Glib/GIO i saw a fix for that problem:

http://svn.gnome.org/viewvc/glib?view=revision&revision=6666

i did test the gio at revision 6666 with mine last gthumb patch and it works great. Maybe i am doing something wrong :(

 - Gabriel Falcão

OK, the "open with" stuff has been committed:

http://svn.gnome.org/viewvc/gthumb?rev=2335&view=rev

I'll check the owner/permission copying bits next. Why did you include size in those gfileinfo calls?

- Mike

> (gthumb:18372): GLib-GObject-WARNING **: IA__g_object_new_valist: object class
> `GThemedIcon' has no property named `names'

That's bug 537555.

any news here?

Hmm, I've forgotten to look at the g_file_query_info patch bits. I'll look at them and get them committed.

However, the hard parts of the gio migration (the file xfer stuff) are basically stalled... Gabriel, are you looking at it? If not, Paolo will need to when he has free time. It's beyond my skill level.


- Mike

This month i am looking at other projects, actually i need to close a bunch of unterminated projects, but this Gthumb port is in my list, and in the first week of August i will be attending to debian camp, and the Gthumb GIO port have the highest priority.

The xfer stuff is beyond my skill level too, but have "almost simple" in which i want to work on.

Created attachment 115980 [details] [review]
More ports

Porting the file_rename and xfer_file functions (libgthumb/file-utils.[ch]) to GIO, and porting all the stuff which use the file_rename to the new function signature.
Migrating all CopyDoneFunc

Gabriel,

Great to see progress!

Could you merge the non-committed bits of patch 111031 with the full 115980 patch, so that I can apply & test cleanly against svn rev 2381 (the latest)?

Some parts of patch 111031 were not committed last time, because I didn't see the point of loading "standard::size" in the g_file_info calls. Please remove that.

Thanks!

- Mike

Gabriel, Mike,

That is good news, indeed!

On my side, I started to GFile-ify catalog-web-exporter.[ch].

Replacing paths and uris with GFile wherever it makes sense greatly simplifies the code.

With the great work done on xfer functions by Gabriel, the Web exporter will be soon very gio friendly!

I am almost done. I will post a patch when all the pending patches that Mike refers to are committed.

Note that I had to import a few functions from file-roller:file-utils.c.

Bests,

Christophe

OK, I merged the pending patches and committed them with minor corrections. Thanks Gabriel!

The Trash handling still needs to be fixed.

- Mike

Created attachment 116102 [details] [review]
GIO port -- use GFile in web exporter

Mike, Gabriel,

Here is a first step toward the GFile-ification of the web exporter. This patch is limited to catalog-web-exporter.[ch].

In the context of the GIO/GVFS port, I think it would be great to use GFile as our preferred data structure for representing file paths/uris, converting to char* only when it is necessary (e.g. for display). This will certainly simplify the code.

The transition might be quite difficult, though, since char* representations for file paths/uris are spread everywhere in the code. A possible strategy could be to implement (or borrow form other projects like file-roller) GFile versions of the main files/directories functions (starting from file-utils.c), plus a wrapper matching the current api (that is, using char* as parameters). That way, it would be possible to migrate progressively to the GFile-api, and get rid of the char* wrappers at the end of the process.

What do you think?

Best Regards,

Christophe

Christophe,

Yes, that's sort of what I was suggesting in comment 5 - put the gfile in the FileData struct, and start using it as the main file identifier, rather than path/uri. For a while we would need both the gfile and path/uri in FileData, but later the path/uri would be removed.

Hopefully that would eliminate a lot of the annoying escaping / encoding / charset issues that constantly pop up.

I don't think it would be that hard to do, but it would take a while.

So if you feel ambitious, please go ahead!

I'll check your patch later today, hopefully.

- Mike

Working on file copy_file_async (done!), dlg_files_move_to_trash (done!) and dlg_folder_move_to_trash (working on).
I am making some tests with g_io_scheduler_push_job, and i'm having some sucessfulness.
Trash stuff almost done.

Where are these stuff:
libgthumb/file-utils.[ch]
src/dlg-file-utils.[ch]

(In reply to comment #25)
> Working on file copy_file_async (done!), dlg_files_move_to_trash (done!) and
> dlg_folder_move_to_trash (working on).
> I am making some tests with g_io_scheduler_push_job, and i'm having some
> sucessfulness.
> Trash stuff almost done.

Yay!


> Where are these stuff:
> libgthumb/file-utils.[ch]
> src/dlg-file-utils.[ch]

Huh? What do you mean?

- Mike

Christophe,

Why do you have this in your patch...

static char *
file_get_uri (GFile *file)
{
        return g_file_get_uri (file);
}

Shouldn't you just call g_file_get_uri directly?

- Mike

(In reply to comment #26)
> > Where are these stuff:
> > libgthumb/file-utils.[ch]
> > src/dlg-file-utils.[ch]
> 
> Huh? What do you mean?
> 
> - Mike
> 
Sorry, it is because my poor english :(
I was tryng to say that the functions which i am working on, are located in these files.

I keep working in the trash stuff, my tests are almost done, i am just dealing with asyncronous I/O stuff...

(In reply to comment #27)
> Christophe,
> 
> Why do you have this in your patch...
> 
> static char *
> file_get_uri (GFile *file)
> {
>         return g_file_get_uri (file);
> }
> 
> Shouldn't you just call g_file_get_uri directly?
> 
> - Mike
> 

Mike, 

I wrote a set of functions in catalog-web-exporter, organized in two layers: 

1) functions returning an object whose type is our new "internal" representation of a file name (that is, a GFile)

2) functions converting from this internal representation to various (char*-based) "external" formats (for display or other purposes). These formats are paths and uris, relative or absolute.

I think it is better for general readability and maintenability that a piece of code that needs to perform one of these "generic" tasks (i.e. creating an instance of our internal representation, or tranforming into an external representation) explicitly calls a function in the proper layer. It gives use better control over the callers (who knows, maybe one day we will figure out that we have much more to do into file_get_uri than just a g_file_get_uri), and it facilitates debugging.

So this "file_get_uri" is just a side effect of my programming style: I do like layers that callers are "forced" to hook into.

But of course feel free to edit!

Bests,

Christophe

Created attachment 116212 [details] [review]
Fix png exporter (creation of the image map html file)

rev 2382 broke the png exporter. This is a quick fix.

Christophe

(In reply to comment #29)

> 2) functions converting from this internal representation to various
> (char*-based) "external" formats (for display or other purposes). These formats are paths and uris, relative or absolute.

Note that these four functions may move to file-utils.c and become mandatory for GFile->char* conversions.

The UNREF macro may be moved too.

Another possibility is to create a new gfile-utils.[ch] where we would progressively move gio-ported file-utils.[ch] functions, leaving only path/uri wrappers into file-utils.[ch].

Getting rid of file-utils.[ch] would be one goal of the gio port.

Christophe

Christophe,

OK, I committed your two patches (rev 2388).

Creating gfile-utils.[ch] is probably a good idea. Gabriel, can you move your updated functions from file-utils.c into gfile-utils.c?

I have created empty gfile-utils.[ch] files (rev 2389).

- Mike

Thanks Mike.

May I kindly suggest that we give a special prefix to public functions in gfile-utils.[ch] such that it becomes obvious if a caller has been ported or not?

For instance, for a function 

  foo (char *)

in file-utils, we could use 

  gfile_foo (GFile *) 

in gfile-utils and keep 

  foo (char *) 

as a small wrapper in file-utils.

At the end of the process, we may get rid of the gfile_ prefixes, or keep them. But in the meantime it could prove useful.

My 2¢!

Christophe

Sure, gfile_foo sounds good.

It would of course be nice to completely replace foo with gfile_foo where possible, rather than keeping wrapper functions around... but I suppose things could be messy for a while :-)

- Mike

Created attachment 116239 [details] [review]
Example gio & wrapper: path_is_dir, path_is_file

Mike,

Here is an example of this "gio + wrapper" approach, applied to path_is_dir & path_is_file. The gio part is based on a function found in file-roller. I just disentangled the GFile* from the char* interface.

Note that this patch did not convert any caller. I think we should first make sure that a wrapping does not introduce new bugs by itself before converting the callers.

Bests,

Christophe

Created attachment 116262 [details] [review]
gio & wrapper (path_is_*, ensure_dir_exists)

More: ensure_dir_exists (with caller catalog-web-exporter.c converted).

Chr.

Mike, Chris,
i still working on trash stuff.
Actually, i spent lot of time writting stuff to test and learn how to use g_io_scheduler stuff.
Since Gthumb are very stuck in the rough way that GnomeVFS does async stuff, i decided to write a functions for the libgthumb/file-utils that have the same (or almost at least), behavior and interface than gnome_vfs_async_*.
For now, i wrote the async_xfer_file_gio, and am migrating stuff like xfer_file and so on to use it.

Nevertheless, i was using GIT to control my modifications, but since the last time i tried to make a git pull (unsuccessfully) i had to go back to svn, and it is making me do the things without the organization i wanted to.

But as i said, i still working on it, and hopefully today i will send a patch with lots of modifications.

PS.: I am having so much dificulties to deal with the gthumb name conventions, because sometimes a function have a parameter like char *source_uri, but actually it receives a path, and sometimes the inverse happens, so i am using the g_file_new_for_commandline_arg to create gfiles instead of specific g_file_new_for_uri or g_file_new_for_path, since g_file_new_for_commandline_arg recognizes the which kind of parameters is dealing with.

(In reply to comment #37)

Gabriel,

On my side, I am still working on the gfile/char* split (see comment #33).

path_is_file, path_is_dir, and ensure_dir_exists are on my pending patch 116262.

I am currently converting get_destination_free_space () and get_temp_dir_name ().

To avoid the path/uri mess we had in file-util.c, I am enforcing the following policy: all the functions in gfile-util.c only accept/return uri-style GFiles.

Your get_file_size () is a good candidate for a split. Do you want me to do it as part of the next version of my patch? (the interface in file-util.c will not change, so it should be transparent for you).

Bests,

Christophe

Created attachment 116277 [details] [review]
gio wrapper, last version

Adding:
  get_destination_free_space () 
  get_temp_dir_name ()

and eating my own dog food! (in catalog-web-exporter.c)

Note: I am enforcing the following policy: all the functions in gfile-util.c only accept/return uri-style GFiles.

Bests,

Christophe

Thanks for explaining Chris :)
I would like if you split the get_file_size, i was testing he exactly now...
I am up to convert all the gnome_vfs_async_xfer, to the mine async_xfer_file_gio, but when moving a file from my desktop to gthumb window, i got a segfault :(
And it is something related to getting inexistent attributes of GFiles, anyway, ot should not happen. I think that my version of Glib/GIO is with bugs...

And oh... you are doing a great job! Congrats!

Created attachment 116278 [details] [review]
gio wrapper, last version

Adding:
  dir_remove_recursive ()
  get_file_size ()

Bests,

Christophe

(In reply to comment #40)
> Thanks for explaining Chris :)
> I would like if you split the get_file_size, i was testing he exactly now...
> I am up to convert all the gnome_vfs_async_xfer, to the mine
> async_xfer_file_gio, but when moving a file from my desktop to gthumb window, i
> got a segfault :(
> And it is something related to getting inexistent attributes of GFiles, anyway,
> ot should not happen. I think that my version of Glib/GIO is with bugs...
> 
> And oh... you are doing a great job! Congrats!
> 

Thank you Gabriel.

Compared to what you are dealing with, cleaning up file-utils / gfile-utils is piece of cake!

BTW, I converted your get_file_size ().

Bests,

Christophe

Great stuff, guys!

The gfile code is so much cleaner than the old char*-based stuff.

The wrapper approach looks nice too.

Christophe's patch has been committed to rev 2391.

- Mike

correcting patch status as per last comment.

Created attachment 116326 [details] [review]
More GIO work on gfile-utils.c & catalog-web.exporter.c

Mike, Gabriel,

This patch contains:
- bug fixes
- more work on the Web exporter
- more work on gfile-utils.c
  in particular: introduce two constructors enforcing the uri-only policy: 
  gfile_new (path) and gfile_new_va (paths1, path2, ..., NULL).
  Please use them!

Using these constructors simplifies a bit the xfer functions written by Gabriel in file-utils.c

Bests,

Christophe

Created attachment 116340 [details] [review]
More GIO work on gfile-utils.c & catalog-web.exporter.c (v2)

Better patch:

- kill more gnome_vfs stuffs in catalog-web-exporter.c
- split Gabriel's file_copy, file_move, file_rename 
  (side note: I dropped the third parameter of file-utils.c:file_name since nobody uses it. We could reintroduce it latter in a gfile-utils.c:file_name if needed)

The split is currently clean (but far form complete, of course): GFiles in file-utils.c are only used in wrappers.

Bests,

Christophe

Christophe, I committed v1 of your patch just before v2 appeared. Can you re-submit v2 against current trunk? Thanks!

The new gfile-creation code is very elegant.

- Mike

Created attachment 116341 [details] [review]
More GIO work on gfile-utils.c & catalog-web.exporter.c (against trunk) 

Sure. --Chr.

Christophe,

You probably didn't want a pointer here:

gboolean   *copy_done;

catalog-web-exporter.c: In function 'export__copy_image':
catalog-web-exporter.c:2664: warning: assignment makes pointer from integer without a cast

- Mike

Oops, indeed! --Chr.

Committed to 2393, with above correction.

- Mike

Created attachment 116374 [details] [review]
Replace gnome_vfs_xfer_uri with file_copy

Low hanging fruit!

Bests,

Chr.

Ooh, I like patches with many more "-" signs than "+" signs!

Committed, rev 2394.

- Mike

Created attachment 116457 [details] [review]
implemented the async_xfer_file_gio, to be used as replacement for gnome_vfs_async_xfer... stuff

This function has been tested in a rough way, but works pretty.
I tried to migrate the xfer_file function from src/dlg-file-utils.c, but i got infinites segfaults, so i decided to send this patch and rewrite the xfer_file function.

Gabriel,

After a quick review, I see one major problem: the trash implementation is too simple. user_data_dir/Trash is not the only possible trash location. There can be "top-level" trash dirs too.

We really need to use g_file_trash, even though it isn't a nice fit for the existing code. It will figure out the correct trash location for us.

Also, the new xfer code should probably go in gfile-utils.c, not file-utils.c. 

Christophe, can you comment as well?

- Mike

Hi Gabriel,

I agree with Mike. As stated in

  http://library.gnome.org/devel/gio/unstable/ch15.html

g_file_trash () is the way to go.

With respect to your xfer code, I can't wait using it! 

But before, I would be glad if you could sync it with the gfile-utils / file-utils split. 

For instance, your source_list, destination_list could be GList's of GFile's, in order to avoid using g_file_new_for_commandline_arg () in _async_xfer_job_do (). That way you will be able to move char*-free code to gfile-utils, and leave only a small wrapper into file-utils (use gfile_ as prefix for public functions).

You could use the already migrated code as reference examples.

Also, please uses the GFile constructors gfile_new () and gfile_new_va () when you need to create a GFile from a char* path.

Hey, we are getting closer!

Christophe

Created attachment 116489 [details] [review]
Improved async copy and move

Chris, you have made a awesome work with that gfile-utils features!
That thing have just avoided me to get segfaults! :D

I hope this patch be useful :)
Mike, i haven't maked the trash stuff as well because my brain got insanely in lust to resolve the asyncronous I/O stuff, now i can have a nice work.
Today i will work on trash stuff.

Thanks a lot, guys, i am learning so much things !

Gabriel,

Thanks for the work!

However, the patch seems to be malformed - it is missing async_xfer_file_gio from gfile-utils.c and other code. Please check it and re-submit.

- Mike

Created attachment 116516 [details] [review]
Improved async copy and move [fixed]

Sorry Mike, now i am sending the full patch. My bad :(

Thanks Gabriel!

Please fix the indentation/bracketing style to follow gthumb conventions (see comment 3 for a reminder).

- Mike

Created attachment 116545 [details] [review]
Improved async copy and move (fixed indentation)

I think now the indentation is ok.

It still has non-standard waste-spacing bracketing. Change

if (foo == bar)
        {
                do_func (bla);
        }

to

if (foo == bar) {
        do_func (bla);
}

You can use:

astyle --style=linux < infile > outfile

to fix style errors.

Gabriel & Christophe,

I will be away for ~5 days.

Christophe, can you review/test/modify Gabriel's patches and merge them into your patches, if you are working on gthumb over the next week?

- Mike

Created attachment 116576 [details] [review]
Improved async copy and move (fixed indentation)

I did use the astyle.
Fortunely, comparing the new diff i found a little memory leak in my patch, so i just had fixed it.

Hello Gabriel,

Your patch is almost 5000 lines, mainly due to tab/8 spaces conversions on existing code. I am not sure it is a good thing to mix in a single patch formatting and new real content. In particular, the svn history would become a bit messy.

I suspect that Mike's advice about using astyle was more about checking by yourself what kind of reformatting were needed in your patch, and then correct by hand your own code. After astyle --style=linux < infile > outfile, use diff to check for the differences between the two files and update your code accordingly.

Or you may extract your code, apply astyle and then paste back.

Note that I still submit code containing tabs, and by looking at the size of your patch it seems that I am not the only one! Well, we may apply a giant, formatting only, patch latter, and set up our editors to use 8 spaces instead of tabs. But that is a separate issue.

So, if you don't mind sending a new patch limited to your new code, I will be glad to test it!

Bests,

Christophe

Still here for a few more hours...

Yes, the patch should be limited to the new stuff, or it becomes impossible to review.

Tabs and spaces are used interchangeably in gthumb code. Don't worry too much about that - it all looks the same, after all. However, I do care about the bracketing style. 

- Mike

(In reply to comment #63)
> Gabriel & Christophe,
> 
> I will be away for ~5 days.
> 
> Christophe, can you review/test/modify Gabriel's patches and merge them into
> your patches, if you are working on gthumb over the next week?
> 
> - Mike
> 

I will do my best.

Christophe

Created attachment 116771 [details] [review]
GIO port -- mime

Mike,

This patch converts part of the MIME type detection functions.

- I started with a copy of file-utils.c:get_file_mime_type () in file-roller. I figured out it assumed that g_file_info_get_content_type () returns the value of attribute G_FILE_ATTRIBUTE_STANDARD_FAST_CONTENT_TYPE when g_file_query_info () asked for it. A test showed me that it is not the case. Thus, if I did not make a mistake somewhere, I think file-roller's source code should be corrected.
- I tested with sample image files how the exceptions hard-coded in file-utils:get_file_mime_type () (Nikon nef, Canon cr2, OpenEXR exr, ...) were now handled by GIO, and left only what was necessary.

Also:

- private functions in gfile-utils are now all prefixed by _gfile.
- mute a compile warning triggered by gfile_warning ()

Bests,

Christophe

PS: Thanks for your funny (and quite embarrassing) rev 2400!

Created attachment 116831 [details] [review]
convert MIME and use output stream in web exporter

Mike,

This is a new, extended version of my previous patch.

The web exporter now uses output streams.

Note that I had to touch your gfile_output_stream_write_line (). I needed a gfile_output_stream_write () which did not write a "\n", and was able to set its error parameter.

I think the new gfile_output_stream_write_line () is more homogeneous with the stock gio functions: 1) you can pass it an **error or NULL, 2) if error is not NULL, *error will be set if an error occurs, 3) even if you pass NULL, the value returned by the function enables you to determine if an error occurred.

I hope this change is OK for you.

Bests,

Christophe

Created attachment 116930 [details] [review]
Sending files to trash

This patch ports the dlg_files_trash to GIO.
Now i am porting the structure that sends a whole directory to trash.

Thanks Gabriel!

The first step to review a patch is to read it directly, to get a sense of what material is added / deleted. In that respect your patch is very difficult to review because most of it is tabs / spaces conversions.

Please try to work out a patch limited to the new stuffs. Adjust your editor's settings such that it does not convert tabs.

Nonetheless I spotted that you create:

  new_uri_string_from_path ()
  get_trash_path ()
  get_trash_uri ()

in libgthumb/file-utils.c, but it seems that you don't use them (which is OK, see comment #55 and comment #56).

Bests,

Christophe

Created attachment 116939 [details] [review]
MIME & stream

In this new version, the gthtml parser uses GIO.

Run src/make-albumtheme.sh before make.

Bests,

Christophe

Hi Chris,
i really got some indentation problems, and i've realized that when i used that indentation tool of comment #62, the code got many modifications nonsense.
I am very sorry :(
This week i'll stop working on gthumb because i moved to other city/state, new job, etc.
But i have created a function to send whole folders to trash asyncronnously, i hope to send this patch today.

Regards,
Gabriel

mime/stream patch committed to 2402.

Thanks, Christophe!

- Mike

To help things along, I ran astyle on gfile-utils.c in trunk.

Gabriel, please run "svn update" and "svn diff" again to regenerate your patches. They should be much smaller now, since both sides now have the astyle tidy-ups.

- Mike

Hi Gabriel,

Nudge, nudge... do you have a tidied-up patch for us to play with? :-)

- Mike

Created attachment 117658 [details] [review]
trash stuff done

Finished to port the trash stuff.

Created attachment 117721 [details] [review]
Fix g_propogate_warnings assertion when there is no error.

g_propagate_error should only be called if an error has actually occurred, otherwise an assertion gets thrown when debugging with the G_DEBUG=fatal_warnings environment variable.

Gabriel,

The trash patch is impossible to review. As I've said before, please strip the patch down to its essentials, removing the white space changes!

You can do something like checking out another copy of the code from trunk, running astyle on the affected files (without your patches), and run "diff" (rather than "svn diff") against the astyle+patched files, so that only the "real" changes are written to the patch. (There are probably other methods, but that should work fine.)

I'd love to get your code into trunk, but you are making it hard by ignoring the white space problem.

- Mike

g_propagate_error patch applied to svn rev 2407 (after fixing the bracket style). Thanks, Geoffrey!

- Mike

Mike, i am very sorry about that whitespace issues. I am quite noob on it, but i will follow your tip.

Actually i am not ignoring because i want to. :(

I will send a cleaned version today.

Regards,
Gabriel Falcão

Created attachment 118070 [details] [review]
Trash stuff done and stripped

I did this patch using:
svn diff --diff-cmd diff -x -uw
instead of simply svn diff

Thanks, Gabriel, that is much easier to read! Here are a few minor things to fix, based on a first reading:

1)  /* TODO: g_error_free (error); */  ???

2)  In _after_each_file_trashed, does the string "Copying file %d of %d" make sense, or should it be "Deleting file %d of %d"?

3)  "_after_each_fof_trashed" - What's a "fof"? Where is this function used?

4)  g_message ("Aumentando o index e o total"), and others. In English, please.

5)  gfile_trash_async (GList *file_list,
		   GCancellable *cancel,
		   GFileTrashedCb each_file_callback,
		   gpointer each_file_data,
		   gint io_priority,
		   GSourceFunc done_func,
		   gpointer done_data,
		   GError **error)

  Please follow the gthumb coding style, with the variable names aligned in the same column. (Needs fixing in quite a few spots.)


I haven't actually tested the patch.

- Mike

Created attachment 118490 [details] [review]
Trash stuff done, stripped and checked

Thanks Mike, i hope that it is done now...


About your questions...
1)  /* TODO: g_error_free (error); */  ???
>>> it was just a test i forgot to remove...

2)  In _after_each_file_trashed, does the string "Copying file %d of %d" make
sense, or should it be "Deleting file %d of %d"?
>>> of course, but i was actually just wondering that i should not add new strings to be internationalized and the trash stuff from before used exactly that string. Anyway, i changed to "Deleting ...."
3)  "_after_each_fof_trashed" - What's a "fof"? Where is this function used?
>>> Sorry, my bad. I have removed it this time.
4)  g_message ("Aumentando o index e o total"), and others. In English, please.
>>> Sorry, my bad again... Removed!

5)  gfile_trash_async (GList *file_list,
                   GCancellable *cancel,
                   GFileTrashedCb each_file_callback,
                   gpointer each_file_data,
                   gint io_priority,
                   GSourceFunc done_func,
                   gpointer done_data,
                   GError **error)
>>> Done!

Gabriel,

Sorry for the delay in reviewing. I have less free time these days. Anyway, the patch needs some tweaking. I compile everything with the -Wall flags (./autogen.sh --prefix=/usr CFLAGS="-Wall -ggdb") to catch stuff like this:


[mjc@xena trunk]$ make > eraseme
file-utils.c: In function 'copy_file_async_done':
file-utils.c:2409: warning: control reaches end of non-void function
file-utils.c: At top level:
file-utils.c:76: warning: '_empty_file_progress_cb' defined but not used
gfile-utils.c: In function '_gfile_trashed_cb':
gfile-utils.c:262: warning: control reaches end of non-void function
gfile-utils.c: In function '_gfile_trash_recursive':
gfile-utils.c:281: warning: value computed is not used
gfile-utils.c:282: warning: value computed is not used
gfile-utils.c:315: warning: value computed is not used
gfile-utils.c: In function '_gfile_trash_folder_job_do':
gfile-utils.c:349: warning: control reaches end of non-void function
gfile-utils.c: At top level:
gfile-utils.c:112: warning: '_async_xfer_data_ref' defined but not used
gfile-utils.c:336: warning: '_gfile_trash_folder_job_do' defined but not used
dlg-file-utils.c: In function ‘_trash_files_done’:
dlg-file-utils.c:1761: warning: unused variable ‘fcdata’
dlg-file-utils.c: In function ‘dlg_folder_move_to_trash’:
dlg-file-utils.c:2455: warning: unused variable ‘scan’
dlg-write-to-cd.c: In function ‘ok_clicked_cb’:
dlg-write-to-cd.c:108: warning: passing argument 7 of ‘dlg_copy_items’ from incompatible pointer type
dlg-write-to-cd.c:119: warning: passing argument 7 of ‘dlg_copy_items’ from incompatible pointer type
dlg-write-to-cd.c:131: warning: passing argument 7 of ‘dlg_folder_copy’ from incompatible pointer type
gth-browser.c: In function ‘image_list_drag_data_received’:
gth-browser.c:3874: warning: passing argument 7 of ‘dlg_copy_items’ from incompatible pointer type
gth-browser.c: In function ‘dir_list_drag_data_received’:
gth-browser.c:3952: warning: passing argument 7 of ‘dlg_copy_items’ from incompatible pointer type
gth-browser-actions-callbacks.c: In function ‘folder_delete__continue’:
gth-browser-actions-callbacks.c:1153: warning: passing argument 3 of ‘dlg_folder_delete’ from incompatible pointer type
gth-browser-actions-callbacks.c: In function ‘folder_delete’:
gth-browser-actions-callbacks.c:1195: warning: passing argument 3 of ‘dlg_folder_move_to_trash’ from incompatible pointer type
gth-browser-actions-callbacks.c: In function ‘folder_copy__response_cb’:
gth-browser-actions-callbacks.c:1333: warning: passing argument 7 of ‘dlg_folder_copy’ from incompatible pointer type
catalog-web-exporter.c: In function 'export__copy_to_destination__step2':
catalog-web-exporter.c:2151: warning: passing argument 3 of 'dlg_folder_delete' from incompatible pointer type
catalog-web-exporter.c: In function 'export__copy_to_destination':
catalog-web-exporter.c:2169: warning: passing argument 7 of 'dlg_folder_copy' from incompatible pointer type
catalog-web-exporter.c: In function 'load_next_file':
catalog-web-exporter.c:2529: warning: passing argument 3 of 'dlg_folder_delete' from incompatible pointer type


Could you fix these warnings, please?


- Mike

Yeah, i'll do this as soon as possible.

Hi Gabriel,

Could you take a look at that soon?

- Mike

Gabriel, Christophe, mclasen, anybody...

Can someone donate some spare time to get the gio patches rolling again? We've stalled badly...

- Mike

*** Bug 560696 has been marked as a duplicate of this bug. ***

Hi Mike,

Indeed, it's a pity.

Working on the GIO port is still on my todo list. Unfortunately, I have a hard time finding time to work on it. I expect Q1 2009 to be a bit better with that respect, though.

Best regards,

Christophe

OK, maybe someone can give me some gvfs pointers here.

I've added a FileData->local_path entry to point to the ~/.gvfs mount of remote files (and the normal path of local files). This lets us replaces the local_file cache that we had been using, which simplifies a lot of things.

The path is determined in libgthumb/file-utils.c:gfile_get_path_from_uri.

I have a g_assert to abort if the returned path is NULL.

However, the problem I'm having is that to access, say:

sftp://server/foo/bar.jpg

I have to open it first in Nautilus, otherwise if I try to open it in gthumb (from a bookmark, say) this assertion trips.

So I must be missing a "mount" step in gthumb. Is that right?

In other words, for an arbitrary and existing gio uri, how do I ensure that the matching ~/.gvfs file exists?

- Mike

Never mind my last comment - I was looking for g_file_mount_enclosing_volume & g_file_mount_enclosing_volume_finish.

However, I reverted my changes for now - too many things had to be changed (or broken) at once...

- Mike

> In other words, for an arbitrary and existing gio uri, how do I ensure that the
> matching ~/.gvfs file exists?

You can't really assure the FUSE mounted path exists, period. Some users won't run the FUSE daemon, some administrators don't want it running on their systems, etc. If it's there, great, if it's not, you're going to have to deal with that too.

We also (AFAIK) never merged the patch in 530654 for reverse mapping GDaemonFiles to fuse paths, so that's the reason for g_file_get_path() returning NULL.

OK, I've purged a lot of gnomevfs code from trunk lately. Still lots to do :-(

Is there anyone interested who could take a look at:

1) Tidying and testing Gabriel's orphaned patch
2) Porting libgthumb/file-utils.c:resolve_symlinks to use GFile
3) Porting src/gth-monitor.c to use GFileMonitor
4) "grep -i vfs */*.c" and fixing anything they see!

- Mike

Mike,

If Gabriel does not mind, I am going to work on 1). It will help me to get rid of the last pieces of GnomeVFS in catalog_web_exporter.c.

--Christophe

Great, Christophe!

I'll be away from my computer for a week or so...

- Mike

Created attachment 125696 [details] [review]
isolate vfs code in dlg-file-utils.c

Migrating dlg-file-utils.c and file-utils.c is going to be tricky, so we should proceed carefully.

Here is a baby step!

This patch isolates the old VFS code in dlg-file-utils.c, preparing its replacement by GIO code. It generalizes something Gabriel started in his patch.

In particular, the attached patch completely replaces GnomeVFSResult members in struct FolderCopyData and friends by GError members.

Also, the function set_error_from_vfs_result () maps VFS error codes to GIO GErrors. It is meant to be deleted when the port of dlg-file-utils.c is done.

Best Regards and Happy New Year!

Christophe

Hi Christophe!

I'm back now, and I'll review the patch in the next day or two.

- Mike

Patch committed, thanks!

One addition - I deleted:

-#include <libgnomevfs/gnome-vfs-utils.h>

from src/dlg-write-to-cd.c

- Mike

Christophe,

Any further patches? Nudge, nudge...  :-)

- Mike

Mike,

Unfortunately, not yet.

This is not due to a lack of interest for the project. Just a lack of free time...

Keep nudging me from time to time!

8-)

Christophe

Another gio todo: the photo importer should use the gphoto/gio backend end, if present, and then fall back to the existing libgphoto code otherwise.

Otherwise, the gvfs code mounts the camera and breaks the libgphoto-based access.

- Mike

I've put in a whole bunch of commits recently. We are getting closer!

Can someone take a look at gfiling path_list_async_new? (The sync version of it already exists.) It's a nicely isolated bit of code. I'm just not sure that I understand how to cancel async gfile operations properly (and whether the existing gnomevfs code is doing it properly either).

I'll be away for a few days...

Note: To checkout gthumb, you now need git:

git clone git://git.gnome.org/gthumb

- Mike

I'm currently working on this bug.

Created attachment 135783 [details] [review]
GIO conversion of path_list_async_new and associated functions.

GIO conversion of path_list_async_new and associated functions.
Move slow file classify code into an idle callback
Implement gcancellable for path list code
Remove GnomeVfsResult from PathListData
comments/formatting

Created attachment 135784 [details] [review]
GIO root path (Filesystem) string updates

Add a special case to remove_ending_separator for root dir
Change File System elements uri to file:///
Update file:/// string in get_icon_for_uri
Add checks for file:// vs file:/// to update_uri()

The path_list_async_new patch is committed, with minor formatting adjustments, like:

if( foo( bar ))

changes to

if (foo (bar))

Thanks!

- Mike

I've committed the root-path patch, but some things are still broken.

Running "gthumb /" shows the filesystem children, but running "gthumb /home" and then clicking on ".." does not.

- Mike

The gth-location code should make use of g_file_get_parent anyway, once it is migrated, instead of remove_level_from_path.

- Mike

Created attachment 135820 [details] [review]
GIO Fix .. element in location sidebar

That doesn't look right... you have:

g_strdup("file:///");

The duplicated string isn't assigned to anything.

- Mike

Created attachment 135860 [details] [review]
GIO Fix .. element in location sidebar

Oops. Should be assigned to new_path.

Committed!

Created attachment 135887 [details] [review]
GIO src/gth-location.c converstion

update_drives converted
update_uri converted
monitor_changed_cb converted
Unused functions/includes removed

This needs lots of testing with device mounting and unmounting and catalgoue browsing.

You attached the wrong patch.

Created attachment 135889 [details] [review]
GIO src/gth-location.c converstion

Indeed, how careless.

update_drives converted
update_uri converted
monitor_changed_cb converted
Unused functions/includes removed

Some quick comments:

1) Please run "astyle --style=linux src/gth-location.c" before submitting the patch, just to tidy things up a bit.

2) If I mount a floppy, the floppy appears in the volume list, but it has no icon.

3) If I mount a sftp:// uri in Nautilus, I can open with gthumb (from Nautilus), but the sftp:// mount does not show up in the volume list. It should.

I'll have to test it more later, of course.

- Mike

4) If I try to load a bookmark for a location that no longer exists (like a folder that has been deleted, or a remote URI that is no longer mounted), no error is thrown. The directory appears to load. An error dialog should be launched instead, and the directory should not change.

I've committed the above patch, with minor formatting tweaks, to get more experience with it.

Thanks!

- Mike

Created attachment 135919 [details] [review]
GIO Fix error passing on directory load

Replace GnomeVFSResult with GError for dir_list_done_cb, gth_dir_list_change_to
__step2 and directory_load_cb

Created attachment 135920 [details] [review]
GIO Fix error passing on directory load

GIO Fix error passing on directory load
Replace GnomeVFSResult with GError for dir_list_done_cb, gth_dir_list_change_to__step2 and directory_load_cb
Remove unused GnomeVFS includes

Created attachment 135930 [details] [review]
GIO Fixes to allow remote mount browsing

Change path_list_classify_files_cb to use g_file_enumerator_get_next_file_async
Remove local file restriction from update_drives

If you try to load a bookmark for a location that no longer exists, you do not get an error dialog. These warnings occur on the console:

(gthumb:13007): GLib-GObject-CRITICAL **: g_value_get_int: assertion `G_VALUE_HOLDS_INT (value)' failed

I would guess that "gthumb_marshal_VOID__INT" is no longer appropriate in g_signal_new ("done",...).

So the patch in comment 121 needs some fixing...

- Mike

Created attachment 135935 [details] [review]
GIO Fix error passing on directory load

Replace GnomeVFSResult with GError for dir_list_done_cb,
gth_dir_list_change_to__step2 and directory_load_cb
Remove unused GnomeVFS includes

I can't reproduce the problem but I think your right about g_signal_new, please try this.

OK, both committed. One glitch:

1. Mount a remote "sftp://..." location in nautilus.

2. Go to it in gthumb, view some photos, bookmark a location (under sftp://...)

3. Unmount the remote URI in nautilus.

4. Try to load the remote bookmark in gthumb. gthumb freezes.

- Mike

The icon-related stuff in src/main.c should probably be moved in gth-location.c. Also, can we rely more on gfile to provide icons?

- Mike

Created attachment 135948 [details] [review]
GIO Fix bookmarks to unmounted locations

It should be possible to get all our icons through GFile (and GMount/GVolume/GDrive) functions in the future. src/gth-location.c get_mount_icon() already gets an icon for a GMount but it doesn't seem to work for new mounts (e.g. inserting a CD). Possibly because the mount isn't ready yet when its run, I haven't looked into it. 

Maybe you can open a new bug for icon changes?

The icons are probably working better now, after I committed a change to the way gicons were converted into pixbufs.

- Mike

Created attachment 136789 [details] [review]
GIO use ITEMS_PER_NOTIFACTION for g_file_enumerator_next_files_async

(In reply to comment #130)
> Created an attachment (id=136789) [edit]
> GIO use ITEMS_PER_NOTIFACTION for g_file_enumerator_next_files_async
> 

Please ignore this, it's right the way it is, this patch will break things on large directory lists.

Created attachment 136891 [details] [review]
GIO conversion of dlg-duplicates to GFile

Rewrite search_dir_async and search_dir_next_files_cb using GFile
Add search_dir_next_files_cb function
Use GCancellable for canceling the search
Change data->dirs and data->queue to a GFile GList
Add a GFile to ImageData
Convert file reading functions to GFile
Update get_file_mtime and add gfile_get_mtime
Add gfile_is_image_video_or_audio function

Thanks! Committed with some revisions.

- Mike

Created attachment 137151 [details] [review]
GIO dlg-search conversion

Add start_from_gfile to SearchData
Pass name_only to file_respects_search_criteria
Update search_dir_aysnc to use GFiles
Update directory_load_cb to use GFiles
Add scan_next_dir function
Add search_dir_next_files_cb
Change data->dirs to GFiles


Unlike dlg-duplicates, dlg-search follows symlinks so please remember to test with some symlinks.

Committed with minor revisions. Thanks!

- Mike

Created attachment 137349 [details] [review]
GIO Error handling

Remeber to call g_error_free on errors in a few places
Fix a mistake with error handling in path_list_next_files_cb
Only request standard file attributes in path_list_async_new
Pass the right gfile to gfile_warning in directory_load_cb
Improve error handling for symlinks in dlg-search

Created attachment 137785 [details] [review]
GIO port dlg-file-utils delete and trash functions to GFile

Add get_checked_images_as_fd to dlg-duplicates
Update delete_cb in dlg-duplicates to call dlg_file_delete__confirm with a FileData GList
Rewrite dlg_file_delete__confirm and real_files_delete(_continue) functions to use GFiles.
Add real_files_delete__no_trash
Rewrite dlg_files_move_to_trash and dlg_files_delete using GFile
Add delete_next_file and trash_next_file_function
Add new function for displaying progress dialog while trashing files
Update gth_browser_activate_action_image_delete to call dlg_file_delete__confirm with a FileData GList
Update gth-fullscreen.c delete_current_image to call dlg_file_delete__confirm with a FileData GList

There is a new string in file_trash_progress_update, I'm not sure what needs to be done for translations.

This needs more testing, particularly on large file sets and on sets that stop because of error (for e.g. no permission to delete a file in the set).

Nothing special needs to be done for translation except:

1. Mark the string for translation using the _("") macro.

2. Make sure the file is listed in po/POTFILES.in.

- Mike

That should already be done.

Anything else need fixing or changing before you're happy with this? It's kind of blocking the rest of the work on dlg-file-utils until this is committed/merged in a fairly stable state.

Sorry, I've been slow and busy :-(

I didn't see any major issues on a first read-through. Just the usual formatting annoyances.

I'll try to commit it "soon".

- Mike

No problem. As long as you don't think any major changes will be required I'll keep going with the other functions in dlg-file-utils.

I'm not sure how you work with Git but something I would really appreciate if you can do it is, when fixing up my formatting etc. do it as two commits. First with my patch unchanged then with your changes as anther commit. It seems to be the way recommended by most people and it would help me easily see what changes you're making.

OK, that sounds like a good idea.

I'll try to get to it tonight.

- Mike

I committed the patch, with a follow-up commit to fix minor style issues.

However, bug 588125 needs to be fixed - there is some improper unref'ing in the duplicates tool.

- Mike

Created attachment 138486 [details] [review]
GIO port dlg-file-utils to GFile

Complete rewrite of folder_copy functions using GIO/GFile
Convert file_copy functions to GIO/GFile
Convert item_copy functions to use the new file_copy/folder_copy functions
Update dlg-write-to-cd, gth-browser-action-callbacks, catalog-web-exporter to use the new functions
Update overwrite_dialog functions
Removed all remaining GnomeVFS functions/references from dlg-file-utils

There is a lot of completely new code here, needs testing with copying/moving files and folders, dragging & dropping files/folders and deleting folders.

Bug 587795 has broken my build system (anybody know how to fix it?), and I will be away next week, so I will be slow to review this patch...

- Mike

Is this bug fixed in the ext branch?

yes