Database merge confirmation dialog

[taminob]

This PR adds a dialog allowing a user to review the changes of a merge operation.
This dialog displays the changes and allows the user to abort the merge without modifying the database.

Fixes #1152

Screenshots

Testing strategy

Manual testing and the execution of the already existing test suite testmerge.

I also added a some basic tests verifying that a database will remain unmodified when only calculating the changelist and if for a merge without re-calculation only that changelist will be applied to the database.

Type of change

• ✅ New feature
• ✅ Refactor (small refactoring)

[taminob]

This PR doesn't yet follow the contribution guidelines and contains temporary code, so don't take it too seriously.

I did a first draft to simply be able to call with a dryRun parameter which would disable all modifying operations.
However, I feel like this is kind of dirty and doesn't really help with the code complexity.
Thus, I tried to split off the modifying operations and be able to execute them afterwards which would also already enable a future feature like only partially merging a database (although this will require some additional logic).

I was struggeling a bit with the lifetime of the objects and if there are any guarantees defined somewhere and if e.g. the entry pointers are actually guaranteed to be not null and will remain valid.

The last major open task is to fix the merge of the deletions since the current implementation relies on the other entries being merged first - however, this change requires to not modify anything before confirmed by the user.
Let me know if you'd rather keep these changes minimal and I'll consider less invasive alternatives like merging into a temporary database.

[droidmonkey]

Awesome MVP!!

[taminob]

Sorry that the PR became kind of huge - I encountered some unexpected issues with the merge logic for deletions since it relied on the groups/entries already being merged before.
However, I need those merged deletions before the actual merge to display the change list.
Please read the changes carefully to avoid any data loss in the merges since I could easily have missed some edge case.

Thanks a lot to whoever wrote the extensive test suite for the merge logic, that helped a lot to debug issues and also understand the reasoning behind the merge behavior.

If you want to, I can also split this into two PRs, I already kept it apart in two commits (one for the merge logic refactoring and one for the UI).

I also left a couple of TODO markers in there with a few questions regarding the existing code and if that's still up-to-date.

[mmahmoudian]

@taminob Thanks for investing time into implementing this. I have suggested some related CLI features and I've been told to suggest them here as this PR is the perfect place for this.

It would be amazing if we can have the table you implemented in GUI into CLI since the info is there and it would be just a matter for formatting (for instance in markdown table format):

❯  keepassxc-cli merge --dry-run --same-credentials db-local.kdbx db-remote.kdbx
Enter password to unlock db-local.kdbx

| Action                               | what      | UUID               |
|--------------------------------------|-----------|--------------------|
| Overwriting                          | blah blah | [814h814h814h814h] |
| '_ Synchronizing from newer source   | blah blah | [814h814h814h814h] |
|   '_ Creating missing                | blah blah | [814h814h814h814h] |
|   '_ Creating missing                | blah blah | [814h814h814h814h] |

Database was not modified by merge operation.

Additionally, It would be extra useful to specify which file is the "newer source" (newer can be based on file modification time or internal variable in databases). In the screenshot you posted in GUI, you also have "newer source" which is imho ambiguous.

Also (slightly off-topic), I wonder if it is feasible to use some sort of color coding. Then --color=[auto,always,never] can control that like other commonly use CLI tools.

As a final remark and comment/feedback on your amazing work, I would like to suggest to move the UUID to the last column of the table as to the user UUID is not so useful compared to other columns.

Thanks again for your efforts. I've been waiting for this feature for quite some time :)

[taminob]

@mmahmoudian thanks for suggesting, I think that would be an improvement, too. I also like your mockup, although I'm not quite sure what's the meaning of the tree-like structure in the Action column?

However, I think that it might be better to keep further user experience improvements to a separate follow-up PR since this PR is already quite complex. I even thought about moving the entire GUI stuff to a separate PR.
Although I noticed that some of the complexity might be not even necessarily required when looking at the --dry-run implementation in the CLI tool (unfortunately didn't notice that before - could've saved me a lot of trouble implementing the refactoring :) ) since modifications to the Database object seem to be safe as long as it's not saved (and the dialog is blocking all other input anyway, so user shouldn't be able to save manually).

