Change role of automatically save database on non-data changes

[xboxones1]

Overview

When opening/closing a group, it requires you to save the database.

Steps to Reproduce

Expected Behavior

This was not observed in older versions, you do not need to save the database when opening/closing a group

KeePassXC - Version 2.7.6 Revision: dd21def

Qt 5.15.10 Debugging mode is disabled.

Operating system: Windows 11 Version 2009 CPU architecture: x86_64 Kernel: winnt 10.0.22621

[phoerious]

You do not need to save, but now you can.

[xboxones1]

But when keepassxc is closed, a save request is issued, this behavior adds unnecessary actions for nothing.

[phoerious]

A save prompt on exit is indeed annoying and superfluous @droidmonkey.

[droidmonkey]

You disabled automatically save non-data changes on exit. Go to application settings and re-enable.

This was BROKEN prior to 2.7.6 which caused loss of state.

[phoerious]

IMHO you shouldn't be prompted to save on exit if only non-data changes occurred. Those are safe to dismiss, whereas triggering saves unnecessarily is definitely annoying and potentially dangerous.

[droidmonkey]

You won't be prompted if you enable the setting

[xboxones1]

I turned on autosave, it is not offered to be saved, it saves itself, it also requires you to touch the yubikey every time you exit. This is not an adequate action and is completely broken.

[phoerious]

You won't be prompted if you enable the setting

No, but you will trigger automatic saves when they are not needed, which take time and can be quite annoying with YubiKeys. Turning the setting off should not result in a save prompt.

[phoerious]

I would vote for the following behaviour if only non-data changes were made:

[x] auto save non-data changes -> save quietly, discard if error occurs due to missing YK [ ] auto save non-data changes -> discard changes

[xboxones1]

I want to touch the yubikey when opening the base but not when closing it, it shouldn't be like that.

[droidmonkey]

Your choice to use yuibkey touch is exactly that. The only other option here is to IGNORE non data changes (essentially the broken behavior). That includes group collapse state, entry order, and something else I can't remember right now.

[xboxones1]

This is now broken, before version 2.7.6 it worked as it should

[phoerious]

TBH, it's not just the touch (I have it disabled). It's having to have it plugged in still or remembering to reinsert it before you leave your desk. I don't think that's practical at all, especially if you keep it on your keychain.

[xboxones1]

These changes need to be rolled back, it's just not practical or convenient. Save each time the base when moving through it, it should not be so.

[phoerious]

The change per se is legit. Just the unwanted side effects need to be fixed.

[r3times]

The only other option here is to IGNORE non data changes (essentially the broken behavior). That includes group collapse state, entry order, and something else I can't remember right now.

May I ask why ignoring these unsaved non-data changes is considered to be broken behaviour? Would someone wanting to make these changes persistent not understand that they need to save, as they will have done for previous versions?

[droidmonkey]

The broken behavior did not mark the database as modified so you COULD NOT choose to save. The fix was to mark the database as modified to allow you the chance to save. It appears, based on this thread, that it disrupts the apple cart of yubikey users. That is entirely unexpected and unintentional.

[r3times]

Thanks and understood. Not something I'd had cause to notice was missing previously.

Not a Yubikey user, but being able to maintain the 'modified time' of my database after data changes, while also not having to recall if I'd made any data changes in the hours of having it open, was very useful when deciding whether/when to synchronise two copies.

[droidmonkey]

When in doubt just merge them, it won't do anything if there are no changes.

[xboxones1]

Broke what worked. I do not suffer from loss of memory and I do not need to remember whether I made changes or not. Here the problem is not only in Yubikey, such behavior is simply not acceptable when the base is saved simply after navigation on the panel. I want to decide for myself when to keep changes, and when not. This should be a disabling option, not be permanent.

[droidmonkey]

@phoerious I propose we change this setting to "Mark database modified on non-data changes". If it is disabled, then we will do the behavior prior to #9634 (ie, do not mark the database as modified). If it is enabled then we will do the current behavior.

[phoerious]

I would rather use the new databaseNonDataChanged() signal or add a parameter to the databaseChanged() signal indicating whether the change was a data change or a non-data change. A non-data change would enable the save icon, but would not trigger any of the other effects, i.e., we simply keep track of whether databaseChanged(true) has fired or only databaseChanged(false). We already keep track, so replacing the existing boolean state with a std::optional would suffice.