The main idea is to save the session file list more often to prevent accidental lost but saving the rest of the configuration might help as well. To prevent too many save attempts, an idle function is used and it's only added once until it was executed.
This might help #1826, #1426 and replaces #1860.
IIRC at the very beginning I was a bit concerned about IO access and performance when writing the settings too often. At least performance doesn't seem to be a problem: `configuration_save` takes about 1-2 milliseconds on my system and IO access happens in the document-related actions in any way.
Even though I tested the code, I would like to use it "in production" for some time to get sure there are no unseen side effects or similar. You can view, comment on, or merge this pull request online at:
https://github.com/geany/geany/pull/2114
-- Commit Summary --
* Save main and project configuration whenever documents are opened/closed
-- File Changes --
M src/keyfile.c (38)
-- Patch Links --
https://github.com/geany/geany/pull/2114.patch https://github.com/geany/geany/pull/2114.diff
Also related are #51 and #52.
At least performance doesn't seem to be a problem: configuration_save takes about 1-2 milliseconds on my system and IO access happens in the document-related actions in any way.
I guess the only issue is if the config directory is on a slow drive like a network share or something. We could try it and if it causes people problems in reality, maybe look at adding a preference to disable it. I can't see it being much of an issue though as most programs do similar things.
The only thing I don't like, which could be improved later, is that it's still pretty arbitrary where the settings are being saved. In a perfect world, it would queue a save whenever the contents of the config file need to be saved.
👍
Oh my, all those people who have two Geanys using the same config are gonna have many more races :grin:
And an occasional failure when both try to write at the same time (on those file systems where such access is exclusive).
The only thing I don't like, which could be improved later, is that it's still pretty arbitrary where the settings are being saved.
If by "where" you mean "when" then I thought settings are now pretty much always saved when they are changed, its only the session that saved at the end, and thats what this addresses. Of course because of the way g_key_files work the whole lot has to be saved.
But as @codebrainz points out there are potential problems with slow or flakey drives maybe there needs to be a setting to disable it instead of forcing it on everyone.
Of course since it only writes to `geany_debug()` on error nobody will notice its failing I guess.
Agree best to commit immediately after next release.
If by "where" you mean "when" then I thought settings are now pretty much always saved when they are changed
I mean this PR only adds more places where configuration is saved (compared to only on prefs dialog applied, plugin manager closed, app closed, etc.) but it's still not comprehensive. In a perfect world there would be an interface/abstraction for the config system that whenever any setting is changed, a save is automatically queued, similar to something like Xfconf or GSettings or such. To give an example, even after this PR, if you used `View->Show Indentation Guides`, that setting still wouldn't be saved until one of the arbitrary places which saves the configuration is hit.
if you used View->Show Indentation Guides, that setting still wouldn't be saved until one of the places which saves the configuration is hit.
Sure, its not comprehensive yet, pull requests are welcome.
@eht16 maybe this should be in the autosave plugin since it will be most useful when the files that its saving in the session are themselves being saved.
@codebrainz I agree that the current implementation is quite arbitrary about when to save. Though, in daily work, I guess this fits for most users. And yes, a more general config system (as you mentioned) with notices and a queue would be awesome but is a whole another story.
@elextr this PR doesn't address the issue with multiple instances which overwrite the config mutually. I think we should stop writing the config at all in new instances but again, another story.
@elextr I'm not sure how useful a setting for this feature would be. the config we write is only a few kilobytes and I have no clue if there are still any users out there who actually store configs on network shares or the like.
Oh, and I think this should *not* go into the SaveActions plugin as the plugin is for (auto) saving documents not Geany's settings. And actually, the code of this PR is triggered by the SaveActions plugin already via the "document-save" signal. So what you request is already done :).
Agree that multiple instances sharing the same config or project is a separate problem. I always try to discourage people running more than one geany using the same config or project, thats why I had the evil grin icon on the comment :grin:
I have no clue if there are still any users out there who actually store configs on network shares or the like.
Given that NAS appliances are now only a few hundred dollars and as the US _consumer_ market was [half a billion dollars in 2017 and growing](https://www.statista.com/statistics/750899/united-states-consumer-network-at...) I suspect that the numbers of user configs and project files on network devices may be increasing, not reducing.
And actually, the code of this PR is triggered by the SaveActions plugin already via the "document-save" signal. So what you request is already done :).
I do hope your aggregation algorithm works. :grinning:
To be clear, In general this change is a good thing to do, I'm just trying to anticipate any issues, and would like having the "off" option hidden in various as a workaround for the ones we didn't think of.
@eht16 pushed 1 commit.
b07c090b881b60d14c5d7ce514386e5bae4796b9 Add setting, defaulting to TRUE, to disable automatic configuration save
Ok, https://github.com/geany/geany/pull/2114/commits/b07c090b881b60d14c5d7ce5143... brings you a nice new setting. As you are a grateful guy, you probably want to check the documentation change and tell me if it can be phrased better :).
elextr commented on this pull request.
@@ -2636,6 +2636,16 @@ reload_clean_doc_on_file_change Whether to automatically reload documents f
on disk. If unsaved changes exist then the user is prompted to reload manually. +save_config_on_file_change Automatically save Geany's configuration true immediately + to disk once the document list changes + (i.e. new documents are opened, saved or + closed). This helps to accidentally lose
I hope it doesn't help to lose stuff, maybe ... to prevent accidentally losing ...
you probably want to check the documentation change and tell me if it can be phrased better :).
elextr licks lips, extracts purple editing pencil from behind ear ... two comments.
also LGBI
@eht16 pushed 1 commit.
59317693e36680962fc5bb0956a031e4aa9f213f Reword the doc text to not tell what we want to prevent
eht16 commented on this pull request.
@@ -2636,6 +2636,16 @@ reload_clean_doc_on_file_change Whether to automatically reload documents f
on disk. If unsaved changes exist then the user is prompted to reload manually. +save_config_on_file_change Automatically save Geany's configuration true immediately + to disk once the document list changes + (i.e. new documents are opened, saved or + closed). This helps to accidentally lose
Ouch. Thanks for spotting this, I always appreciate your editing pencil!
Seems to work fine, used it in the last weeks and noticed no problems or side effects so far.
Hmm, travis seems to have a problem with that unrelaible download.geany.org :)
Fixed by restarting the build and now it's green again :)
b4n commented on this pull request.
Looks mostly good, see comments.
@@ -59,7 +59,7 @@ G_BEGIN_DECLS
* @warning You should not test for values below 200 as previously * @c GEANY_API_VERSION was defined as an enum value, not a macro. */ -#define GEANY_API_VERSION 239 +#define GEANY_API_VERSION 240
Why bumping API?
void configuration_init(void)
{ keyfile_groups = g_ptr_array_new(); pref_groups = g_ptr_array_new(); init_pref_groups(); + + g_signal_connect(geany_object, "document-new", G_CALLBACK(document_list_changed_cb), NULL); + g_signal_connect(geany_object, "document-open", G_CALLBACK(document_list_changed_cb), NULL); + g_signal_connect(geany_object, "document-save", G_CALLBACK(document_list_changed_cb), NULL);
It would be nice to avoid saving the conf when saving a file in the common case, e.g. when just updating the file's content. AFAIK, we don't affect the session then, do we? My point is that I'm usually saving my files very often, and there's no use of saving the config then, only if I saved as another name or such, is there? I agree it's probably cheap to save it anyway (although it might matter a little on SSDs?), but if it's of no use at all I'd rather see it avoided.
Not sure how to implement that though… the only thing I can think of is to check in `:document-before-save` that `doc->real_path == NULL` and then set a flag to handle that one in `:document-save`, for when saving is successful? Or just don't bother to check if save worked, and just do it in `:document-before-save` with the check, as the likely case is a successful save anyway.
--- Untested possible implementations:
The simple solution: ```diff +static void document_before_save_cb(GObject *obj, GeanyDocument *doc, gpointer data) +{ + if (! doc->real_path) + document_list_changed_cb(obj, doc, data); +} + /* … */ - g_signal_connect(geany_object, "document-save", G_CALLBACK(document_list_changed_cb), NULL); + g_signal_connect(geany_object, "document-before-save", G_CALLBACK(document_before_save_cb), NULL); /* … */ ```
The convoluted solution:
```diff +static void document_before_save_cb(GObject *obj, GeanyDocument *doc, gpointer data) +{ + gboolean *saved_as = data; + *saved_as = doc->real_path == NULL; +} + +static void document_save_cb(GObject *obj, GeanyDocument *doc, gpointer data) +{ + gboolean *saved_as = data; + if (*saved_as) + document_list_changed_cb(obj, doc, NULL); +} + /* … */ + gboolean *saved_as_p = g_malloc(sizeof *saved_as_p); + g_signal_connect_data(geany_object, "document-before-save", G_CALLBACK(document_before_save_cb), saved_as_p, (GClosureNotify) g_free, 0); - g_signal_connect(geany_object, "document-save", G_CALLBACK(document_list_changed_cb), NULL); + g_signal_connect(geany_object, "document-save", G_CALLBACK(document_list_changed_cb), saved_as_p); /* … */ ```
elextr commented on this pull request.
void configuration_init(void)
{ keyfile_groups = g_ptr_array_new(); pref_groups = g_ptr_array_new(); init_pref_groups(); + + g_signal_connect(geany_object, "document-new", G_CALLBACK(document_list_changed_cb), NULL); + g_signal_connect(geany_object, "document-open", G_CALLBACK(document_list_changed_cb), NULL); + g_signal_connect(geany_object, "document-save", G_CALLBACK(document_list_changed_cb), NULL);
Well, the session includes cursor position so its probably worth saving.
elextr commented on this pull request.
@@ -1313,11 +1316,47 @@ void configuration_apply_settings(void)
}
+static gboolean save_configuration_cb(gpointer data) +{ + configuration_save();
Just continuing @b4n's theme, do we need to save both config and project? The active session is in the project, not the config.
elextr commented on this pull request.
@@ -1313,11 +1316,47 @@ void configuration_apply_settings(void)
}
+static gboolean save_configuration_cb(gpointer data) +{ + configuration_save();
And on that note, shouldn't the config be saved on project open/close as well, although it will often happen anyway because files will be opened/closed when a project opens, but not always.
What's missing on this? Want this desperately.
I want to work on the open feedback to get this finished. Last weeks I was very limited on free time for this because of other priorities and few spare time. But this will change soon, so I'm confident we will get this ready to merge before 1.36.
ntrel commented on this pull request.
void configuration_init(void)
{ keyfile_groups = g_ptr_array_new(); pref_groups = g_ptr_array_new(); init_pref_groups(); + + g_signal_connect(geany_object, "document-new", G_CALLBACK(document_list_changed_cb), NULL); + g_signal_connect(geany_object, "document-open", G_CALLBACK(document_list_changed_cb), NULL); + g_signal_connect(geany_object, "document-save", G_CALLBACK(document_list_changed_cb), NULL);
Why is document-new connected to? If there's no file yet it won't affect the session.
It would be good to reduce file writes e.g. to prolong hard disk life. What if we track when session data changes but only write the keyfile when the main window loses focus? That would seem to solve the user clicking logout problem and drastically reduce disk/network writes.
I don't think its neccessary to worry about disk IO unless its having an effect on performance, but since save only occurs when config data changes and that is when there is a major screen update (new document, close document etc) its not likely to be noticed. If it does cause performance issues, eg because the config file is on some slow remote filesystem then there is now an option to turn this saving off.
As for saving disk life, on my systems something in the OS writes to disk every few seconds (with no user apps open), so I doubt the Geany config saving will have a material effect on lifetime.
Only aggregating and only saving on focus loss is ok for user logout since it will be hard to invoke logout or shutdown without de-focussing geany, but it will not handle a crash though.
eht16 commented on this pull request.
@@ -59,7 +59,7 @@ G_BEGIN_DECLS
* @warning You should not test for values below 200 as previously * @c GEANY_API_VERSION was defined as an enum value, not a macro. */ -#define GEANY_API_VERSION 239 +#define GEANY_API_VERSION 240
Because of the new member in `GeanyFilePrefs` which is part of the API.
eht16 commented on this pull request.
@@ -1313,11 +1316,47 @@ void configuration_apply_settings(void)
}
+static gboolean save_configuration_cb(gpointer data) +{ + configuration_save();
Just continuing @b4n's theme, do we need to save both config and project? The active session is in the project, not the config.
Maybe we do not need to save the config if a project is open regarding the session files but for all other maybe changed settings and I think it won't hurt(except disk IO) to leave a consistent state on disk.
And on that note, shouldn't the config be saved on project open/close as well, although it will often happen anyway because files will be opened/closed when a project opens, but not always.
In my tests, the `save_configuration_cb()` was always called on project related actions. What exact cases do you refer to by "not always"?
@eht16 pushed 1 commit.
d6ebff6c4f6c04786e5448735ac59462e2f7619a Remove probably unnecessary "document-new" signal handling
eht16 commented on this pull request.
void configuration_init(void)
{ keyfile_groups = g_ptr_array_new(); pref_groups = g_ptr_array_new(); init_pref_groups(); + + g_signal_connect(geany_object, "document-new", G_CALLBACK(document_list_changed_cb), NULL); + g_signal_connect(geany_object, "document-open", G_CALLBACK(document_list_changed_cb), NULL); + g_signal_connect(geany_object, "document-save", G_CALLBACK(document_list_changed_cb), NULL);
It would be nice to avoid saving the conf when saving a file in the common case, e.g. when just updating the file's content. AFAIK, we don't affect the session then, do we? My point is that I'm
Hmm, I'm not sure if checking `doc->real_path` for `NULL` is sufficient, there might be more cases when we want to trigger the config save. Also, the `doc->real_path` check would have to differentiate cases like renaming and closing documents. The config save process is pretty lightweight and, as said initially, very fast. Users trying to reduce disk IO still can disable this feature.
Can we agree to implement more sophisticated checks independent of this PR afterwards if someone wants to?
Why is document-new connected to? If there's no file yet it won't affect the session.
Removed in https://github.com/geany/geany/pull/2114/commits/d6ebff6c4f6c04786e5448735ac.... Probably not necessary, I just added it for completeness.
elextr commented on this pull request.
@@ -1313,11 +1316,47 @@ void configuration_apply_settings(void)
}
+static gboolean save_configuration_cb(gpointer data) +{ + configuration_save();
"not always"
On further consideration since any project action closes one set of files and opens another (even if they are the same files) there will be file open/close signals in all cases except the corner case where no files are open before and after the project open/close, so don't worry about it I think.
I agree with @elextr: better saving the config too often than too few. The primary intention is to save data loss on crashes (of Geany or the host). Users concerning their disk IO can disable the feature. As said above, if you or anyone else want, let's improve the feature afterwards. The current implementation will probably be already sufficient and help a lot most of the users.
As said above, if you or anyone else want, let's improve the feature afterwards...
:+1:, I might even work on this (again), either like #1257 or something a little less invasive.
@eht16 what's missing on this? I'm too tired of losing my open files because my laptop doesn't resume from suspend properly after 2 weeks of uptime.
kugel- approved this pull request.
@kugel- <ctrl>s? On maybe test this PR and say its working for you.
But in case its not clear I'm happy with this as is, but with the caveat that I havn't tested it.
What do you refer to with `<ctrl>s` ?
@kugel- "save more often" :) but the point is, if you think this is gonna help you and you want it, why not test it. Although it doesn't actually save your files, at least you would have the list of the ones you lost.
The point of this is to save the session list more often, and that's what I'm looking for. I save often enough to not lose contents.
Anyway, I am testing this at the moment, so far it works as advertised.
Oh, ok, I read "I'm too tired of losing my open files" as you hadn't saved them.
ntrel commented on this pull request.
@@ -59,7 +59,7 @@ G_BEGIN_DECLS
* @warning You should not test for values below 200 as previously * @c GEANY_API_VERSION was defined as an enum value, not a macro. */ -#define GEANY_API_VERSION 239 +#define GEANY_API_VERSION 240
AFAIK, anything without a doc-comment shouldn't be considered part of the API.
eht16 commented on this pull request.
@@ -59,7 +59,7 @@ G_BEGIN_DECLS
* @warning You should not test for values below 200 as previously * @c GEANY_API_VERSION was defined as an enum value, not a macro. */ -#define GEANY_API_VERSION 239 +#define GEANY_API_VERSION 240
Hmm, ok. Removed. I still consider it that bad as it is only a bump indicating there are changes (and in this case only additions). It's not like as we would change the ABI. Anyway, I kept the API version as is.
@kugel- I was waiting for feedback after I answered the review feedback of @ntrel, @elextr and @b4n. I just squashed the commits after I removed the API version bump. Only @b4n's voice missing.
@eht16 I just noticed that the plugin API description of the `project_save` signal lists when it happens, I presume this needs to be added to that list.
@elextr what do you mean exactly? Should we add a listener on `project-save` or should we extend the documentation of `project-save`?
Documentation I meant. The description of `plugin_save` on [this](https://www.geany.org/manual/reference/pluginsignals_8c.html#ac093c4c2a6ef1e...) page that lists when it happens.
@eht16 pushed 1 commit.
41227a4084879de554cda8e982229ed788847353 Document new emission of "project-save" signal
Ahhh, good catch! Thanks, see 41227a4.
@b4n any other feedback? Would you be fine with merging this PR and let @codebrainz or whoever do the rest later?
codebrainz approved this pull request.
Unfortunately this wasn't merged, even with two approvals and the target milestone 1.36. I'd say merge this quickly so we don't forget it again ;-) @eht16 ?
@eht16 pushed 1 commit.
1a558f5cc9e362586da864f1cf7ee80fa1951a32 Merge branch 'master' into save_config_on_doclist_change
I'm fine with merging. Just solved the merge conflict. If nobody beats me by time or with feedback, I'll merge this on Sunday.
Hopefully you'll squash or rebase to avoid the back merge?
@eht16 pushed 3 commits.
8763d59c7ab9612bf7af1479c6ed964fd7acbe3d Save main and project configuration whenever documents are opened/closed f1f7d9ebe5a153e508394fec9eb03a1bbe5739e1 Add setting, defaulting to TRUE, to disable automatic configuration save 3a9ae46e2ea88a0f1ad734913881b01e6051e672 Document new emission of "project-save" signal
Hopefully you'll squash or rebase to avoid the back merge?
Done.
Great. FWIW, I've been using this patch since a while and have no problems so far.
Merged #2114 into master.
github-comments@lists.geany.org