From 15246fa2313c482bd829a8dcf03f9c9c007a04f1 Mon Sep 17 00:00:00 2001 From: Loose Cannon Date: Wed, 25 Feb 2026 13:44:49 -0500 Subject: [PATCH] address copilot review: split update flags, fix device ID handling, remove dead code - split m_inFileUpdate into separate file/dir flags to prevent dropped updates - check mkpath return value and report error on failure - add QRegularExpressionValidator and length cap (32) to device ID - clear device ID config when field emptied (allows auto-detect reset) - remove unused hasPerDeviceConfig/perDeviceSyncPath and KeeShare_PerDeviceSync - skip cycleImportExport warning in per-device mode (not a real conflict) - move tooltip from pathLabel to pathEdit input field - add testPerDeviceModeImportExport test coverage --- src/keeshare/KeeShare.cpp | 17 ++-------- src/keeshare/KeeShare.h | 3 -- src/keeshare/SettingsWidgetKeeShare.cpp | 7 ++++ src/keeshare/ShareObserver.cpp | 17 ++++++---- src/keeshare/ShareObserver.h | 1 + .../group/EditGroupWidgetKeeShare.cpp | 8 +++-- src/keeshare/group/EditGroupWidgetKeeShare.ui | 6 ++-- tests/TestSharing.cpp | 34 +++++++++++++++++++ tests/TestSharing.h | 2 ++ 9 files changed, 66 insertions(+), 29 deletions(-) diff --git a/src/keeshare/KeeShare.cpp b/src/keeshare/KeeShare.cpp index 3ce03ebeb..65016270c 100644 --- a/src/keeshare/KeeShare.cpp +++ b/src/keeshare/KeeShare.cpp @@ -29,7 +29,6 @@ namespace { static const QString KeeShare_Reference("KeeShare/Reference"); - static const QString KeeShare_PerDeviceSync("KeeShare/PerDeviceSync"); } KeeShare* KeeShare::m_instance = nullptr; @@ -80,9 +79,10 @@ QString KeeShare::deviceId() void KeeShare::setDeviceId(const QString& id) { - // Sanitize to [A-Za-z0-9] only + // Sanitize to [A-Za-z0-9] only and enforce max length QString sanitized = id; sanitized.remove(QRegularExpression("[^A-Za-z0-9]")); + sanitized.truncate(32); config()->set(Config::KeeShare_DeviceId, sanitized); } @@ -145,19 +145,6 @@ void KeeShare::setReferenceTo(Group* group, const KeeShareSettings::Reference& r customData->set(KeeShare_Reference, serialized.toUtf8().toBase64()); } -bool KeeShare::hasPerDeviceConfig(const Group* group) -{ - return group && group->customData()->contains(KeeShare_PerDeviceSync); -} - -QString KeeShare::perDeviceSyncPath(const Group* group) -{ - if (!group || !group->customData()->contains(KeeShare_PerDeviceSync)) { - return {}; - } - return group->customData()->value(KeeShare_PerDeviceSync); -} - bool KeeShare::isEnabled(const Group* group) { const auto reference = KeeShare::referenceOf(group); diff --git a/src/keeshare/KeeShare.h b/src/keeshare/KeeShare.h index 51f6f9859..0a5e91af4 100644 --- a/src/keeshare/KeeShare.h +++ b/src/keeshare/KeeShare.h @@ -67,9 +67,6 @@ public: static void setReferenceTo(Group* group, const KeeShareSettings::Reference& reference); static QString referenceTypeLabel(const KeeShareSettings::Reference& reference); - static bool hasPerDeviceConfig(const Group* group); - static QString perDeviceSyncPath(const Group* group); - void connectDatabase(QSharedPointer newDb, QSharedPointer oldDb); bool setSharingEnabled(QSharedPointer db, bool enabled); diff --git a/src/keeshare/SettingsWidgetKeeShare.cpp b/src/keeshare/SettingsWidgetKeeShare.cpp index 472bf0a13..850864d76 100644 --- a/src/keeshare/SettingsWidgetKeeShare.cpp +++ b/src/keeshare/SettingsWidgetKeeShare.cpp @@ -23,6 +23,7 @@ #include "gui/MessageBox.h" #include "keeshare/KeeShare.h" +#include #include #include #include @@ -33,6 +34,9 @@ SettingsWidgetKeeShare::SettingsWidgetKeeShare(QWidget* parent) { m_ui->setupUi(this); + m_ui->deviceIdEdit->setValidator( + new QRegularExpressionValidator(QRegularExpression("[A-Za-z0-9]{0,32}"), this)); + connect(m_ui->ownCertificateSignerEdit, SIGNAL(textChanged(QString)), SLOT(setVerificationExporter(QString))); connect(m_ui->generateOwnCerticateButton, SIGNAL(clicked(bool)), SLOT(generateCertificate())); } @@ -73,6 +77,9 @@ void SettingsWidgetKeeShare::saveSettings() auto deviceId = m_ui->deviceIdEdit->text().trimmed(); if (!deviceId.isEmpty()) { KeeShare::setDeviceId(deviceId); + } else { + // Clear stored ID so it will be auto-detected next time + config()->set(Config::KeeShare_DeviceId, QString()); } config()->set(Config::KeeShare_QuietSuccess, m_ui->quietSuccessCheckBox->isChecked()); diff --git a/src/keeshare/ShareObserver.cpp b/src/keeshare/ShareObserver.cpp index 2b78c2c86..e391b91a4 100644 --- a/src/keeshare/ShareObserver.cpp +++ b/src/keeshare/ShareObserver.cpp @@ -270,16 +270,16 @@ void ShareObserver::handleDirectoryUpdated(const QString& dirPath) dirWatcher->addPath(dirPath); } - if (!m_inFileUpdate) { + if (!m_inDirUpdate) { QTimer::singleShot(100, this, [this, dirPath] { auto shareGroup = m_shareToGroup.value(dirPath); if (!shareGroup) { - m_inFileUpdate = false; + m_inDirUpdate = false; return; } auto shareRef = KeeShare::referenceOf(shareGroup); auto results = importPerDeviceShares(dirPath, shareRef, shareGroup); - m_inFileUpdate = false; + m_inDirUpdate = false; QStringList success; QStringList warning; @@ -300,7 +300,7 @@ void ShareObserver::handleDirectoryUpdated(const QString& dirPath) } notifyAbout(success, warning, error); }); - m_inFileUpdate = true; + m_inDirUpdate = true; } } @@ -406,7 +406,12 @@ QList ShareObserver::exportShares() // Per-device mode: export to {directory}/{DEVICE_ID}.kdbx QDir dir(resolvedPath); if (!dir.exists()) { - dir.mkpath("."); + if (!dir.mkpath(".")) { + results << Result{resolvedPath, + Result::Error, + tr("Could not create directory %1").arg(resolvedPath)}; + continue; + } } const auto deviceFile = dir.absoluteFilePath(KeeShare::deviceId() + ".kdbx"); @@ -418,7 +423,7 @@ QList ShareObserver::exportShares() results << ShareExport::intoContainer(deviceFile, reference.config, reference.group); - // Resume directory watcher + // Resume directory watcher (also add path if it was newly created) if (dirWatcher) { dirWatcher->addPath(resolvedPath); } diff --git a/src/keeshare/ShareObserver.h b/src/keeshare/ShareObserver.h index b888e7ff2..ac1a2e645 100644 --- a/src/keeshare/ShareObserver.h +++ b/src/keeshare/ShareObserver.h @@ -89,6 +89,7 @@ private: QMap> m_fileWatchers; QMap> m_dirWatchers; bool m_inFileUpdate = false; + bool m_inDirUpdate = false; bool m_enabled = true; }; diff --git a/src/keeshare/group/EditGroupWidgetKeeShare.cpp b/src/keeshare/group/EditGroupWidgetKeeShare.cpp index c9eecef32..62ab6c98f 100644 --- a/src/keeshare/group/EditGroupWidgetKeeShare.cpp +++ b/src/keeshare/group/EditGroupWidgetKeeShare.cpp @@ -149,8 +149,12 @@ void EditGroupWidgetKeeShare::updateSharingState() } multipleImport |= other.isImporting() && reference.isImporting(); conflictExport |= other.isExporting() && reference.isExporting(); - cycleImportExport |= - (other.isImporting() && reference.isExporting()) || (other.isExporting() && reference.isImporting()); + // In per-device mode, import+export to the same directory is expected + // (export writes own device file, import reads other devices' files) + if (!reference.isPerDeviceMode()) { + cycleImportExport |= + (other.isImporting() && reference.isExporting()) || (other.isExporting() && reference.isImporting()); + } } if (conflictExport) { m_ui->messageWidget->showMessage(tr("%1 is already being exported by this database.").arg(reference.path), diff --git a/src/keeshare/group/EditGroupWidgetKeeShare.ui b/src/keeshare/group/EditGroupWidgetKeeShare.ui index 626272f52..582165efc 100644 --- a/src/keeshare/group/EditGroupWidgetKeeShare.ui +++ b/src/keeshare/group/EditGroupWidgetKeeShare.ui @@ -100,9 +100,6 @@ Qt::AlignRight|Qt::AlignTrailing|Qt::AlignVCenter - - File path for classic mode, or directory path for per-device sync - @@ -127,6 +124,9 @@ Path to share file field + + File path for classic mode, or directory path for per-device sync + diff --git a/tests/TestSharing.cpp b/tests/TestSharing.cpp index 7e63b3e02..4034a9882 100644 --- a/tests/TestSharing.cpp +++ b/tests/TestSharing.cpp @@ -206,6 +206,40 @@ void TestSharing::testPerDeviceMode_data() QTest::newRow("directory with dots") << "/some/path.d/sync" << true; } +void TestSharing::testPerDeviceModeImportExport() +{ + QFETCH(QString, path); + QFETCH(int, type); + QFETCH(bool, expectedImporting); + QFETCH(bool, expectedExporting); + + KeeShareSettings::Reference reference; + reference.path = path; + reference.type = static_cast(type); + + QCOMPARE(reference.isImporting(), expectedImporting); + QCOMPARE(reference.isExporting(), expectedExporting); +} + +void TestSharing::testPerDeviceModeImportExport_data() +{ + QTest::addColumn("path"); + QTest::addColumn("type"); + QTest::addColumn("expectedImporting"); + QTest::addColumn("expectedExporting"); + + QTest::newRow("per-device sync imports") + << "/some/dir" << int(KeeShareSettings::SynchronizeWith) << true << true; + QTest::newRow("per-device import only") + << "/some/dir" << int(KeeShareSettings::ImportFrom) << true << false; + QTest::newRow("per-device export only") + << "/some/dir" << int(KeeShareSettings::ExportTo) << false << true; + QTest::newRow("classic file sync") + << "/some/dir/share.kdbx" << int(KeeShareSettings::SynchronizeWith) << true << true; + QTest::newRow("inactive per-device") + << "/some/dir" << int(KeeShareSettings::Inactive) << false << false; +} + const QSharedPointer TestSharing::stubkey(int index) { static QMap> keys; diff --git a/tests/TestSharing.h b/tests/TestSharing.h index 2947479c0..1c5d5cc20 100644 --- a/tests/TestSharing.h +++ b/tests/TestSharing.h @@ -38,6 +38,8 @@ private slots: void testSettingsSerialization_data(); void testPerDeviceMode(); void testPerDeviceMode_data(); + void testPerDeviceModeImportExport(); + void testPerDeviceModeImportExport_data(); private: const QSharedPointer stubkey(int index = 0);