Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Port more prefs to Stash #1184

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Port more prefs to Stash #1184

wants to merge 10 commits into from

Conversation

b4n
Copy link
Member

@b4n b4n commented Aug 20, 2016

This both reduces the amount of code (by more than ⅔!), and tries to move towards a state where new contributors don't feel like using manual keyfile read/write is the way to go (experience has shown the load_dialog_prefs() has a way of going unnoticed -- but @elextr will come up with a nice way of documenting this 😉).

In theory it also reduce the possibility for typos leading to settings not being saved/loaded properly (i.e. if the setting name/group doesn't match), but as it's currently working it mostly allows for mistakes in the diff of this PR 😄
Though, I should have done that carefully enough to not risk too much. But review is of course a good idea (hint hint).

@codebrainz
Copy link
Member

Seems like a reasonable first step to get prefs all the using the same API so we can more easily switch them all at once to something more appropriate like GSettings (w/ keyfile backend).

Still need to review the code.

@elextr
Copy link
Member

elextr commented Aug 20, 2016

LGBCI

but @elextr will come up with a nice way of documenting this

Love to help, but I don't know what you are talking about (guess whatever is unnoticed is still unnoticed :)

The UI for the icon size and style are a little tricky, because they
use radio buttons but spread them across two settings: the "system
default", and the specific values.  To handle this, we give a dummy
default to the "system default" in the specific setting, and handle
the separate "system default" setting *after* for it to override the
radio button choice.

This is somewhat tricky, but not worse than what was previously done,
where the specific setting wasn't loaded if the "system default" was
used, yet it was written (and actually used in the fallback path in
case the "system default" is missing), leading to using a potentially
random value.
Are left the ones requiring special handling.
@b4n
Copy link
Member Author

b4n commented Aug 20, 2016

@elextr what's "C" in "LGBCI"? "Code"? "Careful"?

Love to help, but I don't know what you are talking about (guess whatever is unnoticed is still unnoticed :)

Heh ^^

/* note: new settings should be added in init_pref_groups() */
static void load_dialog_prefs(GKeyFile *config)
{

@elextr
Copy link
Member

elextr commented Aug 21, 2016

On 21 August 2016 at 00:44, Colomban Wendling notifications@github.com
wrote:

@elextr https://github.com/elextr what's "C" in "LGBCI"? "Code"?
"Careful"?

​Cursory, wouldn't want you to think I had minutely checked each setting :)​

Love to help, but I don't know what you are talking about (guess whatever
is unnoticed is still unnoticed :)

Heh ^^

/* note: new settings should be added in init_pref_groups() */static void load_dialog_prefs(GKeyFile *config)
{

​Ahhh​


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#1184 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAxgTTBmd9OstVh-yXrsnCKReu61ekGAks5qhxK5gaJpZM4Jo8a_
.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants