Fix several cases of incorrect handling of a Group/Entry's LastModification/Creation time

src/core/Entry.cpp

@@ -946,10 +946,15 @@ Entry* Entry::clone(CloneFlags flags) const
 
     if (flags & CloneResetTimeInfo) {
         QDateTime now = Clock::currentDateTimeUtc();
-        entry->m_data.timeInfo.setCreationTime(now);
-        entry->m_data.timeInfo.setLastModificationTime(now);
-        entry->m_data.timeInfo.setLastAccessTime(now);
-        entry->m_data.timeInfo.setLocationChanged(now);
+        if (flags & CloneResetCreationTime) {
+            entry->m_data.timeInfo.setCreationTime(now);
+        }
+        if (flags & CloneResetLastAccessTime) {
+            entry->m_data.timeInfo.setLastAccessTime(now);
+        }
+        if (flags & CloneResetLocationChangedTime) {
+            entry->m_data.timeInfo.setLocationChanged(now);
+        }
     }
 
     if (flags & CloneRenameTitle) {
@@ -1267,10 +1272,8 @@ void Entry::setGroup(Group* group, bool trackPrevious)
             m_group->database()->addDeletedObject(m_uuid);
 
             // copy custom icon to the new database
-            if (!iconUuid().isNull() && group->database() && m_group->database()->metadata()->hasCustomIcon(iconUuid())
-                && !group->database()->metadata()->hasCustomIcon(iconUuid())) {
-                group->database()->metadata()->addCustomIcon(iconUuid(),
-                                                             m_group->database()->metadata()->customIcon(iconUuid()));
+            if (group->database()) {
+                group->database()->metadata()->copyCustomIcon(iconUuid(), m_group->database()->metadata());
             }
         } else if (trackPrevious && m_group->database() && group != m_group) {
             setPreviousParentGroup(m_group);
@@ -1487,7 +1490,10 @@ QUuid Entry::previousParentGroupUuid() const
 
 void Entry::setPreviousParentGroupUuid(const QUuid& uuid)
 {
+    bool prevUpdateTimeinfo = m_updateTimeinfo;
+    m_updateTimeinfo = false; // prevent update of LastModificationTime
     set(m_data.previousParentGroupUuid, uuid);
+    m_updateTimeinfo = prevUpdateTimeinfo;
 }
 
 void Entry::setPreviousParentGroup(const Group* group)

src/core/Entry.h

@@ -175,13 +175,18 @@ class Entry : public ModifiableObject
     {
         CloneNoFlags = 0,
         CloneNewUuid = 1, // generate a random uuid for the clone
-        CloneResetTimeInfo = 2, // set all TimeInfo attributes to the current time
-        CloneIncludeHistory = 4, // clone the history items
+        CloneResetCreationTime = 2, // set timeInfo.CreationTime to the current time
+        CloneResetLastAccessTime = 4, // set timeInfo.LastAccessTime to the current time
+        CloneResetLocationChangedTime = 8, // set timeInfo.LocationChangedTime to the current time
+        CloneIncludeHistory = 16, // clone the history items
+        CloneRenameTitle = 32, // add "-Clone" after the original title
+        CloneUserAsRef = 64, // Add the user as a reference to the original entry
+        ClonePassAsRef = 128, // Add the password as a reference to the original entry
+
+        CloneResetTimeInfo = CloneResetCreationTime | CloneResetLastAccessTime | CloneResetLocationChangedTime,
+        CloneExactCopy = CloneIncludeHistory,
+        CloneCopy = CloneExactCopy | CloneNewUuid | CloneResetTimeInfo,
         CloneDefault = CloneNewUuid | CloneResetTimeInfo,
-        CloneCopy = CloneNewUuid | CloneResetTimeInfo | CloneIncludeHistory,
-        CloneRenameTitle = 8, // add "-Clone" after the original title
-        CloneUserAsRef = 16, // Add the user as a reference to the original entry
-        ClonePassAsRef = 32, // Add the password as a reference to the original entry
     };
     Q_DECLARE_FLAGS(CloneFlags, CloneFlag)
 

src/core/Group.cpp

@@ -77,11 +77,11 @@ Group::~Group()
     cleanupParent();
 }
 
-template <class P, class V> inline bool Group::set(P& property, const V& value)
+template <class P, class V> inline bool Group::set(P& property, const V& value, bool preserveTimeinfo)
 {
     if (property != value) {
         property = value;
-        emitModified();
+        emitModifiedEx(preserveTimeinfo);
         return true;
     } else {
         return false;
@@ -440,6 +440,15 @@ const Group* Group::parentGroup() const
     return m_parent;
 }
 
+void Group::emitModifiedEx(bool preserveTimeinfo) {
+    bool prevUpdateTimeinfo = m_updateTimeinfo;
+    if (preserveTimeinfo) {
+        m_updateTimeinfo = false; // prevent update of LastModificationTime
+    }
+    emitModified();
+    m_updateTimeinfo = prevUpdateTimeinfo;
+}
+
 void Group::setParent(Group* parent, int index, bool trackPrevious)
 {
     Q_ASSERT(parent);
@@ -469,9 +478,8 @@ void Group::setParent(Group* parent, int index, bool trackPrevious)
             recCreateDelObjects();
 
             // copy custom icon to the new database
-            if (!iconUuid().isNull() && parent->m_db && m_db->metadata()->hasCustomIcon(iconUuid())
-                && !parent->m_db->metadata()->hasCustomIcon(iconUuid())) {
-                parent->m_db->metadata()->addCustomIcon(iconUuid(), m_db->metadata()->customIcon(iconUuid()));
+            if (parent->m_db) {
+                parent->m_db->metadata()->copyCustomIcon(iconUuid(), m_db->metadata());
             }
         }
         if (m_db != parent->m_db) {
@@ -497,7 +505,7 @@ void Group::setParent(Group* parent, int index, bool trackPrevious)
         m_data.timeInfo.setLocationChanged(Clock::currentDateTimeUtc());
     }
 
-    emitModified();
+    emitModifiedEx(true);
 
     if (!moveWithinDatabase) {
         emit groupAdded();
@@ -946,12 +954,16 @@ Group* Group::clone(Entry::CloneFlags entryFlags, Group::CloneFlags groupFlags)
 
     clonedGroup->setUpdateTimeinfo(true);
     if (groupFlags & Group::CloneResetTimeInfo) {
-
         QDateTime now = Clock::currentDateTimeUtc();
-        clonedGroup->m_data.timeInfo.setCreationTime(now);
-        clonedGroup->m_data.timeInfo.setLastModificationTime(now);
-        clonedGroup->m_data.timeInfo.setLastAccessTime(now);
-        clonedGroup->m_data.timeInfo.setLocationChanged(now);
+        if (groupFlags & Group::CloneResetCreationTime) {
+            clonedGroup->m_data.timeInfo.setCreationTime(now);
+        }
+        if (groupFlags & Group::CloneResetLastAccessTime) {
+            clonedGroup->m_data.timeInfo.setLastAccessTime(now);
+        }
+        if (groupFlags & Group::CloneResetLocationChangedTime) {
+            clonedGroup->m_data.timeInfo.setLocationChanged(now);
+        }
     }
 
     if (groupFlags & Group::CloneRenameTitle) {
@@ -983,7 +995,7 @@ void Group::addEntry(Entry* entry)
         connect(entry, &Entry::modified, m_db, &Database::markAsModified);
     }
 
-    emitModified();
+    emitModifiedEx(true);
     emit entryAdded(entry);
 }
 
@@ -1000,7 +1012,7 @@ void Group::removeEntry(Entry* entry)
         entry->disconnect(m_db);
     }
     m_entries.removeAll(entry);
-    emitModified();
+    emitModifiedEx(true);
     emit entryRemoved(entry);
 }
 
@@ -1071,7 +1083,7 @@ void Group::cleanupParent()
     if (m_parent) {
         emit groupAboutToRemove(this);
         m_parent->m_children.removeAll(this);
-        emitModified();
+        emitModifiedEx(true);
         emit groupRemoved();
     }
 }
@@ -1222,7 +1234,7 @@ void Group::sortChildrenRecursively(bool reverse)
         child->sortChildrenRecursively(reverse);
     }
 
-    emitModified();
+    emitModifiedEx(true);
 }
 
 const Group* Group::previousParentGroup() const
@@ -1240,7 +1252,7 @@ QUuid Group::previousParentGroupUuid() const
 
 void Group::setPreviousParentGroupUuid(const QUuid& uuid)
 {
-    set(m_data.previousParentGroupUuid, uuid);
+    set(m_data.previousParentGroupUuid, uuid, true);
 }
 
 void Group::setPreviousParentGroup(const Group* group)

src/core/Group.h

@@ -47,10 +47,16 @@ class Group : public ModifiableObject
     {
         CloneNoFlags = 0,
         CloneNewUuid = 1, // generate a random uuid for the clone
-        CloneResetTimeInfo = 2, // set all TimeInfo attributes to the current time
-        CloneIncludeEntries = 4, // clone the group entries
-        CloneDefault = CloneNewUuid | CloneResetTimeInfo | CloneIncludeEntries,
-        CloneRenameTitle = 8, // add "- Clone" after the original title
+        CloneResetCreationTime = 2, // set timeInfo.CreationTime to the current time
+        CloneResetLastAccessTime = 4, // set timeInfo.LastAccessTime to the current time
+        CloneResetLocationChangedTime = 8, // set timeInfo.LocationChangedTime to the current time
+        CloneIncludeEntries = 16, // clone the group entries
+        CloneRenameTitle = 32, // add "- Clone" after the original title
+
+        CloneResetTimeInfo = CloneResetCreationTime | CloneResetLastAccessTime | CloneResetLocationChangedTime,
+        CloneExactCopy = CloneIncludeEntries,
+        CloneCopy = CloneExactCopy | CloneNewUuid | CloneResetTimeInfo,
+        CloneDefault = CloneCopy,
     };
     Q_DECLARE_FLAGS(CloneFlags, CloneFlag)
 