Do you have any opinion @droidmonkey, do you think the current refactoring is worth the risk? I'd also be willing to look into the CLI improvements, generating such a table shouldn't be too hard.

Otherwise, this PR would also be ready if you have time to review it. The failing CI is unfortunately caused by out-dated translations on develop.

[mmahmoudian]

@taminob

what's the meaning of the tree-like structure in the Action column?

Basically this hierarchical structure is better render of what the CLI outputs at the moment. To illustrate it further, compare the screenshot and the table in this post:

#6264 (comment)

However, I think that it might be better to keep further user experience improvements to a separate follow-up PR since this PR is already quite complex.

Sure. Definitely you know better :)

[droidmonkey]

Generally speaking, I try to avoid refactor unless we are solving a code pain point. Looks like you'd be good to not make all this additions to Merger.h/cpp

[taminob]

Generally speaking, I try to avoid refactor unless we are solving a code pain point. Looks like you'd be good to not make all this additions to Merger.h/cpp

I completely understand that and agree that unnecessary refactorings can cause a lot of pain.

I looked again at the alternative which is modifying the database (or a temporary copy of it) and restore the original database if the merge was aborted.
However, I noticed that the database might get automatically reloaded when changed on disk and I'm also not sure if the database might get saved automatically in the background without the merge being confirmed.
For the first case, I think it would be possible to avoid reloading via an additional isMergeInProgress check in

keepassxc/src/gui/DatabaseWidget.cpp

Line 1838 in a472ef8

if (!m_db || isLocked() || isEntryEditActive() || isGroupEditActive() || isSaving()) {

, but I'm not sure if the second requirement can be guaranteed somehow. If the merge would modify the database anyway (or the actually merged changes do not match the displayed ones), that would, in my opinion, be way worse than simply not having the confirmation dialog.

[taminob]

@droidmonkey while this kind works, is there a better way to clone a database? Wasn't able to find one in the current code base.
Also, this does not include the Database::m_metadata which doesn't leads to missing changes regarding Recycling Bin and Custom Icons - is there a reason why it doesn't implement a clone() function or should I implement it to do a full deep copy of a database?

[droidmonkey]

I'd have to check but I don't think so since cloning a database is not really a thing in the program. Not saying it wouldn't be useful, but the current method you are using here is not the correct way. That would be more like transferring root group and children.

[taminob]

But doesn't Group::clone() create an independent copy of the root group (the ownership of which I then transfer to the temporary database)?

The alternative to cloning the database would be to implement something like discardModifications() for a database - as far as I can see, that doesn't exist yet. But then we have to be absolutely sure that no one will save the database in the meantime before the merge is confirmed.

Which of these two approaches do you think fit the current design better? Or do you have another idea on how to achieve such a "temporary merge" without major changes to the merge logic?

[Qwerty-Space]

Another (hopefully small) addition you could make to this merge window is the option to exclude entries from the merge.

Let's say we have DB foo and DB bar both with some new and outdated entries. You might want to exclude the outdated entries when merging to avoid having to roll them back later.

[taminob]

Another (hopefully small) addition you could make to this merge window is the option to exclude entries from the merge.

Let's say we have DB foo and DB bar both with some new and outdated entries. You might want to exclude the outdated entries when merging to avoid having to roll them back later.

When implementing my first draft, I had this feature already in mind (although for a separate PR). However, this could require a complete rewrite of the merge logic which was rejected before (see #10173 (comment)) for valid reasons - refactoring such a complex part of the code is not without risks.
I can have a look if we could implement this without the refactoring once we have database cloning, but no promises that that will work. But it is a more complex change than it might seem because if you e.g. exclude the deletion of a Group, you need to add all the differences of the entries in the group to the merge again (which were previously removed because of the deletion).

Otherwise, I'm currently waiting on feedback on #10173 (comment) @droidmonkey - some hints on how you think the feature would fit the current architecture best would be appreciated.