Closes #1763.
I introduced an `enum` in `src/keyfile.c` called `ConfigPayload` which determines whether to read/write preference-data or session-data.
The strings `"geany.conf"` and `"session.conf"` are now defined as preprocessor macros `PREFERENCES_FILE` and `SESSION_FILE`, respectively. However, the string `"geany.conf"` still appears in `src/keybindings.c` and `src/libmain.c`.
For the sake of backwards compatibility, if `session.conf` does not exist, the data is read from `geany.conf` instead. However, the old session data is not cleared from `geany.conf` when this happens. You can view, comment on, or merge this pull request online at:
https://github.com/geany/geany/pull/2776
-- Commit Summary --
* split config file into preferences and session * read rec files from geany.conf if no session.conf
-- File Changes --
M src/keyfile.c (99)
-- Patch Links --
https://github.com/geany/geany/pull/2776.patch https://github.com/geany/geany/pull/2776.diff
@abmorris pushed 1 commit.
ea5356baee1f9511ab72d27a1d73f3411a4ef608 minor: update comment
@abmorris pushed 1 commit.
b3abe17e696210da6ed22a33bb28c9ea8008c2fb more careful use of the new macros
Would need an option (default off) because this change is not backward compatible with older versions of Geany, it would need to be the users choice to turn it on.
The change is backwards compatible, existing session from geany.conf is converted. No need for a user setting in my opinion.
What's missing is the same treatment for projects. I think open project files can also be saved in session.conf, with the project name as the key (or some other unique identifier). The project files can then be checked into VCS, and the corresponding "open project files" list does not clutter the checkouts.
In general I like the implementation, it's a pain point with a very long history and the patch is actually quite small and elegant. I support merging it in the end.
@kugel- requested changes on this pull request.
@@ -1187,6 +1214,22 @@ gboolean configuration_load(void)
}
+gboolean configuration_load(void) +{ + gboolean prefs_loaded = read_config_file(PREFERENCES_FILE, PREFERENCES); + /* backwards-compatibility: try to read session from preferences if session file doesn't exist */ + gchar *session_filename = SESSION_FILE; + gchar *session_file = g_build_filename(app->configdir, session_filename, NULL);
`g_free(session_file)` is missing
- load_dialog_prefs(config);
- load_ui_prefs(config); - project_load_prefs(config); - configuration_load_session_files(config, TRUE); - + switch (payload) + { + case PREFERENCES: + load_dialog_prefs(config); + load_ui_prefs(config); + project_load_prefs(config); + break; + case SESSION: + configuration_load_session_files(config, TRUE); + break; + } /* this signal can be used e.g. to delay building UI elements until settings have been read */ g_signal_emit_by_name(geany_object, "load-settings", config);
The signal should be emitted only once, arguably for the PREFERENCES payload. Might add another signal for load-session ?
{
GKeyFile *config = g_key_file_new(); - gchar *configfile = g_build_filename(app->configdir, "geany.conf", NULL); + gchar *configfile = g_build_filename(app->configdir, filename, NULL); gchar *data;
g_key_file_load_from_file(config, configfile, G_KEY_FILE_NONE, NULL);
/* this signal can be used e.g. to prepare any settings before Stash code reads them below */ g_signal_emit_by_name(geany_object, "save-settings", config);
The signal should be emitted only once, arguably for the PREFERENCES payload. Might add another signal for save-session?
@elextr commented on this pull request.
{
GKeyFile *config = g_key_file_new(); - gchar *configfile = g_build_filename(app->configdir, "geany.conf", NULL); + gchar *configfile = g_build_filename(app->configdir, filename, NULL); gchar *data;
g_key_file_load_from_file(config, configfile, G_KEY_FILE_NONE, NULL);
/* this signal can be used e.g. to prepare any settings before Stash code reads them below */ g_signal_emit_by_name(geany_object, "save-settings", config);
Probably ok with just the prefs, not aware of anything in Geany or any plugins that use the `save-settings` for getting the session or MRUs. Since they would have to change to the new signal anyway, leave it for now and add if they ask.
@kugel- agree its useful (see https://github.com/geany/geany/issues/1763#issuecomment-364107685) but its not backward compatible, an old geany would not find the session file, we often get situations where users have differing versions of Geany that are sharing files (two machines with different distros for eg). So a setting is needed (despite how common it is, me saying "needs a setting" isn't automatic, I do actually think about it :-).
What's missing is the same treatment for projects. I think open project files can also be saved in session.conf, with the project name as the key (or some other unique identifier). The project files can then be checked into VCS, and the corresponding "open project files" list does not clutter the checkouts.
Its necessary to have the whole config directory inside the VCS tree, not just `geany.conf`, because its only possible to specify the whole config (with -c) not `geany.conf` by itself, and that then lets the OP change and share _all_ settings including keybindings and those in filetypes and geany.css and tags for libraries they use etc, on a per repo (ie per project) basis, and they then have no need for Geany projects which have only a few settings and the session so its outside this use-case.
But it raises the point that the open project is saved to `geany.conf` that probably needs to move into the session too.
If they did want to change projects even if its outside their use-case there doesn't seem any reason not to split into `project_name.geany` and `project_name.geany.session` next to each other in the same directory, `.gitignore` could be taught to ignore it and it works for projects stored outside the tree too. Not sure if that would break the project plugins though?
@kugel- agree its useful (see [#1763 (comment)](https://github.com/geany/geany/issues/1763#issuecomment-364107685)) but its not backward compatible, an old geany would not find the session file, we often get situations where users have differing versions of Geany that are sharing files (two machines with different distros for eg). So a setting is needed (despite how common it is, me saying "needs a setting" isn't automatic, I do actually think about it :-).
This is not a supported use case as of today. geany.conf is really only usable by one Geany instance *even on the same machine*. Actually this PR is about to support to share geany.conf between machines which is not a thing right now.
Please don't make up use cases that aren't actually properly supported right now to raise the bar for PRs like this.
Anyway, I maintain that this change is backwards compatible. Newer Geany converts *its* session to the new file, hypothetical older Geany keep reading and writing the session files from geany.conf (the session file list isn't yet cleared from geany.conf).
What's missing is the same treatment for projects. I think open project files can also be saved in session.conf, with the project name as the key (or some other unique identifier). The project files can then be checked into VCS, and the corresponding "open project files" list does not clutter the checkouts.
Its necessary to have the whole config directory inside the VCS tree, not just `geany.conf`, because its only possible to specify the whole config (with -c) not `geany.conf` by itself, and that then lets the OP change and share _all_ settings including keybindings and those in filetypes and geany.css and tags for libraries they use etc, on a per repo (ie per project) basis, and they then have no need for Geany projects which have only a few settings and the session so its outside this use-case.
I'm confused. For the "dotfiles" use-case, sure, geany.conf alone is not enough. But this PR helps to not clutter the "dotfiles" repo with machine-specific paths from the session files.
For projects, only the project.geany should be needed in VCS of that project. And it makes sense to have it since there are some settings in project.geany that override geany.conf and a corresponding open files list (which is not stored in the VCS).
But it raises the point that the open project is saved to `geany.conf` that probably needs to move into the session too.
Good point, I agree. I think that implies the whole "recent projects" list. And I guess over time we will find more stuff that's better placed in the session.conf
If they did want to change projects even if its outside their use-case there doesn't seem any reason not to split into `project_name.geany` and `project_name.geany.session` next to each other in the same directory, `.gitignore` could be taught to ignore it and it works for projects stored outside the tree too. Not sure if that would break the project plugins though?
IMO geany should not junk files into a project, other than project.geany which is explicitly asked for. If I want to cleanup geany state files I think only of $HOME/.config/geany, I wouldn't go and search my whole computer for project session files. Therefore I stand by my opinion that project open file lists must be stored in $HOME/.config/geany once they move out of the project files.
This is not a supported use case as of today. geany.conf is really only usable by one Geany instance even on the same machine. Actually this PR is about to support to share geany.conf between machines which is not a thing right now.
I wasn't implying that we support concurrent use of a config directory, but socket has been moved out of the config directory so storing the config directory on a non-linux filesystem on a fileserver is perfectly acceptable. And as I said its not possible to share `geany.conf` by itself unless you are going to copy it into/from a config directory each time, you have to share the config directory itself.
Newer Geany converts its session to the new file, hypothetical older Geany keep reading and writing the session files from geany.conf (the session file list isn't yet cleared from geany.conf).
True, but removing them is on the OPs bugs checklist at the beginning of the PR, and then it will not be backward compatible.
And I guess over time we will find more stuff that's better placed in the session.conf
True
I wouldn't go and search my whole computer for project session files
Why would you be looking all over the computer? If its next to the project file you delete it at the same time as you delete the project file. If you are not deleting the project why are you deleting the project session?
Therefore I stand by my opinion that project open file lists must be stored in $HOME/.config/geany once they move out of the project files.
And lots of users will end up with accumulating session files because they don't delete them.
But more importantly there is an option for project files to be stored in the project base directory, separating session files when that is selected doesn't make sense and breaks current usage.
True, but removing them is on the OPs bugs checklist at the beginning of the PR, and then it will not be backward compatible.
It is less work for me not to implement this, if you prefer.
It leaves the file a bit untidy, but it should be harmless.
@abmorris pushed 2 commits.
c0303c846024134dd7b8310f5e32a50d46170a7f add missing g_free statement e406c69fe42be285a776e2eaf2888be405df5c7c emit signals only when reading/writing preferences
@abmorris commented on this pull request.
@@ -1187,6 +1214,22 @@ gboolean configuration_load(void)
}
+gboolean configuration_load(void) +{ + gboolean prefs_loaded = read_config_file(PREFERENCES_FILE, PREFERENCES); + /* backwards-compatibility: try to read session from preferences if session file doesn't exist */ + gchar *session_filename = SESSION_FILE; + gchar *session_file = g_build_filename(app->configdir, session_filename, NULL);
Added in c0303c84
@abmorris commented on this pull request.
- load_dialog_prefs(config);
- load_ui_prefs(config); - project_load_prefs(config); - configuration_load_session_files(config, TRUE); - + switch (payload) + { + case PREFERENCES: + load_dialog_prefs(config); + load_ui_prefs(config); + project_load_prefs(config); + break; + case SESSION: + configuration_load_session_files(config, TRUE); + break; + } /* this signal can be used e.g. to delay building UI elements until settings have been read */ g_signal_emit_by_name(geany_object, "load-settings", config);
Added in e406c69f (also fixed the "double vision" document pane bug). I'll leave this unresolved for you to check over the diff.
@abmorris commented on this pull request.
{
GKeyFile *config = g_key_file_new(); - gchar *configfile = g_build_filename(app->configdir, "geany.conf", NULL); + gchar *configfile = g_build_filename(app->configdir, filename, NULL); gchar *data;
g_key_file_load_from_file(config, configfile, G_KEY_FILE_NONE, NULL);
/* this signal can be used e.g. to prepare any settings before Stash code reads them below */ g_signal_emit_by_name(geany_object, "save-settings", config);
Added in e406c69f (also fixed the "double vision" document pane bug). Like with the "load-settings" case, I'll leave this unresolved for you to check over the diff.
And I guess over time we will find more stuff that's better placed in the session.conf
I can make three suggestions now: ``` [geany] msgwindow_position treeview_position geometry ``` It looks like these can be extracted from a single if-statement in `save_ui_prefs()`.
Of course saving these can be disabled: ``` [geany] pref_main_save_winpos=false pref_main_save_wingeom=false ``` so it's not the highest priority if my goal is to make `geany.conf` dotfiles-friendly.
Yea, lets concentrate on the session and project file lists for this PR. I think for regular prefs we might want a generic method to specify which config they belong in (maybe this further complicated by prefs that can be overridden by projects).
Will you work on separating the project file list as well (so that the open files per project is not stored in the project.geany files anymore)? If so, please make a suggestion (possibly by doing a proof-of-concept) of where to store those. You see there is disagreement between @elextr and me, and since I don't feel overly strong I would like to leave that to your judgment for now, as you're the implementer.
I agree that separating the project file a natural next step, but I would have to spend some time getting familiar with `project.c`. I'm back at work tomorrow after the long Easter weekend, so it might take a while to come up with a proof of concept. I would lean towards keeping the project session files next to the project files, but I don't think this has a huge impact on the implementation (just change how the path to the file is built, right?).
If it's OK with you, I would like to move that to a separate issue and PR. I think what we have here my changes to `keyfile.c` is fairly self-contained and in a good state.
I am removing "WIP:" and I believe there is one outstanding point to be addressed: should geany clear the old session data from `geany.conf` + introduce a preference to enable this new behaviour, or should it just leave it? `dotfiles` users are presumably already used to dealing with exactly this themselves. A "clear history" button (as suggested in https://github.com/geany/geany/issues/1763#issuecomment-364176848) which checks `geany.conf` and (deletes?) `session.conf` would take care of this, but I see that as a separate feature for another PR.
If it's OK with you, I would like to move that to a separate issue and PR. I think what we have here with keyfile.c is fairly self-contained and in a good state.
That's OK but I wouldn't really want to merge the one without the other (not into master, at least). Not sure if github can handle inter-dependent PRs. Maybe have to merge this to a staging branch so that your other PR can use that as a base.
I would lean towards keeping the project session files next to the project files, but I don't think this has a huge impact on the implementation (just change how the path to the file is built, right?).
Maybe. For the most part it's a matter of taste I guess. But, keep in mind that projects can be stored anywhere, e.g. also in a $HOME/projects that is just a pool of *.geany files with no relation to the actual project base directory. I might not want the session files in that pool directory. On the other hand, when it's in the project (to be checked into VCS), then you probably want it to be a dot-file so it's hidden (and add it to .gitignore), but then it's easy to forget to delete them. Many question marks around that, for me it would be simpler and natural to store machine-specific meta-stuff like this centralized location within $HOME/.config/geany. I would prefer a single key-file with the project name or some other uniq identifier as keys. Perhaps with a timestamp so we can do GC on the file.
But it's up to you.
@abmorris
True, but removing them is on the OPs bugs checklist at the beginning of the PR, and then it will not be backward compatible.
It is less work for me not to implement this, if you prefer.
It leaves the file a bit untidy, but it should be harmless.
Ok, sessions won't share between versions, but at least old versions won't get their sessions deleted, so no option is ok.
``` [geany] msgwindow_position treeview_position geometry ```
Yes, geometry relates to the configuration of the machine, moving from a system with two screens to one with one screen or large screen to small screen can result in Geany being offscreen, so its a setting that should stay with the system, ie session (thats why the options to not save it IIRC).
Also search dialog location `[search] position_*` is the same, its a separate window and might be offscreen, we have had issues where people unplugged their second monitor and lost it.
It would more than likely be useful if those were moved to session, but its up to you, if all your sharing machines are identical you won't need it. Could also be a separate PR to keep this small.
The internal window positions are less important, your choice if you do them now.
A "clear history" button (as suggested in #1763 (comment)) which checks geany.conf and (deletes?) session.conf would take care of this, but I see that as a separate feature for another PR.
Yes, if someone needs it, they can make a PR later
@kugel-
Yea, lets concentrate on the session and project file lists for this PR.
+1 someone doing a separate PR for projects when we get our ducks in a row is fine, but it never hurts to raise it
So lets not pollute this PR with that discussion any more and we can continue it on another issue/PR when (if) it gets created.
IIRC there have been undecided discussions before raising many of the same things, that we probably need to revisit, I vaguely remember some incredibly complex suggestion I had at one point :grin:.
As @abmorris said, this is stand alone, and I will add smaller PRs are more likely to get merged quickly.
If this gets merged in its current state (to a staging branch or master, whichever) do we consider #1763 closed?
@kugel- requested changes on this pull request.
Almost. In principle I approve the change in the current state.
@@ -628,34 +631,44 @@ static void save_ui_prefs(GKeyFile *config)
} }
+typedef enum ConfigPayload +{ + PREFERENCES,
Minor nitpick: Can you please shorten this to PREFS (and above PREFERENCES_FILE -> PREFS_FILE)? We usually refer to the configuration as "prefs" throughout the source and this would follow the pattern.
I'll probably squash the commits when merging, if that's OK?
If this gets merged in its current state (to a staging branch or master, whichever) do we consider #1763 closed?
Github will probably close this once the commit lands in the main repo in whatever branch. Personally i think no issue is truly closed until the fix is actually *released*.
@abmorris pushed 1 commit.
df0673349be6c7a63f4fd795492cee849febf4a9 minor: PREFERENCES->PREFS
@abmorris commented on this pull request.
@@ -628,34 +631,44 @@ static void save_ui_prefs(GKeyFile *config)
} }
+typedef enum ConfigPayload +{ + PREFERENCES,
Done in df06733
Yes, squashing the commits is fine by me. I just removed the "closes" keyword from the PR description so the issue shouldn't be closed automatically.
@kugel- approved this pull request.
thanks
@kugel- requested changes on this pull request.
So I tested the change. There is one more request (sorry about not coming up with that earlier!):
Since the recent projects is also in session.conf, I think the `[project]` stuff should also go into session.conf. That is the current project and the path to the .geany file.
There is a detail that I found during testing but I don't think it needs to be handled now:
Once the session.conf exists, it will be read exclusively for the expected settings. So when we move things to session.conf afterwards, then session.conf won't have them yet and geany.conf won't be considered for that, i.e. the prefs are lost.
This has happened to me when I tested a small change to move `[project]` to session.conf, after a previous run already created the file (without `[project]` section)
If that turns out to be a problem after we actually release this we might need some versioning to be able to rescue prefs during the move.
Agree with @kugel- that searching geany.conf for settings missing in session.conf when other things are added to session should be left to the PRs that make that change, although it actually suggests a better implementation which uses both similar to the way both system config and user config are searched (almost) everywhere. Probably could use the existing helpers for that too.
I have an idea how we can deal with that, but it won't be part of this PR. I'll merge this PR now into a new session_split branch and will do the project_config config change on my own, since it's really trivial.
I hope we get the PR that moves the project open files out of project.geany soon; also we have time until release to identify more settings that are more suitable in the session.conf
@kugel- approved this pull request.
Merged #2776 into session_split.
github-comments@lists.geany.org