@@ -204,8 +210,9 @@ private slots:
     void updateTimeinfo();
 
 private:
-    template <class P, class V> bool set(P& property, const V& value);
+    template <class P, class V> bool set(P& property, const V& value, bool preserveTimeinfo = false);
 
+    void emitModifiedEx(bool preserveTimeinfo);
     void setParent(Database* db);
 
     void connectDatabaseSignalsRecursive(Database* db);

src/core/Metadata.cpp

@@ -419,14 +419,21 @@ QUuid Metadata::findCustomIcon(const QByteArray& candidate)
     return m_customIconsHashes.value(hash, QUuid());
 }
 
+void Metadata::copyCustomIcon(const QUuid& iconUuid, const Metadata* otherMetadata)
+{
+    if (iconUuid.isNull()) { 
+        return;
+    }
+    Q_ASSERT(otherMetadata->hasCustomIcon(iconUuid));
+    if (!hasCustomIcon(iconUuid) && otherMetadata->hasCustomIcon(iconUuid)) {
+        addCustomIcon(iconUuid, otherMetadata->customIcon(iconUuid));
+    }
+}
+
 void Metadata::copyCustomIcons(const QSet<QUuid>& iconList, const Metadata* otherMetadata)
 {
     for (const QUuid& uuid : iconList) {
-        Q_ASSERT(otherMetadata->hasCustomIcon(uuid));
-
-        if (!hasCustomIcon(uuid) && otherMetadata->hasCustomIcon(uuid)) {
-            addCustomIcon(uuid, otherMetadata->customIcon(uuid));
-        }
+        copyCustomIcon(uuid, otherMetadata);
     }
 }
 

src/core/Metadata.h

@@ -138,6 +138,7 @@ class Metadata : public ModifiableObject
                        const QString& name = {},
                        const QDateTime& lastModified = {});
     void removeCustomIcon(const QUuid& uuid);
+    void copyCustomIcon(const QUuid& iconUuid, const Metadata* otherMetadata);
     void copyCustomIcons(const QSet<QUuid>& iconList, const Metadata* otherMetadata);
     QUuid findCustomIcon(const QByteArray& candidate);
     void setRecycleBinEnabled(bool value);

src/gui/group/GroupModel.cpp

@@ -258,14 +258,16 @@ bool GroupModel::dropMimeData(const QMimeData* data,
         Group* group = dragGroup;
 
         if (sourceDb != targetDb) {
-            QSet<QUuid> customIcons = group->customIconsRecursive();
-            targetDb->metadata()->copyCustomIcons(customIcons, sourceDb->metadata());
+            targetDb->metadata()->copyCustomIcons(group->customIconsRecursive(), sourceDb->metadata());
 
-            // Always clone the group across db's to reset UUIDs
-            group = dragGroup->clone(Entry::CloneDefault | Entry::CloneIncludeHistory);
             if (action == Qt::MoveAction) {
-                // Remove the original group from the sourceDb
+                // For cross-db moves use a clone with new UUID but original CreationTime
+                group = dragGroup->clone(Entry::CloneFlags(Entry::CloneCopy & ~Entry::CloneResetCreationTime),
+                                         Group::CloneFlags(Group::CloneCopy & ~Group::CloneResetCreationTime));
+                // Remove the original from the sourceDb to allow this change to sync to other dbs
                 delete dragGroup;
+            } else {
+                group = dragGroup->clone(Entry::CloneCopy);
             }
         } else if (action == Qt::CopyAction) {
             group = dragGroup->clone(Entry::CloneCopy);
@@ -298,15 +300,16 @@ bool GroupModel::dropMimeData(const QMimeData* data,
             Entry* entry = dragEntry;
 
             if (sourceDb != targetDb) {
-                QUuid customIcon = entry->iconUuid();
-                if (!customIcon.isNull() && !targetDb->metadata()->hasCustomIcon(customIcon)) {
-                    targetDb->metadata()->addCustomIcon(customIcon, sourceDb->metadata()->customIcon(customIcon).data);
-                }
+                targetDb->metadata()->copyCustomIcon(entry->iconUuid(), sourceDb->metadata());
 
                 // Reset the UUID when moving across db boundary
-                entry = dragEntry->clone(Entry::CloneDefault | Entry::CloneIncludeHistory);
                 if (action == Qt::MoveAction) {
+                    // For cross-db moves use a clone with new UUID but original CreationTime
+                    entry = dragEntry->clone(Entry::CloneFlags(Entry::CloneCopy & ~Entry::CloneResetCreationTime));
+                    // Remove the original from the sourceDb to allow this change to sync to other dbs
                     delete dragEntry;
+                } else {
+                    entry = dragEntry->clone(Entry::CloneCopy);
                 }
             } else if (action == Qt::CopyAction) {
                 entry = dragEntry->clone(Entry::CloneCopy);

tests/CMakeLists.txt

@@ -168,7 +168,7 @@ if(WITH_XC_SSHAGENT)
 endif()
 
 add_unit_test(NAME testentry SOURCES TestEntry.cpp
-        LIBS ${TEST_LIBRARIES})
+        LIBS testsupport ${TEST_LIBRARIES})
 
 add_unit_test(NAME testmerge SOURCES TestMerge.cpp
         LIBS testsupport ${TEST_LIBRARIES})

tests/TestEntry.cpp

@@ -25,13 +25,34 @@
 #include "core/TimeInfo.h"
 #include "crypto/Crypto.h"
 
+#include "mock/MockClock.h"
+
 QTEST_GUILESS_MAIN(TestEntry)
 
+namespace
+{
+    MockClock* m_clock = nullptr;
+}
+
 void TestEntry::initTestCase()
 {
     QVERIFY(Crypto::init());
 }
 
+void TestEntry::init()
+{
+    Q_ASSERT(m_clock == nullptr);
+    m_clock = new MockClock(2010, 5, 5, 10, 30, 10);
+    MockClock::setup(m_clock);
+}
+
+void TestEntry::cleanup()
+{
+    MockClock::teardown();
+    m_clock = nullptr;
+}
+
+
 void TestEntry::testHistoryItemDeletion()
 {
     QScopedPointer<Entry> entry(new Entry());
@@ -109,6 +130,8 @@ void TestEntry::testClone()
     QCOMPARE(entryCloneNewUuid->timeInfo().creationTime(), entryOrg->timeInfo().creationTime());
 
     // Reset modification time
+    entryOrgTime.setLastAccessTime(Clock::datetimeUtc(60));
+    entryOrgTime.setLocationChanged(Clock::datetimeUtc(60));
     entryOrgTime.setLastModificationTime(Clock::datetimeUtc(60));
     entryOrg->setTimeInfo(entryOrgTime);
 
@@ -122,7 +145,12 @@ void TestEntry::testClone()
     QCOMPARE(entryCloneResetTime->uuid(), entryOrg->uuid());
     QCOMPARE(entryCloneResetTime->title(), QString("New Title"));
     QCOMPARE(entryCloneResetTime->historyItems().size(), 0);
+    // Cloning with CloneResetTimeInfo should affect the CreationTime, LocationChanged, LastAccessTime
     QVERIFY(entryCloneResetTime->timeInfo().creationTime() != entryOrg->timeInfo().creationTime());
+    QVERIFY(entryCloneResetTime->timeInfo().locationChanged() != entryOrg->timeInfo().locationChanged());
+    QVERIFY(entryCloneResetTime->timeInfo().lastAccessTime() != entryOrg->timeInfo().lastAccessTime());
+    // Cloning with CloneResetTimeInfo should not affect the LastModificationTime
+    QCOMPARE(entryCloneResetTime->timeInfo().lastModificationTime(), entryOrg->timeInfo().lastModificationTime());
 
     // Date back history of original entry
     Entry* firstHistoryItem = entryOrg->historyItems()[0];
@@ -768,3 +796,33 @@ void TestEntry::testPreviousParentGroup()
     QVERIFY(entry->previousParentGroupUuid() == group1->uuid());
     QVERIFY(entry->previousParentGroup() == group1);
 }
+
+void TestEntry::testTimeinfoChanges()
+{
+    Database db;
+    auto* root = db.rootGroup();
+    auto* subgroup = new Group();
+    subgroup->setUuid(QUuid::createUuid());
+    subgroup->setParent(root);
+    QDateTime startTime = Clock::currentDateTimeUtc();
+    TimeInfo startTimeinfo;
+    startTimeinfo.setCreationTime(startTime);
+    startTimeinfo.setLastModificationTime(startTime);
+    startTimeinfo.setLocationChanged(startTime);
+    startTimeinfo.setLastAccessTime(startTime);
+    m_clock->advanceMinute(1);
+
+    QScopedPointer<Entry> entry(new Entry());
+    entry->setUuid(QUuid::createUuid());
+    entry->setGroup(root);
+    entry->setTimeInfo(startTimeinfo);
+    entry->setPreviousParentGroup(subgroup);
+    // setting previous parent group should not affect the LastModificationTime
+    QCOMPARE(entry->timeInfo().lastModificationTime(), startTime);
+    entry->setGroup(subgroup);
+    // changing group should not affect LastModicationTime, CreationTime
+    QCOMPARE(entry->timeInfo().creationTime(), startTime);
+    QCOMPARE(entry->timeInfo().lastModificationTime(), startTime);
+    // changing group should affect the LocationChanged time
+    QCOMPARE(entry->timeInfo().locationChanged(), Clock::currentDateTimeUtc());
+}