Add auto-save delay per database #9100

jNullj · 2023-02-10T21:42:18Z

Fixes #8553 - Configure autosave delay per database
Users with big database report bad ux due to long save time of the database when the automatic save on modification option is enabled.
To keep automatic saves yet reduce lag from user this merge presents an option to delay the automatic save since the last edit. That way the changes are saved automatically only a short period of inactivity, making the application more responsive when in use and wait for a save once user makes less engagement.
By default this option is turned off, users with big database should enable it in the database settings.

I am here to learn, let me know if anything missing or something need improvement.

Screenshots

Testing strategy

Tested manually all functionality added and tested using automated tests with 100% pass.
Tested manually on both pre-change db and post-change db.
Also added tests that cover all changes made (checked using coverage-html output)

Type of change

❎ Bug fix (non-breaking change that fixes an issue)
✅ New feature (change that adds functionality)
❎ Breaking change (causes existing functionality to change)
❎ Refactor (significant modification to existing code)
❎ Documentation (non-code change)

After looking at CI tests in pull request keepassxreboot#9100 the margin for the file save delay was too short. This commit extends the margin to 10 seconds and simplifies the test.

src/core/Metadata.h

src/core/Metadata.cpp

src/gui/dbsettings/DatabaseSettingsWidgetGeneral.cpp

src/gui/dbsettings/DatabaseSettingsWidgetGeneral.ui

src/gui/DatabaseWidget.cpp

tests/gui/TestGui.cpp

share/translations/keepassxc_en.ts

src/gui/dbsettings/DatabaseSettingsWidgetGeneral.ui

src/gui/DatabaseWidget.cpp

src/gui/DatabaseWidget.h

src/gui/dbsettings/DatabaseSettingsWidgetGeneral.cpp

share/translations/keepassxc_en.ts

src/gui/DatabaseWidget.cpp

dmaslenko · 2023-02-14T16:49:08Z

I checked out the PR to play locally.
What I noticed and it should be addressed:

Selected checkbox and a delay value are not saved properly.
- Open a DB -> select it and define 1 min -> close the DB -> open DB -> nothing saved.
- Open a DB -> select it and define 1 min -> lock the DB -> unlock DB -> saved only delay value but not check box.
Changing of the delay on unsaved DB
- Have configured autosave with delay, 5 min -> change something -> change the delay to 10 min -> app saves the DB still after 5 min.
- This is questionable use case, should we restart new timer or prompt the user about it, let's discuss. For example, the user started DB changes, then realized the delay is too short, the attempt to increase it on the fly will not apply that will confuse the user.

To be continued, sorry have very limited free time, could not perform whole testing or code review.

jNullj · 2023-02-14T23:03:37Z

I checked out the PR to play locally. What I noticed and it should be addressed:

Selected checkbox and a delay value are not saved properly.

Open a DB -> select it and define 1 min -> close the DB -> open DB -> nothing saved.

Open a DB -> select it and define 1 min -> lock the DB -> unlock DB -> saved only delay value but not check box.

Changing of the delay on unsaved DB

Have configured autosave with delay, 5 min -> change something -> change the delay to 10 min -> app saves the DB still after 5 min.

This is questionable use case, should we restart new timer or prompt the user about it, let's discuss. For example, the user started DB changes, then realized the delay is too short, the attempt to increase it on the fly will not apply that will confuse the user.

To be continued, sorry have very limited free time, could not perform whole testing or code review.

Selected checkbox and a delay value are not saved properly.
- Can reproduce.
- I feel really stupid, i really missed big time there, i should edit KdbxXmlReader/Writer.
Changing of the delay on unsaved DB
- I think its best to reset the timer as there was a change, i think users would expect that, prompting the user for such a small thing seems too much and might be more confusing then useful. As user wanted delay from last action resetting still follows this course of action.
- i will make those changes as well
MockTimer
- The only thing i struggle with right now, i will take some more time into this after i finish everything else.

I really hope i did not cause too much trouble with this pull request, I will try being more careful before making commits as i clearly missed lots of stuff when i did the request.
If those messages make too much noise let me know.
Also should i rebase the branch for a cleaner history? is this a common thing for this project or something forbiden as this rewrites the branch history?

droidmonkey · 2023-02-15T01:42:02Z

I feel really stupid, i really missed big time there, i should edit KdbxXmlReader/Writer.

No, use the database custom data

dmaslenko · 2023-02-15T04:22:53Z

