This is to get the ball rolling on the [Port to GSettings project](https://github.com/geany/geany/projects/1).
See individual commit messages for more details. You can view, comment on, or merge this pull request online at:
https://github.com/geany/geany/pull/1257
-- Commit Summary --
* Add initial GSettings support * Change 'sidebar_pos' to use GSettings * Port "fullscreen" preference to GSettings * Port "sidebar_visible" preference to GSettings * Port "msgwindow_visible" preference to GSettings * Port "statusbar_visible" preference to GSettings * Port "sidebar_*_visible" preferences to GSettings
-- File Changes --
M configure.ac (2) M data/Makefile.am (4) A data/org.geany.Settings.gschema.xml (53) M po/POTFILES.in (1) M src/Makefile.am (2) M src/build.c (11) M src/callbacks.c (70) M src/keybindings.c (5) M src/keyfile.c (29) M src/libmain.c (33) M src/msgwindow.c (25) M src/msgwindow.h (2) M src/prefs.c (44) A src/settings.c (77) A src/settings.h (35) M src/sidebar.c (46) M src/sidebar.h (2) M src/ui_utils.c (63) M src/ui_utils.h (13) M src/vte.c (5)
-- Patch Links --
https://github.com/geany/geany/pull/1257.patch https://github.com/geany/geany/pull/1257.diff
As discussed on IRC, I agree that the settings system could do with an improvement.
But I have concerns about the implementation being able to meet the goals of the overarching [project](https://github.com/geany/geany/projects). If the implementation provides no improvement why do it? (though I might agree that removing custom hand rolled stash is sufficient improvement in itself :)
The areas I see as issues are:
1. Direct binding will not be possible for settings overridden by things like projects and filetypes unless GSettings can handle it itself. Is this possible? If not then asynchronous change handling code needs to be provided for those settings, probably just MMOP, but I would like to see that its possible and how complex it is before we commit to the implementation.
2. The current implementation doesn't handle the `-c` option AFAICT, did I miss something?
If the implementation provides no improvement why do it?
See the description of the project for the Goals, those are the improvements it brings, none of which are "no improvement" as you inferred.
The current implementation doesn't handle the -c option
https://github.com/geany/geany/pull/1257/commits/5a8ec3d2c9ef17ea42a454184ea...
If that's not enough, there is something smelly going on.
FWIW this isn't the first editor I've [converted from adhoc to GSettings](https://git.xfce.org/apps/mousepad/commit/?id=3b084a5adb50fae1cb42c19528d841...), though Geany benefits from the lessons learned on linked changes.
Improve multi-instance/shared-config support. Use g_object_bind_property()/g_settings_bind()/etc. where useful, rather than rolling own with manual code, helper functions and Stash API.
Those are the two that I am just wanting to be sure will be possible and want to know how they will be done for overridable properties. I'm not saying the other two benefits are not enough to make the changes anyhow, but it would be nice to know ahead of time that it will achieve the goals, and how much work is needed, rather than be annoyed or disappointed later that an implementation doesn't live up to its promise.
If that's not enough, there is something smelly going on.
Damn globals :) Thats fine.
FWIW this isn't the first editor I've converted from adhoc to GSettings, though Geany benefits from the lessons learned on linked changes.
Does moosepad do projects or similar? If so how did you do it?
Does moosepad do projects or similar? If so how did you do it?
It does not, and like Geany's projects, it was not considered part of the overall settings improvements. Handling project specific settings (ie. a separate `GKeyFile`) is, IMO, a separate topic and should not be discussed in this context as it should be no more of a mess than at present. That being said, I believe the proposed goals will at worst not make project settings worse and most likely make them better (if only by virtue of overall code cleanup that accompanies the changes), if not also improving project settings in the future directly.
Those are the two that I am just wanting to be sure will be possible
Already with the changes in this PR they are both implemented. Overriding with separate GKeyFile project settings is not changed from the current behaviour, however.
Handling project specific settings (ie. a separate GKeyFile) is, IMO, a separate topic and should not be discussed in this context as it should be no more of a mess than at present.
It is part of this topic because project settings interact with user settings, it cannot be ignored.
Did you read what I asked? To ask it simply, how are asynchronous changes handled?
It is part of this topic because project settings interact with user settings, it cannot be ignored.
As long as the current functionality is not degraded and continues to be supported, I think it can be treated separately once the core/base settings are all using GSettings.
how are asynchronous changes handled?
On a case-by-case basis. In Mousepad, where everything is "properly GObject-ified" I bound lots of stuff together using GObject property binding and GSettings binding, and binding both ways, and this lead to a few racy-like bugs and not helping to reduce settings spaghetti. In this PR for Geany, I try to only for now reproduce the directionality of the settings changes, thinking about which way(s) they should go.
By being careful (as at present) what updates what, we can assure some useful settings transcend instances while others only store to settings and affect new instances. For example I probably don't want a "fullscreen" GSetting to affect all running instances, but I might want it to affect new instances. Conversely, I might want a setting like "editor-font" to affect all running instances (or not). In this PR and related ones, this behaviour is completely open for discussion, since "live updating" of settings was not previously possible.
You just outlined the sorts of things I am concerned about quite well, as I said, Geany does not have any support for asynchronous changes now, so just how complex is the "spaghetti" to implement it for the settings that don't work by the simple automatic bindings. Thats all I'm asking to know, how difficult are the hard things, before we do lots of simple things, and then go "oh shit" when we hit a hard thing :).
And you noted some other considerations I hadn't thought of, the logical correctness of doing automatic settings update. That may mean more settings won't be able to use automagic bindings either.
That may mean more settings won't be able to use automagic bindings either
Indeed, though some will and there will be an overall reduction in spaghetti.
so just how complex is the "spaghetti" to implement it for the settings that don't work by the simple automatic bindings
The 2nd and later commits give an impression of that, mostly they are decidedly directional.
The 2nd and later commits give an impression of that, mostly they are decidedly directional.
I don't see where they handle asynchronous changes?
And following on the logical correctness, I'm not sure message window should follow changes in the settings, if say a build with errors opens it on one instance I'm pretty sure I don't want it to open on all instances. So this setting should only be read at open, then be local until saved at close ... oh wait thats the same problem as now :)
I don't see where they handle asynchronous changes?
None of this is truly asynchronous in the kernel/thread/interrupt sense. It all happens synchronously on the main context/thread.
So this setting should only be read at open, then be local until saved at close
The way it was ported (open to discussion) is that the UI updates the setting, and the setting is used for the active and new instances but will not reflect in other already open instances.
None of this is truly asynchronous in the kernel/thread/interrupt sense. It all happens synchronously on the main context/thread.
True, but irrelevant, what is meant by "asynchronous" is by another instance, not this one.
True, but irrelevant, what is meant by "asynchronous" is by another instance, not this one.
The changes to the backing keyfile arrive as events on the main loop/thread/context using `GFileMonitor`, what the instance does (or does not do) with the events depends on what the desired behaviour is (or is not).
The way it was ported (open to discussion) is that the UI updates the setting, and the setting is used for the active and new instances but will not reflect in other already open instances.
Sigh, this setting is an example where the settings and the session get mixed, this looks right for the setting, but now the session status won't be saved at all.
That is a change from current behaviour.
It probably doesn't matter for this setting, but it could be important for other settings.
Sigh, this setting is an example where the settings and the session get mixed, this looks right for the setting, but now the session status won't be saved at all.
That is a change from current behaviour.
Did you test it? It should not change any behaviour with respect to the "session" (project). If it does in practice, then it's an unintended regression. The desired behaviour is identical to current, except that new instances will pickup the change now that existing instances don't wait until exit to save their config.
Hmmm, AFAICT builds only set the message win [visible](https://github.com/geany/geany/pull/1257/commits/8f56c0fb11e515e480f87193529...) they don't set the setting. Which is also what you said above. So how does it get restored on restart? Currently the state of the message window is restored irrespective of what set it visible. Not talking about projects, just plain user config.
https://github.com/geany/geany/pull/1257/commits/8f56c0fb11e515e480f87193529...
The way it was ported (open to discussion) is that the UI updates the setting
So its not just the UI that updates the setting, the build system does too?
It's the same as before, except rather than everywhere updating a global variable and later saving it to the config file at a central point (ex. on exit), it just sets the GSetting which is like an automatically persistent version of of the global variable.
The changes to the backing keyfile arrive as events on the main loop/thread/context using GFileMonitor, what the instance does (or does not do) with the events depends on what the desired behaviour is (or is not).
What I am asking (repeatedly) for is an example of a setting that uses this and not simply via bind.
The 3rd and later commits don't use any binding. The 2nd commit binds one-way the sidebar notebook page to the related setting.
So why do you keep referring to those commits when I ask for an example of whats required to handle changes from another instance without binding? As I said in the first place, there is no example of that. Thats what I am asking for as its the most complex situation.
Because until you just asked plainly it wasn't clear what you were talking about :)
Read the [GSettings documentation](https://developer.gnome.org/gio/stable/GSettings.html). You can either connect to the changed signal, bind it to an object property, or create an action and connect to its activation. I haven't ported any settings that need to update other instances when changed yet, but if you give me a current preference that would be useful to make update other instances, I can try and port it as an example. Even better, you can try porting one and submit a PR on my repo :)
If you have all this experience why can't you simply do an example that uses that experience instead of giving examples of the simplest cases and then suggest others waste time re-learning your experience?
I don't know what you want exactly, you know what a `g_signal_connect()` call looks like. If you want to see a real setting ported, give me one of the existing settings that makes sense to have update in real-time in all instances. At present none do, and many/most won't make sense to do that, so give name one that does and I'll port it before continuing with the UI prefs.
Ok, one of the indentation settings, say width.
Reason for choice:
I already use separate config files for my python projects from my C++ projects since I want several settings different to my C++ ones that are not available in projects (which autocompletes for eg) and also the ones available in projects like indent settings. It would then be great to to be able to use more than one instance looking at the same project by sharing that config (since Geany doesn't do multiple windows). Since both instances will be looking at the same project changes to indent should be propagated in my case, but as you said in a post above that might not be suitable for all use-cases and so a pref might be needed?
I don't think that one would be useful to update on the fly since it wouldn't affect opened files anyway and newly opened files (which weren't using filetype or project settings) would already pickup the changed default setting.
wouldn't affect opened files anyway
Oh yeah, that damn annoyance, well I _suppose_ thats out of scope for this project :)
Ok, well, then one of `Ensure new line at file end` or `Ensure consistent line endings` or `strip trailing spaces` or (and I was trying NOT to suggest this but...) build settings.
@codebrainz pushed 2 commits.
44a3583 Port "sidebar_pos" preference to GSettings e00e149 Refactor GSettings ported so far
I added a (rather large) commit to refactor/cleanup the way the ported GSettings are handled. Amongst other things it makes use of `g_settings_bind()` to keep things automatically in-sync. If there are changes we don't want propagated between instances sharing the same config file, we could block them. This may or may not be better code-wise (TBD).
Sorry for the huge commit, but I had to do it all at once to keep it building and be able to test.
@codebrainz pushed 1 commit.
d966c58 Port "editor_font" to GSettings
@elextr just ported "editor_font" which shows what you were asking for an example of before, this is probably the "worst case" ugliness since:
a) Has some `#include "settings.h"` which won't be required for porting future settings. b) ScintillaWidget unlike what you'd expect has no "font" property to bind directly to so it requires handling in a `changed` callback. c) Maintains `interface_prefs.editor_font` compatibility for plugin API.
codebrainz commented on this pull request.
@@ -86,12 +86,10 @@
#define GEANY_DEFAULT_TOOLS_BROWSER "open -a safari" #define GEANY_DEFAULT_FONT_SYMBOL_LIST "Helvetica Medium 12" #define GEANY_DEFAULT_FONT_MSG_WINDOW "Menlo Medium 12" -#define GEANY_DEFAULT_FONT_EDITOR "Menlo Medium 12"
@techee for this we could use "Vendor Overrides" mentioned [in the API docs](https://developer.gnome.org/gio/stable/GSettings.html#GSettings.description).
codebrainz commented on this pull request.
@@ -86,12 +86,10 @@
#define GEANY_DEFAULT_TOOLS_BROWSER "open -a safari" #define GEANY_DEFAULT_FONT_SYMBOL_LIST "Helvetica Medium 12" #define GEANY_DEFAULT_FONT_MSG_WINDOW "Menlo Medium 12" -#define GEANY_DEFAULT_FONT_EDITOR "Menlo Medium 12"
Specifically, I think it would require a file called `org.geany.Settings.gschema.override` in the bundle's equivalent of the `$(datadir)/glib-2.0/schemas/` directory with the following contents:
```ini [org.geany.Settings] editor-font="Menlo Medium 12" ```
@codebrainz pushed 1 commit.
1504c63 Port "tagbar_font" to GSettings
@codebrainz pushed 1 commit.
236017f Small cleanup of `settings.c`
elextr commented on this pull request.
@@ -41,6 +41,14 @@ static void on_sidebar_pos_left_changed(GSettings *settings, gchar *key, gpointe
}
+static void on_editor_font_changed(GSettings *settings, gchar *key, gpointer user_data) +{
@codebrainz do we need an `on_foo_bar_setting_changed()` for every "interesting" setting? Or can we use one `on_setting_changed()` with `user_data` specifying which setting. Then we would only possibly need a few callbacks.
codebrainz commented on this pull request.
@@ -41,6 +41,14 @@ static void on_sidebar_pos_left_changed(GSettings *settings, gchar *key, gpointe
}
+static void on_editor_font_changed(GSettings *settings, gchar *key, gpointer user_data) +{
We could do it that way too, there would be one `on_setting_changed` handler and it would "switch" on the `key` parameter. Not sure which way is better.
elextr commented on this pull request.
@@ -86,12 +86,10 @@
#define GEANY_DEFAULT_TOOLS_BROWSER "open -a safari" #define GEANY_DEFAULT_FONT_SYMBOL_LIST "Helvetica Medium 12" #define GEANY_DEFAULT_FONT_MSG_WINDOW "Menlo Medium 12" -#define GEANY_DEFAULT_FONT_EDITOR "Menlo Medium 12"
IIUC the overrides are for distros to use, and Geany OSX is effectively a distro run by @techee so it makes sense.
elextr commented on this pull request.
@@ -41,6 +41,14 @@ static void on_sidebar_pos_left_changed(GSettings *settings, gchar *key, gpointe
}
+static void on_editor_font_changed(GSettings *settings, gchar *key, gpointer user_data) +{
I was hoping that there was enough similarity between them to maybe have a few callbacks and pass sufficient information via the `user_data`. For example this type of setting would handle all string settings that call a set function, and the user data would need a pointer to the setting of course, and a pointer to a function to call (set to `ui_set_editor_font()` for this setting).
codebrainz commented on this pull request.
@@ -41,6 +41,14 @@ static void on_sidebar_pos_left_changed(GSettings *settings, gchar *key, gpointe
}
+static void on_editor_font_changed(GSettings *settings, gchar *key, gpointer user_data) +{
Here is the difference:
```diff diff --git a/src/settings.c b/src/settings.c index 96e0573..d620595 100644 --- a/src/settings.c +++ b/src/settings.c @@ -38,25 +38,24 @@ GSettings *geany_settings = NULL;
-static void on_sidebar_pos_left_changed(GSettings *settings, gchar *key, gpointer user_data) +static void on_setting_changed(GSettings *settings, gchar *key, gpointer user_data) { - sidebar_set_position_left(g_settings_get_boolean(settings, key)); -} - - -static void on_editor_font_changed(GSettings *settings, gchar *key, gpointer user_data) -{ - g_free(interface_prefs.editor_font); - interface_prefs.editor_font = g_settings_get_string(settings, key); - ui_set_editor_font(interface_prefs.editor_font); -} - - -static void on_symbols_font_changed(GSettings *settings, gchar *key, gpointer user_data) -{ - g_free(interface_prefs.tagbar_font); - interface_prefs.tagbar_font = g_settings_get_string(settings, key); - ui_set_symbols_font(interface_prefs.tagbar_font); + if (g_strcmp0(key, "sidebar-pos-left") == 0) + { + sidebar_set_position_left(g_settings_get_boolean(settings, key)); + } + else if (g_strcmp0(key, "editor-font") == 0) + { + g_free(interface_prefs.editor_font); + interface_prefs.editor_font = g_settings_get_string(settings, key); + ui_set_editor_font(interface_prefs.editor_font); + } + else if (g_strcmp0(key, "symbols-font") == 0) + { + g_free(interface_prefs.tagbar_font); + interface_prefs.tagbar_font = g_settings_get_string(settings, key); + ui_set_symbols_font(interface_prefs.tagbar_font); + } }
@@ -75,9 +74,9 @@ static void settings_bind_main(GSettings *settings) interface_prefs.editor_font = g_settings_get_string(geany_settings, "editor-font"); interface_prefs.tagbar_font = g_settings_get_string(geany_settings, "symbols-font");
- g_signal_connect(settings, "changed::sidebar-pos-left", G_CALLBACK(on_sidebar_pos_left_changed), NULL); - g_signal_connect(settings, "changed::editor-font", G_CALLBACK(on_editor_font_changed), NULL); - g_signal_connect(settings, "changed::symbols-font", G_CALLBACK(on_symbols_font_changed), NULL); + g_signal_connect(settings, "changed::sidebar-pos-left", G_CALLBACK(on_setting_changed), NULL); + g_signal_connect(settings, "changed::editor-font", G_CALLBACK(on_setting_changed), NULL); + g_signal_connect(settings, "changed::symbols-font", G_CALLBACK(on_setting_changed), NULL); }
```
elextr commented on this pull request.
@@ -41,6 +41,14 @@ static void on_sidebar_pos_left_changed(GSettings *settings, gchar *key, gpointe
}
+static void on_editor_font_changed(GSettings *settings, gchar *key, gpointer user_data) +{
The `editor_font` and `symbols_font` do the same thing, so they could be handled the way I proposed above and use the same callback. There can be different callbacks, its not a choice between one callback and one per setting.
And if you are going to select stuff, at least make it a switch on an enum, not a series of strcmps :)
codebrainz commented on this pull request.
@@ -41,6 +41,14 @@ static void on_sidebar_pos_left_changed(GSettings *settings, gchar *key, gpointe
}
+static void on_editor_font_changed(GSettings *settings, gchar *key, gpointer user_data) +{
I'm not sure if such genericity is feasible at this point, though I could be mistaken.
codebrainz commented on this pull request.
@@ -41,6 +41,14 @@ static void on_sidebar_pos_left_changed(GSettings *settings, gchar *key, gpointe
}
+static void on_editor_font_changed(GSettings *settings, gchar *key, gpointer user_data) +{
Yeah, if you passed a pointer to the `interface_prefs` field and a pointer to the updating function to apply the change, it might work. Is it overkill for the present situation though?
And if you are going to select stuff, at least make it a switch on an enum, not a series of strcmps :)
GSettings deals in string keys, though it's possible (probable even) that it turns them into GQuarks for faster comparisons.
elextr commented on this pull request.
@@ -41,6 +41,14 @@ static void on_sidebar_pos_left_changed(GSettings *settings, gchar *key, gpointe
}
+static void on_editor_font_changed(GSettings *settings, gchar *key, gpointer user_data) +{
I don't know, it depends on the number of types of setting handlers needed vs the total number of handlers. Needs "somebody" to do a statistical survey of the settings in Geany. :)
And if you are going to select stuff, at least make it a switch on an enum, not a series of strcmps :)
GSettings deals in string keys, though it's possible (probable even) that it turns them into GQuarks for faster comparisons.
Almost certainly, IIUC its a hash table, and quarks (derived from the schema) are used as the hash values of strings.
But hopefully there will be enough commonality to have a few types of callback to do most of the settings, with the user_data controlling them, and the few which are unique can always have their own specific callback to avoid both switches or comparisons.
If that doesn't work out then fall back to a callback per setting, but if I were doing that it would be riddled with cut and paste errors :)
@codebrainz pushed 2 commits.
85b1a9e Port "show_notebook_tabs" and "msgwin_font" to GSettings af6c91c Port "tab_pos_*" preferences to GSettings
@codebrainz pushed 2 commits.
b279591 Port "show_symbol_list_expanders" pref to GSettings f0c6f43 Port "notebook_double_click_hides_widgets" pref to GSettings
codebrainz commented on this pull request.
@@ -91,6 +91,12 @@ static void on_symbols_tree_expanders_visible_changed(GSettings *settings, gchar
}
+static void on_notebook_double_click_hides_widgets_changed(GSettings *settings, gchar *key, gpointer user_data) +{ + interface_prefs.notebook_double_click_hides_widgets = g_settings_get_boolean(settings, key);
@elextr ones like this (which are just for backwards compat BTW) should be easily made generic/reused in a manner similar to `on_tab_pos_changed` above.
elextr commented on this pull request.
@@ -91,6 +91,12 @@ static void on_symbols_tree_expanders_visible_changed(GSettings *settings, gchar
}
+static void on_notebook_double_click_hides_widgets_changed(GSettings *settings, gchar *key, gpointer user_data) +{ + interface_prefs.notebook_double_click_hides_widgets = g_settings_get_boolean(settings, key);
makes sense
@codebrainz pushed 3 commits.
548d5e2 Port "highlighting_invert_all" pref to GSettings b9ef0e0 Port "msgwin_*_visible" prefs to GSettings 8c12cc3 Port "use_native_windows_dialogs" pref to GSettings
@codebrainz pushed 1 commit.
5a3ec1a A small cleanup/refactoring of settings.c
codebrainz commented on this pull request.
@@ -91,6 +91,12 @@ static void on_symbols_tree_expanders_visible_changed(GSettings *settings, gchar
}
+static void on_notebook_double_click_hides_widgets_changed(GSettings *settings, gchar *key, gpointer user_data) +{ + interface_prefs.notebook_double_click_hides_widgets = g_settings_get_boolean(settings, key);
For example 5a3ec1a2097f116c96f2cf8c34ab90a031e5b5fc
@codebrainz pushed 1 commit.
0ccc5e8 Factor-out common code in settings.c
@codebrainz pushed 6 commits.
817d920 Port "show_white_space" pref to GSettings ee05868 Port "show_indent_guide" pref to GSettings be0c6a8 Port "show_line_endings" pref to GSettings cf237ee Port "show_markers_margin" pref to GSettings e4b4426 Port "show_linenumber_margin" pref to GSettings e115612 Small cleanup in settings.c after last few ported settings
@codebrainz pushed 3 commits.
9e10769 Add some settings helper functions for core and plugins 2aa0673 Make use of new settings helper functions in core 3c99f18 Make use of settings functions in splitwindow
@codebrainz Do I understand it correctly that by adopting GSettings Geany configuration will be stored into a binary "registry"-like form by dconf? Worse, if the last answer in
http://askubuntu.com/questions/249887/gconf-dconf-gsettings-and-the-relation...
is true, it will be stored in a completely different form on each platform so the settings couldn't be easily read and transferred? I'd find this really unfortunate and if this is really the case, I'd prefer not migrating to GSettings (config files should be text-based and user-editable IMO).
@techee no, I have [used the GKeyfile backend](https://github.com/geany/geany/pull/1257/files#diff-cfb42647f460cb87f3decb6b...) so it's still just a plain text config/ini file in the same directory as before.
@techee no, I have used the GKeyfile backend so it's still just a plain text config/ini file in the same directory as before.
In that case I'm happy :-)
@codebrainz what's the status on this?
@kugel- not sure, it's a lot of (simple) work to finish all the settings, I kind of stopped until I got confirmation that I wasn't wasting my time. It's basically a proof of concept at this point.
Also some of the editor settings and others which are hacked to work for projects and general have very weird code I couldn't easily understand in order to untangle. I think that was the only real challenge.
@codebrainz I made a PR to your PR (https://github.com/codebrainz/geany/pull/1) to merge from master and a minor fix on error message (14c33d4). You might want to do the merge yourself though, the conflict resolution is trivial.
The use of GKeyFile is neat here; it seems to work fine from the little testing I did.
Closed #1257.
I'm closing this since I'm no longer working on it.
github-comments@lists.geany.org