Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Move up/down not trigger modification state and cannot save #9501

Closed
qupig opened this issue May 31, 2023 · 5 comments · Fixed by #9634
Closed

Move up/down not trigger modification state and cannot save #9501

qupig opened this issue May 31, 2023 · 5 comments · Fixed by #9634
Assignees
Milestone

Comments

@qupig
Copy link

qupig commented May 31, 2023

Overview

After moving entries up and down, the database does not change to the modified state, the save database button and menu are grayed out and cannot be saved.

Expected Behavior

Moving entries up and down belongs to modifying database data, which should trigger the modification state and activate the operation that can save the database.

Actual Behavior

After moving an entry up or down, the database modification state is not triggered. Unable to perform save operation.

Context

KeePassXC - Version 2.7.5
Revision: 9d0537b

Operating System: macOS

@qupig qupig added the bug label May 31, 2023
@droidmonkey
Copy link
Member

The database will save on lock. This is by design to prevent excessive saving when moving entries around. This is not "database data", it is metadata.

@droidmonkey droidmonkey closed this as not planned Won't fix, can't repro, duplicate, stale May 31, 2023
@qupig
Copy link
Author

qupig commented Jun 1, 2023

@droidmonkey

The database will save on lock

In my local testing, no, neither locking nor closing the database will automatically save.
When I unlocked again, my move was canceled.
Yes, I turned off all autosave settings because I prefer to save manually.
Instead I think those autosaves are a kind of "excessive saving".
So, consider the use case of manually saved users, I edited the entry order, but I can't save the database.

This is not "database data", it is metadata.

If that's metadata, it should exist independently of the database file, i.e. not kept in the database file.
But in keepass, that is saved in the database xml structure, so I think that is part of the database data.

@droidmonkey
Copy link
Member

droidmonkey commented Jun 1, 2023

My apologies, this is a bone-fide bug. Thank you for reporting it!

The error occurs when this setting is disabled:

image

We do not "enable the save button" for non-data changes when this setting is disabled. This makes it impossible to save the non-data changes without changing data (ie, an entry).

@droidmonkey droidmonkey reopened this Jun 1, 2023
@droidmonkey droidmonkey self-assigned this Jun 1, 2023
@droidmonkey droidmonkey added this to the v2.7.6 milestone Jun 1, 2023
@qupig
Copy link
Author

qupig commented Jun 1, 2023

Automatically save non-data changes when locking database

I don't know why was this option originally designed, but I think its existence may be debatable.

I understand that the "non-data" here refers to the different from main valid data in the entry (such as username and password etc.), but I'm not sure if all other users understand this intuitively. This description does not seem so clear, and should we distinguish them in the editing and saving operations?

After some thinking, I think, yes, maybe we can treat differently in some designs, so that different data and fields have different priorities, but for the operation of locking the database, I don't think it should be treated differently.

In fact, it is more suitable for the first option (i.e. "every change"), which is an option that is prone to "excessive saving". So being able to enable this action only for key data/fields will have a more significant effect.

One of the reasons I prefer manual saving is that I want to keep a clean and organized backup of my database, rather than having messy and ultimately hard to use backup files due to automatic saving. "auto save after every change" apparently breaks this. Even temporary and hesitant changes will generate a new backup.

Sometimes, I accidentally mess up some data, or just make some experimental changes, the most efficient way to discard these changes is to close and chose don't save the database. Auto-lock related settings may cause unexpected saves due to other two auto-save options.

This is why I ended up choosing to turn off all auto-saves. Only manual saving can meet the above needs. However, I understand the significance of the existence of auto-saves, which will prevent people from losing new data to a certain extent. But that shouldn't be a factor that bothers usage and probably requires a more efficient mechanism design as well.

In any case, even a "non-data" changes is indeed a modification of the actual data in the database structure, and any action that actually modifies the database should trigger a modified state of the database (such as an asterisk in the title). Of course, the user should also be allowed to manually perform save or non-save operations.

Thanks to all the developers for contributing such a great open source project. I just hope it gets better.

@droidmonkey
Copy link
Member

Great points! We have a pending PR for "save after a delay" which is a good middle ground to "every change" and "manual save". I think in introducing that change I would remove the option discussed here.

droidmonkey added a commit that referenced this issue Aug 7, 2023
* Fix #9501 
* Also fix bug where context menu did not update when entry moved to very top or bottom of list
droidmonkey added a commit that referenced this issue Aug 14, 2023
* Fix #9501
* Also fix bug where context menu did not update when entry moved to very top or bottom of list
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants