GNOME Bugzilla – Bug 625897
Port to GSettings
Last modified: 2014-04-02 10:10:36 UTC
http://live.gnome.org/GnomeGoals/GSettingsMigration
Created attachment 198605 [details] [review] gsettings port Here is a patch porting sound-juicer to gsettings. There are probably still a few rough edges, but I can rip CDs using it, which means it's working well enough :) This patch includes the GstEncodeBin changes from #661261 These changes can be found broken down in http://cgit.freedesktop.org/~teuf/sound-juicer/log/?h=gsettings
By the way, I haven't looked at all at the automatic gconf->gsettings migration described at http://developer.gnome.org/gio/stable/ch28s07.html , it might be worth adding this.
NB I have commented out + /*gconf_bridge_bind_window_size(gconf_bridge_get(), GCONF_WINDOW, GTK_WINDOW (main_window)); */ and totally forgot to reimplement it :p
I've rebased http://cgit.freedesktop.org/~teuf/sound-juicer/log/?h=gsettings on top of git master. I have only compile-tested this rebase.
Anybody around to review this ?
Created attachment 216618 [details] [review] Port libjuicer to GSettings [Updated] As the patches mentioned do no longer apply against current master, I've updated and slightly modified them. I hope you'll find this useful. They are split in five parts, to make a review easier.
Created attachment 216619 [details] [review] Port src/ to GSettings [Updated]
Created attachment 216620 [details] [review] Port tests/ to GSettings [Updated]
Created attachment 216621 [details] [review] Add a schema file [Updated]
Created attachment 216622 [details] [review] Remove remaining GConf related stuff [Updated]
Created attachment 216648 [details] [review] Remove remaining references to GConf and remove obsolete files. This is a slightly modified version of the initial patches provided by Christophe Fergeau <cfergeau@redhat.com>
The last patch didn't apply as is because of conflicts in POTFILES.in, here is a rediffed version. While the splitting makes things more tractable from a review point of view, it doesn't make sense to commit this as separate patches since without the gsettings schema introduced in the 4th patch, sound-juicer won't start. It cannot be moved earlier as is because otherwise the compilation of the GConf stuff would be broken. Thanks a lot for updating these patches!
When you say you've slightly modified them, can you detail what changes you made? Just the changes needed for the rebase to work? Or more changes?
Review of attachment 216618 [details] [review]: Usually when you've only "slightly modified" it, you wouldn't set yourself as the author. It should be "From: Christophe Fergeau" with additional work by you. Please also list the differences to the original patches.
I never wanted to imply that I claim authorship. If I offended anyone, I beg for pardon, it wasn't my intention. I'm rather new in using git, so I'll do better next time. Concerning the differences: Most of them are due to changes in the upstream code, changes I made are of the type like correction of the schema path ids (originally starting with /apps) and so on. I'll provide more detailed diff against the changes made in Christophes gsettings branch on fdo on your request, but maybe it will take until the weekend as I currently have not much spare time throughout the week.
Created attachment 216704 [details] [review] Port libjuicer to GSettings I finally figured out how to preserve the original Author, so I'll attach updated patches of the changes made by Christophe Fergeau in the gsettings branch on fdo.
Created attachment 216705 [details] [review] Port src/ and tests/ to GSettings
Created attachment 216706 [details] [review] Remove GConf from the build system
Created attachment 216707 [details] [review] Add a schema file
Created attachment 216708 [details] [review] Minor fixes
Review of attachment 216704 [details] [review]: ::: libjuicer/sj-metadata-musicbrainz5.c @@ +48,3 @@ +#if 0 +#define SJ_SETTINGS_MUSICBRAINZ_SERVER "/apps/sound-juicer/musicbrainz_server" Don't add if 0 code. @@ +620,1 @@ +#if 0 Ditto.
Review of attachment 216705 [details] [review]: ::: src/egg-play-preview.c @@ +530,3 @@ return; + audiosink = gst_element_factory_make ("gsettingsaudiosink", "audiosink"); Use autoaudiosink here. ::: src/sj-main.c @@ +1160,3 @@ */ static void +http_proxy_setup (GSettings *settings) Why do we have this proxy handling code both in sj-main.c and the backends? @@ +1640,3 @@ + sj_settings = g_settings_new ("org.gnome.SoundJuicer"); + if (sj_settings == NULL) { + g_warning (_("Could not create GSettings object.\n")); This is not necessary, g_settings_new() can never fail. @@ +1859,3 @@ duplication_enabled = is_cd_duplication_available(); + /*gconf_bridge_bind_window_size(gconf_bridge_get(), GCONF_WINDOW, GTK_WINDOW (main_window)); */ This will need fixing. g_settings_bind_with_mapping() can probably helper there. ::: src/sj-play.c @@ +389,3 @@ volume = gst_element_factory_make ("volume", "vol"); g_assert (volume); g_object_set (G_OBJECT (volume), "volume", vol, NULL); + out = gst_element_factory_make ("gsettingsaudiosink", "out"); autoaudiosink again. @@ +585,3 @@ g_object_set (G_OBJECT (volume), "volume", vol, NULL); } + g_settings_set_double (sj_settings, SJ_SETTINGS_AUDIO_VOLUME, vol); PulseAudio should be loading and restoring the audio volume, instead of the app itself. It's what Totem does anyway.
Review of attachment 216706 [details] [review]: Looks fine.
Review of attachment 216707 [details] [review]: ::: data/org.gnome.SoundJuicer.gschema.xml @@ +4,3 @@ + <key name="device" type="s"> + <default>''</default> + <summary>The CD to extract from.</summary> CD drive maybe? @@ +20,3 @@ + <key name="base-path" type="s"> + <default>''</default> + <summary>The local directory to save the extracted music to (deprecated, use base_uri)</summary> No need to add deprecated keys here. @@ +35,3 @@ + <default>8</default> + <summary>The paranoia mode to use</summary> + <description>Paranoia mode: 0) disable 2) fragment 4) overlap 8) scratch 16) repair 255) full</description> Should this be using an enum instead? @@ +52,3 @@ + <description>The GStreamer media type to encode to.</description> + </key> + <key name="volume" type="d"> As mentioned in other reviews, this should go.
Review of attachment 216708 [details] [review]: ::: data/org.gnome.SoundJuicer.gschema.xml @@ +1,3 @@ <?xml version="1.0"?> <schemalist> + <schema id="org.gnome.SoundJuicer" path="/org/gnome/SoundJuicer/"> Should probably be fixed in the same patch that adds it. ::: libjuicer/sj-metadata-musicbrainz5.c @@ +49,2 @@ #if 0 +#define SJ_SETTINGS_MUSICBRAINZ_SERVER "musicbrainz-server" Code shouldn't be there in the first place.
Review of attachment 216648 [details] [review]: Looks to be duplicating some other patches that were marked for review (gconf-bridge.[ch] can only be removed so many times ;)
Would be nice to get this in for 3.6, to make our gsettings migration story more complete.
I'm trying to work on the comments, but there is still plenty to do and I currently do not have that much time. As I do not have much experience in programming, very often I'm forced to use "trial and error", what prolongs it additionally. While the migration of the gschema file to use enums for paranoia mode, the change to autoaudiosink and the removal of unneccessary stuff was easy, the window settings code is not. I've tried several possible solutions, one that is based on the code found in rhythmbox, one that is more or less a port of gconf-bridge to gsettings. Both are more or less working (at least for the window size, window positioning is another story) but it's maybe not that what you'll like to integrate. Concerning the audio volume stuff: I do not have any experience with pa code, so I will not be able to port the app to use it. Concerning the duplication of the proxy setup: No idea yet, as I don't use a proxy and I do not know if it needs to be initialized on application startup. If someone who knows more about it is willing to complete the task, you're welcome.
Created attachment 223183 [details] [review] Updated patch against git master I've finally found some time to create patches that are in a state to commit here. Here they are.
Created attachment 223184 [details] [review] Add a schema file, make it possible to translate it
Created attachment 223185 [details] [review] Store and restore window size Adds a working replacement for gconf-bridge. By the way: the already accepted patch is obsoleted by attachment 223183 [details] [review].
Review of attachment 223184 [details] [review]: The hack to translate schema files is not needed anymore with intltool 0.50 Take a look to this commit as an example: http://git.gnome.org/browse/epiphany/commit/?id=65391202450a610ed2f25b433ee8ba73e2cf440f ::: data/Makefile.am @@ +10,3 @@ + +@INTLTOOL_XML_NOMERGE_RULE@ + This is not necesary anymore with intltool 0.50 @@ +81,2 @@ EXTRA_DIST = \ + org.gnome.SoundJuicer.gschema.xml.in \ no need for xml.in file, xml is enough ::: data/org.gnome.SoundJuicer.gschema.xml.in @@ +18,3 @@ + <key name="device" type="s"> + <default>''</default> + <_summary>The CD to extract from.</_summary> _ hack is not needed anymore with intltool 0.50
Gert, are these patches available in a public git repository? Getting the right order to apply these patches + solving merge conflicts is unwieldy with bugzilla/git bz
@Javier I'll have a look at the example given and modify the patch accordingly. @Christophe Unfortunately not. I do not have access to any remote repository. The patches were posted in the order they have to be applied against current git master ( 94d7f95b at the time of writing).
Created attachment 223196 [details] [review] Add a schema file, bump intltool requirement to get it translated Update schema file patch according to the comments of Javier.
In an attempt to revive the discussion around these patches, I rebased the patches on top of the current master and pushed a "wip/gsettings" branch to the sound-juicer repository. Would be nice if someone could give this another review!
Thanks for rebasing the patches Andreas. Looking at b677a0ccb 'Store/restore the window size using gsettings' this uses code from gconf-bridge.[ch] and looking at bug 513615 and https://wiki.gnome.org/SoundJuicerRelicense it doesn't look like the original author (Jorn Baayen) of that file ever agreed to the GPL exception for propriety codecs so it might be a good idea to not use this or try contacting him to see if he agrees to the exception. The code is much more general than we need just to store the size of a single window (we don't use the id returned by sj_window_prefs_bind_window_size, we don't use the code that saves the position and it creates a hash table to store a single value), we could write something simpler. I've not looked at the other commits but do we need our own http proxy settings when there is system-wide setting for that?
(In reply to comment #37) > I've not looked at the other commits but do we need our own http proxy settings > when there is system-wide setting for that? Ignore that I was confused by two different #defines both referring to the system proxy settings. I think the way the proxy is handled could be cleaned up. It's only used/needed by libmusicbrainz so it would make sense to put all the proxy handling in sj-metadata-libmusicbrainz5.c. I should be able to post a patch for that by the beginning of next week.
Created attachment 255523 [details] [review] Rationalize http proxy handling The http proxy is only used by libmusicbrainz so move all the proxy handling code into SjMetadataMusicbrainz5. The existing proxy properties of SjMetadata lacked support for authentication so SjMetadataMusicbrainz5 was doing its own thing anyway. The change to gsettings removes the ability to set a local musicbrainz server. The support for this was half-hearted as it was only possible to set a custom hostname, not a custom port. This patch also fixes a crash whenever sound-juicer tried to do a musicbrainz query introduced in commit 49b5a57857 which removed the code that initalizes the musicbrainz query object.
Created attachment 255524 [details] [review] fixup! Add a schema file Try and improve readability of documentation for format specifiers and add missing composer format specifiers.
Created attachment 255525 [details] [review] Check base_uri is a directory
Created attachment 255526 [details] [review] Add fallback file and path patterns If the file pattern is empty then all the ripped tracks are written to ".ogg". If the path pattern is empty then all ripped music ends up in the same directory. The default patterns were removed in commit 49b5a5785 as they were not synchronized between sj-main.c and sj-prefs.c. Adding two utility functions solves this problem.
Created attachment 255527 [details] [review] fixup! Port SoundJuicer to gsettings
I've not had a through look though the new schema file and changes to the build system yet, if someone with more experience could check that the translation is now setup properly that would be helpful. I noticed that there are schema keys for the window position which are not used. I'm not sure if we want to restore the window position each time or leave its placement up to the window manager.
Created attachment 255529 [details] [review] fixup! Add a schema file Try and improve readability of the descriptions for format specifiers and add missing composer format specifiers. Reword some other descriptions as well.
Created attachment 255530 [details] [review] Make schema name and dbus name match. The gsettings documentation recommends that the two names should match. As changing an existing dbus name in probably a bad idea change the schema name instead.
Keeping track of this in bugzilla is getting messy (the broken down patches were lost on the way, the schema file seems to have been lost at some point, ...), we should just land this on master and see how it goes ;)
Review of attachment 255523 [details] [review]: I'd tend to keep this support in SjMetada rather than SjMetadataMusicbrainz5, unless things are much simpler when it's moved to the mb5 code.
Phillip, apart from the comment on the proxy patches, the other ones look good.
(In reply to comment #49) > Phillip, apart from the comment on the proxy patches, the other ones look good. Thanks Christophe. The patch changing the schema name needs updating as I missed out the conversion file from gconf->gsettings. (In reply to comment #48) >I'd tend to keep this support in SjMetada rather than SjMetadataMusicbrainz5, >unless things are much simpler when it's moved to the mb5 code. Moving proxy support to the mb5 code simplifies things as there is no need to read or set the proxy settings in sj-main.c. As we have to do it in the mb5 code anyway it seems to make sense to only do it there. The proxy properties in SjMetadata do not support authentication so the mb5 code reimplements reading the schema to support authentication and the other classes that do not use the http proxy have basically pointless code implementing the properties they never use. Another solution would be to implement all the proxy properties in SjMetadata so at least we can just read the settings once in sj-main.c and set the properties from there. (In reply to comment #47) >we should just land this on master and see how it goes ;) We need to fix a crash in the mb5 code first. How do you feel about the window size code (see comment 37)? Also what about tracking the window position (we have new schema keys for this but don't use them) should we just leave positioning to the window manager?
Created attachment 255666 [details] [review] fixup! Add a schema file Try and improve readability of the descriptions for format specifiers and add missing composer format specifiers. Reword some other descriptions as well. Add gettext/gesettings type to POTFILES.in
Created attachment 255667 [details] [review] Rationalise http proxy handling Christophe - I've changed this patch so that it extends the proxy properties of SjMetadata to have authentication as well and sets them in SjMetadataGetter when it creates an SjMetadata object. This keeps the properties in SjMetadata as you wanted but avoids getting the settings twice as we were doing. Here's the commit message: Stop using the 'enabled' key for the http proxy as it's not meant to be used - see bug 648237. Only read and use the proxy settings in one place. Add missing proxy properties to SjMetadata and bind them in SjMetadataGetter. Bind the properties with G_SETTINGS_BIND_GET_NO_CHANGES to avoid problems when the settings are updated from a different thread. The change to gsettings removes the ability to set a local musicbrainz server. The support for this was half-hearted as it was only possible to set a custom hostname, not a custom port. This patch also fixes a crash whenever sound-juicer tried to do a musicbrainz query introduced in commit 49b5a57857 which removed the code that initalizes the musicbrainz query object.
Created attachment 255913 [details] [review] Paranoia values should be flags not enum The various values are bitwise flags with 'full' provided as a convenience. As gsettings does not allow more than one bit set per flag make 'full' 32 instead of 255.
Created attachment 255919 [details] [review] Paranoia values should be flags not enum The various values are bitwise flags with 'full' provided as a convenience. As gsettings does not allow more than one bit set per flag make 'full' 256 instead of 255, this allows for future expansion of the flags.
I have pushed the attachments Christophe approved to wip/gsettings: Attachment 255525 [details] pushed as a25f084 - Check base_uri is a directory Attachment 255526 [details] pushed as 6d82aee - Add fallback file and path patterns Attachment 255527 [details] pushed as 10f0f24 - fixup! Port SoundJuicer to gsettings Attachment 255530 [details] pushed as 2fc6e05 - Make schema name and dbus name match. Attachment 255666 [details] pushed as 8775d7e - fixup! Add a schema file
Review of attachment 255919 [details] [review]: The 'full' bit is a bit odd, but why not.
Review of attachment 255667 [details] [review]: Few minor comments, looks good otherwise ::: libjuicer/sj-metadata-gvfs.c @@ +184,3 @@ + case PROP_PROXY_USE_AUTHENTICATION: + /* Do nothing */ + g_value_set_boolean (value, FALSE); missing break; here ::: libjuicer/sj-metadata-musicbrainz5.c @@ +677,2 @@ priv = GET_PRIVATE (self); + priv->mb = mb5_query_new (SJ_MUSICBRAINZ_USER_AGENT, NULL, 0); If this is the bit that fixes a crash introduced in an earlier commit, it's probably better to separate it for now as a fixup! commit.
(In reply to comment #44) > > I noticed that there are schema keys for the window position which are not > used. I'm not sure if we want to restore the window position each time or leave > its placement up to the window manager. Imo we can leave that up to the WM
(In reply to comment #37) > Thanks for rebasing the patches Andreas. Looking at b677a0ccb 'Store/restore > the window size using gsettings' this uses code from gconf-bridge.[ch] and > looking at bug 513615 and https://wiki.gnome.org/SoundJuicerRelicense it > doesn't look like the original author (Jorn Baayen) of that file ever agreed to > the GPL exception for propriety codecs so it might be a good idea to not use > this or try contacting him to see if he agrees to the exception. The code is > much more general than we need just to store the size of a single window (we > don't use the id returned by sj_window_prefs_bind_window_size, we don't use the > code that saves the position and it creates a hash table to store a single > value), we could write something simpler. > gedit does this directly in its app window implementation by saving when its dispose implementation runs, and on configure events. It uses a <key name="size" type="(ii)"> value to store (x, y) at once. It stores a window state to be able to restore both maximized and sticky states. As the gconf-bridge code might be tricky to relicense (it was written by Jorn, but the copyright belongs to Opened Hands which was bought by Intel), it could be better not to depend on it indeed...
From a quick look at the diff between master and wip/gsettings, what's in there (plus the few pending patches here) should be mergeable to master after a bit of history cleanup (I haven't tested it though as my CD reader decided not to work yesterday).
(In reply to comment #60) > (In reply to comment #37) > > Thanks for rebasing the patches Andreas. Looking at b677a0ccb 'Store/restore > > the window size using gsettings' this uses code from gconf-bridge.[ch] and > > looking at bug 513615 and https://wiki.gnome.org/SoundJuicerRelicense it > > doesn't look like the original author (Jorn Baayen) of that file ever agreed to > > the GPL exception for propriety codecs so it might be a good idea to not use > > this or try contacting him to see if he agrees to the exception. The code is > > much more general than we need just to store the size of a single window (we > > don't use the id returned by sj_window_prefs_bind_window_size, we don't use the > > code that saves the position and it creates a hash table to store a single > > value), we could write something simpler. > > > > gedit does this directly in its app window implementation by saving when its > dispose implementation runs, and on configure events. It uses a <key > name="size" type="(ii)"> value to store (x, y) at once. It stores a window > state to be able to restore both maximized and sticky states. > As the gconf-bridge code might be tricky to relicense (it was written by Jorn, > but the copyright belongs to Opened Hands which was bought by Intel), it could > be better not to depend on it indeed... You should remember your size, and window state (fullscreen, maximised), but not the window position (the WM will indeed take care of that), but you certainly shouldn't save state in GSettings. Save it in ~/.config that's what it's there for.
Comment on attachment 255667 [details] [review] Rationalise http proxy handling Attachment 255667 [details] pushed to wip/gsettings as commits 6984307 - fixup! Port SoundJuicer to gsettings and 96bc0db - Rationalise http proxy handling
Created attachment 267031 [details] [review] This patch just removes the window settings, I'll write something that stores them in ~/.config and post it here in the next couple of weeks. Remove window state from gsettings The state of the window should be stored in the xdg user config directory instead.
Comment on attachment 255919 [details] [review] Paranoia values should be flags not enum Attachment 255919 [details] pushed as d1d74af - Paranoia values should be flags not enum I decided the 'full' stuff was a bit weird so I removed it before pushing.
(In reply to comment #61) > From a quick look at the diff between master and wip/gsettings, what's in there > (plus the few pending patches here) should be mergeable to master after a bit > of history cleanup (I haven't tested it though as my CD reader decided not to > work yesterday). I rebased it onto master locally quite easily. It would be nice to have the patch removing the window settings before merging it to master. We could wait until I've implemented the replacement but it's not a major regression - what do you think?
So far, I've rebased it to http://cgit.freedesktop.org/~teuf/sound-juicer/log/?h=gsettings I had to hack the proxy patch a bit in order to have the "port to gsettings" one compile. Regarding the window size stuff, would be nice to keep Gert's name in the history to acknowledge his work :) We indeed should avoid getting this in a release though if we don't want to keep it in gsettings (I remembered this was frowned upon in gconf, was not sure it was still the rule with gsettings).
Created attachment 267615 [details] [review] I forgot to modify the GConf->GSettings conversion file in the previous version of this patch Remove window state from gsettings The state of the window should be stored in the xdg user config directory instead.
Created attachment 267617 [details] [review] Save & restore window size & state Save the window size and state when quitting and restore it on start-up. The state is saved in the xdg user config directory. This restore the functionality removed when converting to GSettings.
(In reply to comment #67) > So far, I've rebased it to > http://cgit.freedesktop.org/~teuf/sound-juicer/log/?h=gsettings > I had to hack the proxy patch a bit in order to have the "port to gsettings" > one compile. > Regarding the window size stuff, would be nice to keep Gert's name in the > history to acknowledge his work :) Yes we should definitely credit him. I squashed all the patches apart from "Save & restore window size & state" together when I rebased so as not to break git-bisect. What do you think about this for the commit message? Author: Christophe Fergeau <cfergeau@redhat.com> Date: Sat Aug 6 16:07:16 2011 +0200 Use GSettings instead of GConf GConf has been deprecated for some time now. This change removes the ability to set a local musicbrainz server. The support for this was half-hearted as it was only possible to set a custom hostname, not a custom port. This functionality could be restored fairly easily if there is a demand for it. Saving the window size and state is also removed as this should be stored in the xdg user config dir instead. This will be implemented soon. Thanks to Gert Kulyk & Phillip Wood for their contributions. https://bugzilla.gnome.org/show_bug.cgi?id=625897
Feel free to put yourself as the author as you are the last one to do significant changes to the patch, ACK for pushing this series.
(In reply to comment #71) > Feel free to put yourself as the author as you are the last one to do > significant changes to the patch, ACK for pushing this series. That's kind but I've left you as the author as apart from the reworking of http proxy code it's mostly your original patches. Are you happy for attachment 267617 [details] [review] to be pushed as well, I'm not sure if you've had a chance to look at it yet? The GSettings patches are pushed as commit 89f1575
Review of attachment 267617 [details] [review]: Mostly looks good, a few minor comments ::: src/sj-window-state.c @@ +32,3 @@ +enum { + SJ_WINDOW_STATE_NORMAL, + SJ_WINDOW_STATE_MAXMIZED, MAXIMIZED @@ +59,3 @@ + gchar *filename = get_state_file_name (); + if (!g_key_file_load_from_file (file, + get_state_file_name (), You mean 'filename' here, not get_state_file_name() @@ +154,3 @@ + SjWindowState *state) +{ + if (state->width > 0 && state->height >0) Missing space after ' > ' @@ +157,3 @@ + gtk_window_set_default_size (window, state->width, state->height); + if (state->state == SJ_WINDOW_STATE_FULLSCREEN) + gtk_window_fullscreen (window); Can sound-juicer be put in fullscreen? (I don't have a CD reader plugged into that laptop, so can't check). I don't necessarily object to that bit of code, just curious @@ +192,3 @@ + e = (GdkEventWindowState*) event; + if ((e->new_window_state & GDK_WINDOW_STATE_FULLSCREEN) != 0) + state->state = SJ_WINDOW_STATE_FULLSCREEN; We could have state->state as a bit flag as well, but not really important, this is enough for sj needs.
Created attachment 270156 [details] [review] fixup! Save & restore window size & state (In reply to comment #73) Thanks for looking at that, I've fixed up the issues you pointed out >Can sound-juicer be put in fullscreen? (I don't have a CD reader plugged into >that laptop, so can't check). I don't necessarily object to that bit of code, >just curious Yes - the window manager can make it fullscreen, I agree that there's not much point to having sound-juicer fullscreen but the user can force it with the window manager so we might as well save all the window state.
I've pushed the patches to store the window state as there were only minor issues in the review which I've fixed and I wanted to get it in the release. Attachment 267617 [details] and attachment 270156 [details] [review] pushed as commit 0a3aa25 - Save & restore window size & state