Also should i rebase the branch for a cleaner history?

Let's ask @droidmonkey, I prefer keep it as is, at least someone made changes in develop branch needed for you. On merging step all your commits can be squashed into single one.

If those messages make too much noise let me know.

As for me it is fine, better to have active discussion with many fixes/corrections and finally release bug-free solution.

droidmonkey · 2023-02-15T06:00:01Z

Yes please rebase, we prefer rebase to merge commits to keep a linear history

jNullj · 2023-02-18T13:37:03Z

I feel really stupid, i really missed big time there, i should edit KdbxXmlReader/Writer.

No, use the database custom data

While doing this i also found out other value in custom data are not checked when reading
for example in Metadata::savedSearches() it appears auto searches = m_customData->value("KPXC_SavedSearch"); is assumed to always exist. Is that normal?
In my code for the autodelaysave i take into account the key might not exist and check if the QString returned is null. as this is what i expect m_customData->value to return if key is not found by reading the code for that function.

droidmonkey · 2023-02-18T13:48:31Z

You don't need to check if it exists, if it doesn't then it will return an empty string

jNullj · 2023-02-18T13:52:09Z

You don't need to check if it exists, if it doesn't then it will return an empty string

Well in my last commit ac8f6fa It seems that i should check for QString.isNull()
When opening an old db without the settings i do get into this breakpoint

This mean it does not return an empty string

jNullj · 2023-02-18T13:56:16Z

I checked out the PR to play locally. What I noticed and it should be addressed:

Selected checkbox and a delay value are not saved properly.

Open a DB -> select it and define 1 min -> close the DB -> open DB -> nothing saved.

Open a DB -> select it and define 1 min -> lock the DB -> unlock DB -> saved only delay value but not check box.

Changing of the delay on unsaved DB

Have configured autosave with delay, 5 min -> change something -> change the delay to 10 min -> app saves the DB still after 5 min.

This is questionable use case, should we restart new timer or prompt the user about it, let's discuss. For example, the user started DB changes, then realized the delay is too short, the attempt to increase it on the fly will not apply that will confuse the user.

To be continued, sorry have very limited free time, could not perform whole testing or code review.

Selected checkbox and a delay value are not saved properly.

Can reproduce.

I feel really stupid, i really missed big time there, i should edit KdbxXmlReader/Writer.

Changing of the delay on unsaved DB

I think its best to reset the timer as there was a change, i think users would expect that, prompting the user for such a small thing seems too much and might be more confusing then useful. As user wanted delay from last action resetting still follows this course of action.

i will make those changes as well

MockTimer

The only thing i struggle with right now, i will take some more time into this after i finish everything else.

I really hope i did not cause too much trouble with this pull request, I will try being more careful before making commits as i clearly missed lots of stuff when i did the request. If those messages make too much noise let me know. Also should i rebase the branch for a cleaner history? is this a common thing for this project or something forbiden as this rewrites the branch history?

Fixed the save issue with ac8f6fa
I could not replicate with the latest commit the "Changing of the delay on unsaved DB" issue, it seems to work as intended.

jNullj · 2023-02-18T19:34:05Z

I fixed everything and did a rebase to start from latest develop commit and make a tidy history without ridicules fixes for things like format that should have been right from the first commit. I think this is ready for a review.
Regarding testing timing i changed everything to run without those large delays i had, i only use now 150ms delays after every modification like done in other areas, otherwise i have issues with the modification timer.

dmaslenko

I checked the run of TestGUI, now it is much faster, no long waiting.
I will review the test changes later. The implementation in general looks good. See my minor remarks.

src/core/Metadata.cpp

src/gui/DatabaseWidget.cpp

tests/gui/TestGui.cpp

src/gui/DatabaseWidget.cpp

jNullj · 2023-06-24T20:50:10Z

@droidmonkey I see we used "Efficient-Compression-Tool" for all images in the docs.
My changes added new images to the docs due to UI additions.
Could you link to the tool you used so i can update my images to save up space as well?
I think it's best to add this to the wiki or as part of a script we expect to run before commit/release so future updates keep using the tool to lower images size.

Did you use this tool?

droidmonkey · 2023-06-24T21:16:56Z

Yes going to add it to the build scripts. Here is the command run: #9546 (comment)

jNullj · 2023-07-01T13:40:44Z

Yes going to add it to the build scripts. Here is the command run: #9546 (comment)

