Page MenuHome GnuPG
Create Task
Maniphest T6667

Kleopatra: kleopatrarc is synced only on clean quit
Open, NormalPublic
Actions

Description

This is actually a bad bug which I had not realized at all until someone mentioned it to me anecdotally recently.

Some of our users have Kleopatra in autostart, and usually keep it running in the systray while their windows session is up.

At one point they had selected "only revoked certificates" as the keyfilter. And then without realizing it actually quit Kleopatra through File -> Quit.

But usually Kleopatra keeps running through the session, and when the user loggs out at the end of the day it seems to be killed in a way which does not trigger the dtor of KleopatraApplication which is the only place where KSharedConfig::openConfig()->sync() is called.

I believe that this is quite an important issue and that we should fix that in the Gpg4win/23.7 branch, too. Because a lot of our "usability" depends on things like reusing the last selected state for certifications or the last used signing key or last used encryption key etc. and I believe that users are just used to Kleopatra not remembering these things that this issue is not reported far more often.

First I will check if on Windows QCoreApplication::aboutToQuit() is signalled when the user logs out. Then this would obviously be a better place. I am sure that Windows Applications receive some kind of notification when the session is ended and Qt will likely pass that.

But we should sync more explicitly at more places. I somehow naively assumed that if you modify a KSharedConfig::openConfig object it would be synced automatically when it goes out of context.

Revisions and Commits

rKLEOPATRA Kleopatra
rKLEOPATRA0c94ed319aa6 Use aboutToQuit signal to sync KSharedConfig
rKLEOPATRAb785819ea301 Use aboutToQuit signal to sync KSharedConfig

Event Timeline

aheinecke triaged this task as High priority.Aug 18 2023, 2:06 PM
aheinecke created this task.
aheinecke claimed this task.Aug 18 2023, 6:31 PM
aheinecke lowered the priority of this task from High to Normal.

I think that fixes the biggest issue here as long as kleo is not just killed it should save the current configuration state. Maybe we should add it in some more places explicitly, too where many things are stored in the config, like with the certifydialog?

aheinecke added a commit: rKLEOPATRAb785819ea301: Use aboutToQuit signal to sync KSharedConfig.Aug 18 2023, 6:31 PM
aheinecke added a commit: rKLEOPATRA0c94ed319aa6: Use aboutToQuit signal to sync KSharedConfig.Aug 18 2023, 6:33 PM
aheinecke added a comment.Aug 21 2023, 9:13 AM

For the record I tested it on Windows that this now saves the config when logging out.

CarlSchwan added a subscriber: CarlSchwan.Aug 21 2023, 9:49 AM

Yeah this is a know issue with kconfig, I ended up in many places in NeoChat and other apps to just call sync directly after every set because I'm often quitting apps with Ctrl+C and the settings were often not saved for me.

aheinecke added a comment.Aug 21 2023, 10:48 PM

Yes, since we also don't have a ton of "temporary" changes (except for window geometries) such a behavior would make the most sense.
Does it even make sense for us in these places to use KSharedConfig?

Or maybe we should propose an API extension for KConfig for it to change this behavior on an Application Level if others have this problem, too?

Instead of fixing the places where it would need to be fixed in Kleoptra
from the top of my head this would be:

  • Filter / Keylistview changes
  • Certifywidget
  • Sign / Encrypt widget
  • Configurationdialog (but that probably does some sync)
ikloecker added a subscriber: ikloecker.Aug 22 2023, 10:03 AM

I'm not sure, but you may also want to sync the state config (which stores mostly window geometries). In fact, we might want to use the state config for more settings that change often, e.g. the different last used directories.

Having to call sync explicitly is need for the original use case of KConfig, i.e. config dialogs, where you write many settings on Apply and don't want to have an automatic sync after each and every set. Possible solutions include an automatic deferred sync and a RAII-class which syncs on destruction. I think I'd prefer the latter because it's more explicit and doesn't break existing use cases. I think it's a common approach to set the config values on every change in the config dialog, but to only commit the changes when the user clicks Apply or OK.

aheinecke added a comment.Aug 22 2023, 10:21 AM

My question would be, should we try to improve KConfig in some way which makes it easy for us to do this? I think we should, if this is a common problem for many applications. Maybe a task for sune?

Maybe a global KConfig setting to change the default behavior that when a mutable KConfigGroup is destructed that it syncs? And then explicitly require an additional paramater to retrieve a non autosyncing mutable KConfigGroup?