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

Save main and project configuration whenever documents are opened/closed #2114

Open
wants to merge 3 commits into
base: master
from

Conversation

@eht16
Copy link
Member

commented Mar 28, 2019

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, #1416 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.

@codebrainz

This comment has been minimized.

Copy link
Member

commented Mar 28, 2019

Also related are #51 and #52.

@codebrainz

This comment has been minimized.

Copy link
Member

commented Mar 28, 2019

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.

👍

@elextr

This comment has been minimized.

Copy link
Member

commented Mar 29, 2019

Oh my, all those people who have two Geanys using the same config are gonna have many more races 😁

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.

@codebrainz

This comment has been minimized.

Copy link
Member

commented Mar 29, 2019

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 places which saves the configuration is hit.

@elextr

This comment has been minimized.

Copy link
Member

commented Mar 29, 2019

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.

@elextr

This comment has been minimized.

Copy link
Member

commented Mar 29, 2019

@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.

@eht16

This comment has been minimized.

Copy link
Member Author

commented Mar 29, 2019

@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 :).

@elextr

This comment has been minimized.

Copy link
Member

commented Mar 29, 2019

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 😁

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 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. 😀

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

This comment has been minimized.

Copy link
Member Author

commented Mar 29, 2019

Ok, b07c090 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 :).

doc/geany.txt Outdated Show resolved Hide resolved
@elextr

This comment has been minimized.

Copy link
Member

commented Mar 30, 2019

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 eht16 force-pushed the eht16:save_config_on_doclist_change branch from 5931769 to 96bbc83 Apr 14, 2019

@eht16 eht16 removed the work in progress label Apr 14, 2019

@eht16

This comment has been minimized.

Copy link
Member Author

commented Apr 14, 2019

Seems to work fine, used it in the last weeks and noticed no problems or side effects so far.

@elextr

This comment has been minimized.

Copy link
Member

commented Apr 14, 2019

Hmm, travis seems to have a problem with that unreliable download.geany.org :)

@eht16

This comment has been minimized.

Copy link
Member Author

commented Apr 14, 2019

Fixed by restarting the build and now it's green again :)

@b4n
Copy link
Member

left a comment

Looks mostly good, see comments.

src/plugindata.h Outdated Show resolved Hide resolved

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);

This comment has been minimized.

Copy link
@b4n

b4n Apr 15, 2019

Member

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:

+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:

+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);
/* … */

This comment has been minimized.

Copy link
@elextr

elextr Apr 15, 2019

Member

Well, the session includes cursor position so its probably worth saving.

This comment has been minimized.

Copy link
@ntrel

ntrel Jun 5, 2019

Member

Why is document-new connected to? If there's no file yet it won't affect the session.

This comment has been minimized.

Copy link
@eht16

eht16 Jun 30, 2019

Author Member

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 d6ebff6. Probably not necessary, I just added it for completeness.

@b4n b4n added this to the 1.36 milestone Apr 15, 2019

@@ -1313,11 +1316,47 @@ void configuration_apply_settings(void)
}


static gboolean save_configuration_cb(gpointer data)
{
configuration_save();

This comment has been minimized.

Copy link
@elextr

elextr Apr 15, 2019

Member

Just continuing @b4n's theme, do we need to save both config and project? The active session is in the project, not the config.

This comment has been minimized.

Copy link
@elextr

elextr Apr 15, 2019

Member

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.

This comment has been minimized.

Copy link
@eht16

eht16 Jun 30, 2019

Author Member

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"?

This comment has been minimized.

Copy link
@elextr

elextr Jun 30, 2019

Member

"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.

@kugel-

This comment has been minimized.

Copy link
Member

commented May 16, 2019

What's missing on this? Want this desperately.

@eht16

This comment has been minimized.

Copy link
Member Author

commented May 16, 2019

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

This comment has been minimized.

Copy link
Member

commented Jun 5, 2019

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.

@elextr

This comment has been minimized.

Copy link
Member

commented Jun 5, 2019

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

This comment has been minimized.

Copy link
Member Author

commented Jun 30, 2019

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.

@codebrainz

This comment has been minimized.

Copy link
Member

commented Jun 30, 2019

As said above, if you or anyone else want, let's improve the feature afterwards...

👍, I might even work on this (again), either like #1257 or something a little less invasive.

@kugel-

This comment has been minimized.

Copy link
Member

commented Jul 19, 2019

@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-
kugel- approved these changes Jul 19, 2019
@elextr

This comment has been minimized.

Copy link
Member

commented Jul 19, 2019

@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.

@kugel-

This comment has been minimized.

Copy link
Member

commented Jul 19, 2019

What do you refer to with <ctrl>s ?

@elextr

This comment has been minimized.

Copy link
Member

commented Jul 19, 2019

@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.

@kugel-

This comment has been minimized.

Copy link
Member

commented Jul 19, 2019

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.

@elextr

This comment has been minimized.

Copy link
Member

commented Jul 19, 2019

Oh, ok, I read "I'm too tired of losing my open files" as you hadn't saved them.

eht16 added 2 commits Mar 28, 2019
Save main and project configuration whenever documents are opened/closed
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.

@eht16 eht16 force-pushed the eht16:save_config_on_doclist_change branch from d6ebff6 to 0000f7d Jul 21, 2019

@eht16

This comment has been minimized.

Copy link
Member Author

commented Jul 21, 2019

@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.

@elextr

This comment has been minimized.

Copy link
Member

commented Aug 3, 2019

@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.

@eht16

This comment has been minimized.

Copy link
Member Author

commented Aug 4, 2019

@elextr what do you mean exactly? Should we add a listener on project-save or should we extend the documentation of project-save?

@elextr

This comment has been minimized.

Copy link
Member

commented Aug 4, 2019

Documentation I meant. The description of plugin_save on this page that lists when it happens.

@eht16

This comment has been minimized.

Copy link
Member Author

commented Aug 5, 2019

Ahhh, good catch! Thanks, see 41227a4.

@eht16

This comment has been minimized.

Copy link
Member Author

commented Aug 5, 2019

@b4n any other feedback? Would you be fine with merging this PR and let @codebrainz or whoever do the rest later?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.