Processed 1 file
Saved 13.42KB out of 73.32KB (18.2960%)

It took my tiny laptop a long time but the saving is huge. I updated the image in my docs as optimized by latest changes.
Did a rebase for the commits so history looks linear.

droidmonkey

This works extremely well. The only thing I can think of is that if you were to disable auto-save AFTER you kicked off a delayed auto-save... then it will still save your database automatically. I don't think this is a big deal, what are your thoughts?

jNullj · 2023-07-04T11:44:53Z

This works extremely well. The only thing I can think of is that if you were to disable auto-save AFTER you kicked off a delayed auto-save... then it will still save your database automatically. I don't think this is a big deal, what are your thoughts?

It might be better to avoid this (unexpected save) so there is no unpredicted issue in the future.
It could be done by adding a check at timer timeout , this is the simple solution. I could also look for the timer and stop it when changing the settings but this is less straightforward.
I should also add this edge case to the tests.

I could add this in a few days. I will ping the review once I am done.
Please provide feedback regarding the implementation, do you think I should take the longer parh and stop the timer. Or a 1 liner shouldn't be an issue for testing once the timer stopped.

droidmonkey · 2023-07-04T11:54:45Z

I like the check on timeout solution.

jNullj · 2023-07-04T19:53:37Z

After looking into this i also noticed that the user might disable autosave while the timer is still running, i will also add this condition.

Add a new propery autosaveDelay in Metadata of the db. The property is saved in customData to do not affect database structure as this setting is unique to keepasxc. The propery sets delay to wait since last modification before saving. Part of issue keepassxreboot#8553

Add metadata propery autosaveDelay to the gui in the DatabaseSettings widget under General. User can change the delay in minutes. Add new translation strings as well. Part of issue keepassxreboot#8553

Implement a autosave delay functionality to the database widget. Added timer to track the delay and trigger a save on timeout using the new onAutosaveDelayTimeout function. Changed the onDatabaseModified to test for the new autosaveDelay property and delay the save if needed or extend the delay on new modifications. After menual save stop the timer to avoid saving when not needed edited in DatabaseWidget::save(). Part of issue keepassxreboot#8553

Try to toggle in gui, set a value, save it. Chcek value saved is currect in metadata. Check that on loading saved value presented in gui and toggeled. Part of issue keepassxreboot#8553

Test autosaveDelay makes delays as expected. Test delay timer reset after new modification before timeout. Part of issue keepassxreboot#8553

Minimize the test time by spying for signals and forwarding time rather then waiting the delay. Only wait where needed to finish modified signal timer

Imrpove code readbility and reduce bug potential byt using static const Qstring for all customData keys in Metadata.

User might trigger the autosave delay timer and disable autosave delay or autosave. This commmit fixes potential issues from the timer triggering after the feature is disabled. The timer now checks if the feature is still enabled when triggered. Adds tests for those edge cases as well

jNullj · 2023-07-06T20:20:34Z

Fixed the issue and did a rebase to latest commit at develop
@droidmonkey I hope this will do for the pr. let me know if you find anything else missing/I could improve.

Add a new propery autosaveDelay in Metadata of the db. The property is saved in customData to not affect database structure as this setting is unique to keepasxc. The propery sets delay to wait since last modification before saving. Co-authored-by: jNullj <jNullj@users.noreply.github.com>

