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 514673 - [PATCH] Port to gio from gnome-vfs
[PATCH] Port to gio from gnome-vfs
Status: RESOLVED FIXED
Product: sound-juicer
Classification: Applications
Component: general
git master
Other Linux
: Normal normal
: ---
Assigned To: Sound Juicer Maintainers
Sound Juicer Maintainers
Depends on:
Blocks:
 
 
Reported: 2008-02-06 04:23 UTC by Michael Terry
Modified: 2008-06-05 10:28 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Proposed patch (22.72 KB, patch)
2008-02-06 04:23 UTC, Michael Terry
committed Details | Review
New patch against bzr (16.73 KB, patch)
2008-02-10 16:35 UTC, Michael Terry
committed Details | Review
Use g_get_special_dir (3.50 KB, patch)
2008-02-10 21:52 UTC, Michael Terry
committed Details | Review

Description Michael Terry 2008-02-06 04:23:11 UTC
Hello!  As part of the GioPort effort (http://live.gnome.org/GioPort), I put together a patch to convert sound-juicer to the new gio file system abstraction.

With this patch, sound-juicer no longer depends on gnome-vfs.  However, it still requires the gnomevfssink element.  Once the giosink element stabilizes (currently set to not build in gst-plugins-bad), switching to it is a simple two line change (configure.in and sj-extractor.c).
Comment 1 Michael Terry 2008-02-06 04:23:48 UTC
Created attachment 104533 [details] [review]
Proposed patch
Comment 2 Ross Burton 2008-02-06 07:35:59 UTC
Thanks for this!

We'd need to switch GStreamer element before this can be used, because otherwise there will be an interesting mismatch between what URLs the UI and the extractor are using.

Of course, once it's all GIO, the URLs can be dropped and replaced with GFiles.

Maybe I should create a gio branch for this...
Comment 3 Bastien Nocera 2008-02-06 10:27:37 UTC
Potentially, it should be using the gio cdda backend as well for input, so that concurrent access to mixed type discs works as expected.
Comment 4 Ross Burton 2008-02-06 10:32:44 UTC
To be honest I'd prefer to use gstreamer's CDDA source.  I've been using it locally for a few weeks now and it seems good.  A little slower than cdparanoia, but I've no idea at what level of "paranoia" it is using.
Comment 5 Bastien Nocera 2008-02-06 10:48:13 UTC
(In reply to comment #4)
> To be honest I'd prefer to use gstreamer's CDDA source.  I've been using it
> locally for a few weeks now and it seems good.  A little slower than
> cdparanoia, but I've no idea at what level of "paranoia" it is using.

You mean gstreamer's libcdio source I guess? That's what the gvfs cdda backend uses as well.
Comment 6 Ross Burton 2008-02-06 10:55:36 UTC
I've made a bzr branch of SJ, and committed that patch to it:

https://code.launchpad.net/~ross/sound-juicer/gio

Next up:
* Sanitise libjuicer API.  All file references should be via GFile, nothing
else.
* Switch to giosink

I don't have time right now to work on this, so if you want to continue working
on this Michael, that would be greatly appreciated!


Both the gstreamer cdda element and gvfs cdda element use libcdio, but the gstreamer element would integrate better surely.
Comment 7 Michael Terry 2008-02-06 12:34:05 UTC
Yeah, I'm interested in continuing the porting work.
Comment 8 Michael Terry 2008-02-10 16:35:51 UTC
Created attachment 104853 [details] [review]
New patch against bzr

OK, here's a further patch against that bzr tree.  It converts libjuicer to GFile (make_directory, get_default_music_dir, get_submit_url).  It also uses GFile more consistently in src/.

It's possible libjuicer could use GDrive instead of device character strings, but those strings come from nautilus and it seemed more effort than it was worth to convert from string->GDrive->string, since gstreamer wants the string too.

I switched to giosink (you need to compile gst-plugins-base with --enable-experimental to get it).

I left cdda alone for now, as it seemed tangental to gio.
Comment 9 Ross Burton 2008-02-10 17:48:24 UTC
Mostly committed and pushed, thanks!

You shouldn't change the URLs returned by the metadata backends, those really are plain URL strings.  I've reverted these changes.

Instead of having sj_get_default_music_directory in sj-utils, we can now use g_get_user_special_dir() directly.
Comment 10 Michael Terry 2008-02-10 21:52:00 UTC
Created attachment 104871 [details] [review]
Use g_get_special_dir

I'd argue that since g_get_special_dir can return NULL, sj_get_default_music_directory is just as useful as before (to automatically provide the fallback value of ~).

But g_get_special_dir does mean that we can drop xdg_user_dir_lookup.  Here's a patch to do that.
Comment 11 Bastien Nocera 2008-02-10 23:45:35 UTC
"The GUserDirectory enumeration can be extended at later date. Not every platform has a directory for every logical id in this enumeration."

This means that (probably) Windows won't be able to find some special directories. That's the only case for which you'd get a NULL back from g_get_user_special_dir(). But you don't have any assurance that the directory will exist.

All in all, the semantics are different, so it's not a straight replacement.
Comment 12 Michael Terry 2008-02-11 01:34:49 UTC
NULL doesn't appear to be that odd a case.  On Linux, if user-dirs.dirs doesn't exist, isn't readable, or doesn't have the right entry, g_get_user_special_dir will return NULL.  Certainly the API doesn't guarantee anything.  I don't know how common those cases are, but they seem worth coding around.

What are the differing semantics?  xdg_user_dir_lookup and g_get_user_special_dir look pretty similar to me.  Neither of them assures that the directory exists.
Comment 13 Ross Burton 2008-02-11 10:17:53 UTC
Committed, thanks.
Comment 14 André Klapper 2008-04-09 17:50:35 UTC
so the port to gvfs is not yet resolved only because of gnomevfssink, if i got it right?
Comment 15 Ross Burton 2008-04-09 18:04:06 UTC
Yes.  When giosink is in gst-plugins-good, I'll commit this.  If the gstreamer people commit to doing that "shortly" I'll put this into sj trunk though.
Comment 16 Bastien Nocera 2008-04-09 20:58:07 UTC
(In reply to comment #15)
> Yes.  When giosink is in gst-plugins-good, I'll commit this.  If the gstreamer
> people commit to doing that "shortly" I'll put this into sj trunk though.

It's already in -base, when --enable-experimental is enabled for configure.
Comment 17 Ross Burton 2008-04-09 21:13:28 UTC
And its packaged in Debian too... Time to branch!
Comment 18 Ross Burton 2008-04-14 10:16:31 UTC
I'll happily test and merge the GIO patch, but first it needs updating for the changes in svn.

https://code.launchpad.net/~ross/sound-juicer/gio is the bzr branch with the code in, a simple "bzr merge" will pull in master and cause a few conflicts which need to be fixed.
Comment 19 Michael Terry 2008-04-17 01:27:44 UTC
OK, assuming I did everything correctly, you should find merged code: https://code.launchpad.net/~mterry/sound-juicer/gio

The conflict arose because s-j trunk had added support for first creating files at .FILENAME and then moving them to FILENAME when encoding completed.  If s-j can assume that files will be written using gio, it may not need to do that (I believe gio promises atomic replacements).  But, it seems safe enough to leave it in, so I did.
Comment 20 Ross Burton 2008-04-17 07:04:59 UTC
Thanks, I'll merge this shortly.  This isn't for atomic operations, but so that if you start ripping, overrwiting an existing file and then cancel, you don't lose the original file.
Comment 21 Ross Burton 2008-04-17 08:37:36 UTC
Merged, thanks.

One last thing... (I sound a little like Columbo) there are several g_file_* calls in sj-extracting which can fail but are not checked for (specifically g_file_move and g_file_delete).  Please add a GError argument and handle errors corectly.  Then I think we're good for merging!
Comment 22 Michael Terry 2008-04-17 11:31:35 UTC
When I said atomic, I meant "doesn't overwrite unless operation is successfully completed".  i.e. if you cancelled it, gio shouldn't mess with original file.  At least that's what I gather from the docs for g_file_replace:

"This will try to replace the file in the safest way possible so that any errors during the writing will not affect an already existing copy of the file. For instance, for local files it may write to a temporary file and then atomically rename over the destination when the stream is closed."

As for GError's, you're right that they should be there, and I'll add them, but in my defense I'll note that the original code didn't handle errors there either!
Comment 23 Michael Terry 2008-04-22 01:01:56 UTC
OK, merge from my bzr branch again.  I've merged with HEAD and added GError support to the delete/move calls.  The delete call uses the generic "Sound Juicer could not extract this CD" error, which is a little odd since you're cancelling the extraction, but I left it alone.  Wasn't sure "could not clean up after itself" was any better.

Regarding my 'atomic' statements about giosink.  I've filed bug 529300 to allow giosink to replace files.  SJ wants this anyway, because otherwise if the temporary file already exists, we can't replace it.  So, my branch really recommends that that patch be applied (having an already-there temporary file is unusually, but probably not impossible).

Once the 529300 patch is applied and giosink uses atomic writes, SJ still can't just drop support for using the create_temporary/move logic.  SJ wants to delete the incomplete file if the user hits Cancel.  But when SJ cancels the gst operation, giosink just closes the stream and replaces the original file with the incomplete one.  Which isn't what we want.  So, in summation, I retract my claim that giosink can solve the overwrite problem for us.
Comment 24 Ross Burton 2008-04-23 10:21:28 UTC
Are you sure the overwrite signal is required?  There doesn't appear to be such a signal in giosink.
Comment 25 Michael Terry 2008-04-23 23:24:38 UTC
You're right, the signal isn't in giosink.  I submitted a patch in bug 529300 to add it, based on the previous gnomevfssink signal.  But as you can see in that bug, exactly how to allow overwriting is in question.

SJ is interested in this because:

Say, you want to extract the file "FOO".  Normally, SJ tells giosink to write ".FOO" and then moves it to the correct place when the write is complete.

But imagine ".FOO" already exists.  giosink will report an error because it never overwrites anything (in HEAD, it returns an odd error about not being able to *read* -- this is a typo in giosink).

Now, either giosink could gain the ability to overwrite (see abovementioned bug 529300) or SJ could solve this by first deleting any existing ".FOO" before writing.  After all, we'd be pretty sure it's our own trash.

But for now, I figured I'd try to get it into giosink proper, since it seems like a generally useful ability (to overwrite the sink target).

My bzr branch will be updated depending on the outcome of where to put this functionality, and I'll comment here.
Comment 26 Bastien Nocera 2008-05-24 16:48:09 UTC
Could we please get the code in sound-juicer trunk? Then we can work on finishing whatever needs to be done with the giosink.
Comment 27 Ross Burton 2008-06-05 10:28:15 UTC
Committed to trunk!