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
This commit is contained in:
Loose Cannon 2026-02-25 13:44:49 -05:00
parent da85170801
commit 15246fa231
9 changed files with 66 additions and 29 deletions

View file

@ -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);

View file

@ -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<Database> newDb, QSharedPointer<Database> oldDb);
bool setSharingEnabled(QSharedPointer<Database> db, bool enabled);

View file

@ -23,6 +23,7 @@
#include "gui/MessageBox.h"
#include "keeshare/KeeShare.h"
#include <QRegularExpressionValidator>
#include <QStandardItemModel>
#include <QStandardPaths>
#include <QTextStream>
@ -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());

View file

@ -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::Result> 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::Result> 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);
}

View file

@ -89,6 +89,7 @@ private:
QMap<QString, QSharedPointer<FileWatcher>> m_fileWatchers;
QMap<QString, QSharedPointer<QFileSystemWatcher>> m_dirWatchers;
bool m_inFileUpdate = false;
bool m_inDirUpdate = false;
bool m_enabled = true;
};

View file

@ -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),

View file

@ -100,9 +100,6 @@
<property name="alignment">
<set>Qt::AlignRight|Qt::AlignTrailing|Qt::AlignVCenter</set>
</property>
<property name="toolTip">
<string>File path for classic mode, or directory path for per-device sync</string>
</property>
</widget>
</item>
<item row="1" column="1">
@ -127,6 +124,9 @@
<property name="accessibleName">
<string>Path to share file field</string>
</property>
<property name="toolTip">
<string>File path for classic mode, or directory path for per-device sync</string>
</property>
</widget>
</item>
<item>

View file

@ -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<KeeShareSettings::Type>(type);
QCOMPARE(reference.isImporting(), expectedImporting);
QCOMPARE(reference.isExporting(), expectedExporting);
}
void TestSharing::testPerDeviceModeImportExport_data()
{
QTest::addColumn<QString>("path");
QTest::addColumn<int>("type");
QTest::addColumn<bool>("expectedImporting");
QTest::addColumn<bool>("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<Botan::RSA_PrivateKey> TestSharing::stubkey(int index)
{
static QMap<int, QSharedPointer<Botan::RSA_PrivateKey>> keys;

View file

@ -38,6 +38,8 @@ private slots:
void testSettingsSerialization_data();
void testPerDeviceMode();
void testPerDeviceMode_data();
void testPerDeviceModeImportExport();
void testPerDeviceModeImportExport_data();
private:
const QSharedPointer<Botan::RSA_PrivateKey> stubkey(int index = 0);