Release 2.7.8 Changes - Add hotkey for showing search help [keepassxreboot#10591] - Add hotkey for group switching (Ctrl+Shift+PgUp/PgDown) [keepassxreboot#10625] - Add per-database auto-save delay setting [keepassxreboot#9100] - Add setting to hide menubar [keepassxreboot#10341] - Improve Bitwarden 1PUX import and support organization collections [keepassxreboot#10499] - Show advanced settings checkbox only for settings that have them [keepassxreboot#6513] - Remove obsolete setting for requiring repeated password entry [keepassxreboot#9722] - Passkeys: Allow registering Passkeys to existing entries [keepassxreboot#10408] - Passkeys: Show warning about data being unencrypted before Passkey export [keepassxreboot#10411] - Passkeys: Support NFC and USB transports [keepassxreboot#10402] - Passkeys: Pass extension JSON data to browser [keepassxreboot#10615] - SSH Agent: Do not use entries from recycle bin [keepassxreboot#10518] - Linux: Change hotkey sequence used for {CLEARFIELD} Auto-Type [keepassxreboot#10008] - Windows: Improve DACL memory access protection [keepassxreboot#10618] Fixes - Fix crash when deleting history items [keepassxreboot#10451] - Fix crash on screen lock or computer sleep [keepassxreboot#10458] - Fix search field not being focused after unlock [keepassxreboot#10459] - Fix loss of window focus when Auto-Type needs to unlock a database [keepassxreboot#10555] - Fix inconsistent TOTP visibility on unlock [keepassxreboot#10009] - Fix CSV import skipping over single-name groups [keepassxreboot#10575] - Fix key file folder being remembered even if disabled in settings [keepassxreboot#10636] - Fix issues with entry editing and database locking [keepassxreboot#10667] - Fix key file text when provided on command line [keepassxreboot#10642] - Fix issues with hardware key auto detection [keepassxreboot#10663] - Do not override monospace font size [keepassxreboot#10282] - Perform group sort only when group view is in focus [keepassxreboot#10202] - Do not show decimals for attachment sizes in Bytes [keepassxreboot#10595] - Prevent merging of global custom data when merging databases [keepassxreboot#10452] - Fix minor translation issues [keepassxreboot#10635] - Passkeys: Fix StrongBox incompatibility [keepassxreboot#10420] - Passkeys: Set RP ID to effective domain if unset instead of returning an error [keepassxreboot#10384] - Passkeys: Various UI fixes and improvements [keepassxreboot#10427, keepassxreboot#10608, keepassxreboot#10609] - AppImage: Fix URL opening [keepassxreboot#10624] - Flatpak: Fix application autostart [keepassxreboot#10563] - Linux/macOS: Fix button sizes on modal alert popups [keepassxreboot#10500] - Linux: Fix clipboard clear on Wayland [keepassxreboot#10500] - Windows: Preserve file-hidden attribute [keepassxreboot#10343] # -----BEGIN PGP SIGNATURE----- # # iQIzBAABCAAdFiEE6A9nU7OzJX8m9B8ILPQdKqhDj5kFAmY4A30ACgkQLPQdKqhD # j5npgBAApBCGfhdugBE3X9iCkGQ69LKKWizgp44AzmezxU2ee7KEoZgSmZpOCPyO # bg9EIgwac+3yCh4i4hJrTvnwIemrUKNsNLE18Kn/Uw3HJBCtsb40CeIFcZktOegu # RQ5G7jhBtnAopnTKQhdwcwJ0Yq6ZSTSiSuo+miDAN22DjnWVd7BLMOioSBPgxFUT # td+2MAPeydLoMdFRmkuBaDSStLWThdCz6DrWcBYQSK2b6Mu+3mzmtE24zNM1jCKu # Tl0t6fRkOhqWSRyWBSMzIH3uMuV95yQNudjDMnuOVWVE9Ai+A1RPFHtf8Zj1ydh9 # n9JGPDyloWRcYQdDBgbn6lFHWnwSaYVCRpRPPmjpmXVwt5/AdtB8wN+6uGbcYTzw # u9l0YYWx84W0kNPkJ0ZejF33qioQ7FaZruJv2ej++NtO0FJP48UVyrQ4EMG6V+17 # AcQ0aoSWWTb5AYhJXLjImDG7DNY1mbgW6deJLKVS7pkoRke1uSLGqYTUAJCFaXnq # d9uZt4HRUUMeq6x8dvFNvIcZhsfRUaO/iXjp81nl8hlWIeTYNTj22eww3yapFs+S # cdmdCmfGZAx5FWCXaszXwD3gLF8Bg6S63l9TvbjEHGR2riYKOO1IbFz8JXXjWpdN # l4SIcWJfdO2mNz0MWfzNtmMYNu9LBfU2Hod5JHJQYiQh3dh4EG4= # =MrBi # -----END PGP SIGNATURE----- # gpg: directory '/home/runner/.gnupg' created # gpg: keybox '/home/runner/.gnupg/pubring.kbx' created # gpg: Signature made Sun May 5 22:09:01 2024 UTC # gpg: using RSA key E80F6753B3B3257F26F41F082CF41D2AA8438F99 # gpg: Can't check signature: No public key

dmaslenko suggested changes Feb 12, 2023

View reviewed changes

src/core/Metadata.h Outdated Show resolved Hide resolved

src/core/Metadata.cpp Outdated Show resolved Hide resolved

src/core/Metadata.cpp Outdated Show resolved Hide resolved

dmaslenko suggested changes Feb 12, 2023

View reviewed changes