Merge database tool #47

droidmonkey · 2016-10-18T02:39:04Z

Description

This PR institutes a manner in which to merge an unopened database to the currently open and selected database. Fixes #22.

Motivation and Context

This is an oft-requested feature and has been coded and waiting in the KeePassX repository for several years.

How Has This Been Tested?

Travis CI

Types of changes

New feature (non-breaking change which adds functionality)

Checklist:

✅ My code follows the code style of this project.
✅ My change requires a change to the documentation.
➖ I have updated the documentation accordingly.
✅ I have read the CONTRIBUTING document.
❌ I have added tests to cover my changes.
✅ All new and existing tests passed.

the GUI. Add tests.

server.

* Add a MergeMode for Groups, which determines what conflict resolution will do * Default to KeepNewer mode, to be in line with KeePass2 * MergeMode can be inherited * Add tests # Conflicts: # src/core/Group.cpp

…keepassx@ce4abdc

TheZ3ro

We must convert each Q_FOREACH to C++11 foreach.
And also check that all the UI fields have appropriate names and can be translated

TheZ3ro · 2016-10-18T06:23:07Z

src/core/Group.cpp

+Entry* Group::findEntry(const Uuid& uuid)
+{
+    Q_ASSERT(!uuid.isNull());
+    Q_FOREACH (Entry* entry, m_entries) {


We need to convert Q_FOREACH to C++11 foreach. (Q_FOREACH will be deprecated soon)

TheZ3ro · 2016-10-18T06:23:26Z

src/core/Group.cpp

+void Group::merge(const Group* other)
+{
+    // merge entries
+    Q_FOREACH (Entry* entry, other->entries()) {


TheZ3ro · 2016-10-18T06:23:34Z

src/core/Group.cpp

+    }
+
+    // merge groups (recursively)
+    Q_FOREACH (Group* group, other->children()) {


TheZ3ro · 2016-10-18T06:23:41Z

src/core/Group.cpp

+
+Group* Group::findChildByName(const QString& name)
+{
+    Q_FOREACH (Group* group, m_children) {


TheZ3ro · 2016-10-18T06:24:24Z

src/gui/DatabaseTabWidget.cpp

+    if (changedFiles.isEmpty())
+        return;
+
+    Q_FOREACH (DatabaseManagerStruct dbStruct, m_dbList) {


TheZ3ro · 2016-10-18T06:27:42Z

src/gui/SettingsWidgetGeneral.ui

@@ -56,54 +56,80 @@
    </widget>
   </item>
   <item row="5" column="0">
+    <widget class="QLabel" name="label_2">


The name of this widget is generic (label_2) should be something else with sense.

TheZ3ro · 2016-10-19T11:46:12Z

I've proposed a fix for the bugs I've found.
IMO we can now merge.

TheZ3ro · 2016-10-27T08:19:44Z

Any news about this?

droidmonkey · 2016-10-27T10:31:38Z

I haven't actually compiled this and tried it out yet. I'll do that today and merge it in. Been really focused on the new search functionality 😉

TheZ3ro · 2016-10-27T10:36:47Z

Ok no problem 😉
Just asking because it was opened 9 days ago

droidmonkey · 2016-10-27T11:27:08Z

src/gui/DatabaseTabWidget.cpp

@@ -271,7 +272,7 @@ void DatabaseTabWidget::checkReloadDatabases()
    if (changedFiles.isEmpty())
        return;

-    Q_FOREACH (DatabaseManagerStruct dbStruct, m_dbList) {
+    for (DatabaseManagerStruct dbStruct: asConst(m_dbList)) {


I get a crash here when I try to autoreload a database.

Can you test if with the Q_FOREACH works or crash as well?

The Q_FOREACH does not crash.

droidmonkey · 2016-10-27T11:27:38Z

I got a crash on autoreload, see comment above

Also, when it asks if I want to "discard my changes and load new database" if I hit NO it should taint the current database file (ie, add the asterisk) so that it prompts to save on close.

TheZ3ro · 2016-10-27T13:57:57Z

@droidmonkey for me crash with the following message: 'src/keepassx test2.kdbx' terminated by signal SIGSEGV (Address boundary error)

Also you are right about the tainting.
A nice thing should be adding a button to merge database in the "discard my changes and load new database" dialog

* All gui tests are run on an unaltered starter database * All gui tests are self contained * Removed clearing of Search Widget text when hiding

* Searches are saved between databases * Search is cleared when all databases are closed * Implemented case sensitive and group searching

# Conflicts: # src/gui/MainWindow.cpp

…ture/merge-db-#22 * Reframed gui test case for merging based on fixed tests * Removed considerations for dbwidget search during merge with new search feature

…autoreload a file change.

droidmonkey · 2016-10-30T12:47:12Z

I fixed all the issues noted, this is ready for a third set of eyes. I would still like to have tests written for the autoreload.

TheZ3ro · 2016-10-30T14:01:08Z

Do you have rebased the #67 in this PR? (I see commit related to Search)
If so we need to approve and merge that PR

droidmonkey · 2016-10-30T19:24:10Z

Yah I merged the search changes in and there were several conflicts. We should approve search first then this one.

droidmonkey · 2016-11-02T11:55:11Z

I intend to add the auto reload tests tonight and merge this in. Last call!

droidmonkey · 2016-11-03T03:40:56Z

I am getting crashes all over the place with the autoreload feature. This needs a lot more work before it can be merged. It is a result of using the iterator to go over the db list... my fault

TheZ3ro · 2016-11-03T07:58:18Z

Have you tried using for and removing the asConst from m_dbList ?

droidmonkey · 2016-11-07T02:48:21Z

Based on my debugging of the autoreload, these crashes occurred prior to the C++ 11 transition. The whole autoreload process needs to be redone in a different manner because it leaves behind a mess in the inner workings of the app.

I'm going to excise the auto reload part of this PR and move that code to its own feature branch. There is likely going to need a bit of replumbing in the way databases are loaded and metadata (in the tab widget) moved into proper classes. A lot of information gets out of sync by maintaining it in two different places.

TheZ3ro · 2016-11-07T09:05:49Z

Nice to know. (Ironically)

…4fb80d)

monomon and others added 5 commits October 6, 2016 17:24

Add capability to merge a database into the currently open one, also in

ac5c1f4

the GUI. Add tests.

Update check for presence of uuid to use isNull(), was upsetting the CI

ac64617

server.

Run your tests, stoopid

9ac32be

Add a function to resolve conflicts when merging groups.

16d7d2e

* Add a MergeMode for Groups, which determines what conflict resolution will do * Default to KeepNewer mode, to be in line with KeePass2 * MergeMode can be inherited * Add tests # Conflicts: # src/core/Group.cpp

Detect background changes to database file. Merged from daniellandau/…

e4fb80d

…keepassx@ce4abdc

droidmonkey added this to the v2.1.0 milestone Oct 18, 2016

TheZ3ro suggested changes Oct 18, 2016

View reviewed changes

🐛 replace Q_FOREACH, use proper name in UI

b627c8f

TheZ3ro approved these changes Oct 19, 2016

View reviewed changes

Merge remote-tracking branch 'origin/develop' into feature/merge-db-#22

758ba9e

droidmonkey commented Oct 27, 2016

View reviewed changes

droidmonkey added 9 commits October 29, 2016 09:23

Rev 1 of search bar always visible #15

46dc39c

Completely updated the Gui tests to be atomic.

d815642

* All gui tests are run on an unaltered starter database * All gui tests are self contained * Removed clearing of Search Widget text when hiding

Test fix for travis

5fb8077

Search box gets disabled instead of hidden; Fixed Travis CI gui tests

360619f

Finalized new search behavior.

069ebd1

* Searches are saved between databases * Search is cleared when all databases are closed * Implemented case sensitive and group searching

Search is disabled on first load and fixed escape to end search.

3c3ecdb

Merge remote-tracking branch 'origin/develop' into feature/merge-db-#22

aa6994d

# Conflicts: # src/gui/MainWindow.cpp

Merge remote-tracking branch 'origin/feature/search-viz-#15' into fea…

8945ee7

…ture/merge-db-#22 * Reframed gui test case for merging based on fixed tests * Removed considerations for dbwidget search during merge with new search feature

Fixed crash in checkReloadDatabases and taint database if you do not …

c0dbc45

…autoreload a file change.

Merge remote-tracking branch 'origin/develop' into feature/merge-db-#22

72ef9fe

Merge remote-tracking branch 'origin/develop' into feature/merge-db-#22

2ee92fe

Merge remote-tracking branch 'origin/develop' into feature/merge-db-#22

4f7cdb5

Reverted Detect background changes to database file. (reverted commit e…

9066cd2

…4fb80d)

droidmonkey changed the title ~~Merge database and Autoreload database~~ Merge database tool Nov 8, 2016

droidmonkey merged commit e25cd9b into develop Nov 8, 2016

droidmonkey deleted the feature/merge-db-#22 branch November 8, 2016 03:37

TheZ3ro mentioned this pull request Nov 26, 2016

Support bidirectional sync with a different database file [$10] #90

Closed

louib mentioned this pull request Jan 6, 2017

Feature : merge databases program #143

Merged

Germano0 mentioned this pull request Dec 15, 2021

keepassxc stuck #7234

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Merge database tool #47

Merge database tool #47

droidmonkey commented Oct 18, 2016 •

edited

Loading

TheZ3ro left a comment

TheZ3ro Oct 18, 2016

TheZ3ro Oct 18, 2016

TheZ3ro Oct 18, 2016

TheZ3ro Oct 18, 2016

TheZ3ro Oct 18, 2016

TheZ3ro Oct 18, 2016

TheZ3ro commented Oct 19, 2016 •

edited

Loading

TheZ3ro commented Oct 27, 2016

droidmonkey commented Oct 27, 2016 •

edited

Loading

TheZ3ro commented Oct 27, 2016 •

edited

Loading

droidmonkey Oct 27, 2016

TheZ3ro Oct 27, 2016

droidmonkey Oct 27, 2016

droidmonkey commented Oct 27, 2016 •

edited

Loading

TheZ3ro commented Oct 27, 2016

droidmonkey commented Oct 30, 2016 •

edited

Loading

TheZ3ro commented Oct 30, 2016

droidmonkey commented Oct 30, 2016

droidmonkey commented Nov 2, 2016

droidmonkey commented Nov 3, 2016 •

edited

Loading

TheZ3ro commented Nov 3, 2016

droidmonkey commented Nov 7, 2016

TheZ3ro commented Nov 7, 2016

Merge database tool #47

Merge database tool #47

Conversation

droidmonkey commented Oct 18, 2016 • edited Loading

Description

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

TheZ3ro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheZ3ro commented Oct 19, 2016 • edited Loading

TheZ3ro commented Oct 27, 2016

droidmonkey commented Oct 27, 2016 • edited Loading

TheZ3ro commented Oct 27, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

droidmonkey commented Oct 27, 2016 • edited Loading

TheZ3ro commented Oct 27, 2016

droidmonkey commented Oct 30, 2016 • edited Loading

TheZ3ro commented Oct 30, 2016

droidmonkey commented Oct 30, 2016

droidmonkey commented Nov 2, 2016

droidmonkey commented Nov 3, 2016 • edited Loading

TheZ3ro commented Nov 3, 2016

droidmonkey commented Nov 7, 2016

TheZ3ro commented Nov 7, 2016

droidmonkey commented Oct 18, 2016 •

edited

Loading

TheZ3ro commented Oct 19, 2016 •

edited

Loading

droidmonkey commented Oct 27, 2016 •

edited

Loading

TheZ3ro commented Oct 27, 2016 •

edited

Loading

droidmonkey commented Oct 27, 2016 •

edited

Loading

droidmonkey commented Oct 30, 2016 •

edited

Loading

droidmonkey commented Nov 3, 2016 •

edited

Loading