Add check to see if merging is required#1882
Add check to see if merging is required#1882louib wants to merge 7 commits intokeepassxreboot:developfrom
Conversation
|
@TheZ3ro @droidmonkey can you guys take a look at this one? |
There was a problem hiding this comment.
The biggest problem with this implementation is it only compares the root group with the supplied group. It also does not compare recursively, just the top layer. This needs to be reworked before merge.
I prefer you add the recursiveness in the database merge function, leave the group "needsMerging" function to compare the top layers as presented to the function.
|
|
||
| bool Group::needsMerging(const Group* otherGroup) | ||
| { | ||
|
|
|
|
||
| Group* rootGroup = this; | ||
| while (rootGroup->parentGroup()) { | ||
| rootGroup = rootGroup->parentGroup(); |
There was a problem hiding this comment.
Forcing this up to the "true" root group is not appropriate. This function is meant to compare two groups as presented. By doing these actions here we cannot reuse this function to compare two arbitrary groups.
|
|
||
| // detect changes in entries | ||
| const QList<Entry*> dbEntries = otherGroup->entries(); | ||
| for (Entry* otherEntry : dbEntries) { |
There was a problem hiding this comment.
const Entry* otherEntry
Prevents any possibility of copying
| // detect changes in entries | ||
| const QList<Entry*> dbEntries = otherGroup->entries(); | ||
| for (Entry* otherEntry : dbEntries) { | ||
| Entry* existingEntry = rootGroup->findEntryByUuid(otherEntry->uuid()); |
|
|
||
| // detect changes in groups | ||
| const QList<Group*> dbChildren = otherGroup->children(); | ||
| for (Group* otherGroup : dbChildren) { |
| // detect changes in groups | ||
| const QList<Group*> dbChildren = otherGroup->children(); | ||
| for (Group* otherGroup : dbChildren) { | ||
| Group* existingGroup = rootGroup->findChildByUuid(otherGroup->uuid()); |
cddc6d9 to
d0d605c
Compare
|
Closing this one in favor of #2457 |
Add a function to detect when merging is needed.
@mbilloo This is what I had in mind when filing
#1010
I think @droidmonkey would like to have your deep comparison operator from #1836 merged nonetheless. Can you open a new PR
for that?
Motivation and context
Fixes #1010
How has this been tested?
Locally + unit tests.
Types of changes
Checklist: