From eb63bde870e27a8fdc417a226896ee2dbcac9f7c Mon Sep 17 00:00:00 2001 From: Keith Smith Date: Tue, 15 Sep 2026 15:08:37 -0600 Subject: [PATCH] Add Delete Folder to the profile tree context menu ProfileRepository::deleteFolder() removes a folder without ever deleting the profiles or subfolders inside it -- everything directly under the deleted folder shifts up to take its place (its parent, or the top level if it had none), exactly as if that one path segment were removed from each affected path. This is a labels-only operation (a "folder" is just a grouping string on each profile, not a container that owns them), so that's the least-surprising behavior versus silently bulk-deleting saved connections. Right-clicking a folder in the tree now offers "Delete Folder"; if it isn't empty, a confirmation states exactly how many profiles/subfolders will move and to where. Covered by 7 new unit tests, which caught the same class of bug fixed in 3fab2f9: the new code's own folder-path remap also bound an unguarded null QString (a folder moving to root) against the `folder_path NOT NULL` column. Fixes #20. Co-Authored-By: Claude Sonnet 5 --- src/profile_repository.cpp | 117 ++++++++++++++++++++++++++++++ src/profile_repository.h | 1 + src/profiles_window.cpp | 76 +++++++++++++++++++ src/profiles_window.h | 1 + tests/test_profile_repository.cpp | 87 ++++++++++++++++++++++ 5 files changed, 282 insertions(+) diff --git a/src/profile_repository.cpp b/src/profile_repository.cpp index 7d8e257..0072412 100644 --- a/src/profile_repository.cpp +++ b/src/profile_repository.cpp @@ -346,6 +346,123 @@ bool ProfileRepository::createFolder(const QString& folderPath) const return true; } +// Deleting a folder never destroys profiles: everything directly inside it +// (profiles and subfolders alike) is shifted up to take the deleted +// folder's place, exactly as if that one path segment had been removed +// from each of their paths. This is the least-surprising behavior for what +// is fundamentally just an organizational label, not a container that owns +// its contents. +bool ProfileRepository::deleteFolder(const QString& folderPath) const +{ + if (!QSqlDatabase::contains(m_connectionName)) { + return false; + } + + const QString normalized = normalizedFolderPath(folderPath); + if (normalized.isEmpty()) { + setLastError(QStringLiteral("Folder path is required.")); + return false; + } + + setLastError(QString()); + + QString parentPath; + const int lastSlash = normalized.lastIndexOf(QChar::fromLatin1('/')); + if (lastSlash >= 0) { + parentPath = normalized.left(lastSlash); + } + + const QString prefix = normalized + QStringLiteral("/"); + auto remap = [&](const QString& oldPath) { + if (oldPath == normalized) { + return nonNullTrimmed(parentPath); + } + const QString remainder = oldPath.mid(prefix.length()); + return nonNullTrimmed(parentPath.isEmpty() ? remainder + : parentPath + QStringLiteral("/") + remainder); + }; + + QSqlDatabase database = QSqlDatabase::database(m_connectionName); + if (!database.transaction()) { + setLastError(database.lastError().text()); + return false; + } + + // Reassign affected profiles. Matching is done in C++ (not SQL LIKE) + // so a folder name containing '%' or '_' can't be misinterpreted as a + // wildcard. + QSqlQuery selectProfiles(database); + if (!selectProfiles.exec(QStringLiteral("SELECT id, folder_path FROM profiles"))) { + setLastError(selectProfiles.lastError().text()); + database.rollback(); + return false; + } + std::vector> profileUpdates; + while (selectProfiles.next()) { + const QString path = normalizedFolderPath(selectProfiles.value(1).toString()); + if (path == normalized || path.startsWith(prefix)) { + profileUpdates.emplace_back(selectProfiles.value(0).toLongLong(), remap(path)); + } + } + for (const auto& [id, newPath] : profileUpdates) { + QSqlQuery update(database); + update.prepare(QStringLiteral("UPDATE profiles SET folder_path = ? WHERE id = ?")); + update.addBindValue(newPath); + update.addBindValue(id); + if (!update.exec()) { + setLastError(update.lastError().text()); + database.rollback(); + return false; + } + } + + // Reassign (or drop, for the folder itself) affected explicit folder + // markers the same way. + QSqlQuery selectFolders(database); + if (!selectFolders.exec(QStringLiteral("SELECT path FROM profile_folders"))) { + setLastError(selectFolders.lastError().text()); + database.rollback(); + return false; + } + QStringList affectedFolders; + while (selectFolders.next()) { + const QString path = normalizedFolderPath(selectFolders.value(0).toString()); + if (path == normalized || path.startsWith(prefix)) { + affectedFolders.push_back(path); + } + } + for (const QString& oldPath : affectedFolders) { + QSqlQuery remove(database); + remove.prepare(QStringLiteral("DELETE FROM profile_folders WHERE path = ?")); + remove.addBindValue(oldPath); + if (!remove.exec()) { + setLastError(remove.lastError().text()); + database.rollback(); + return false; + } + + const QString newPath = remap(oldPath); + if (oldPath != normalized && !newPath.isEmpty()) { + QSqlQuery insert(database); + insert.prepare(QStringLiteral("INSERT OR IGNORE INTO profile_folders(path) VALUES (?)")); + insert.addBindValue(newPath); + if (!insert.exec()) { + setLastError(insert.lastError().text()); + database.rollback(); + return false; + } + } + } + + if (!database.commit()) { + setLastError(database.lastError().text()); + database.rollback(); + return false; + } + + return true; +} + std::vector ProfileRepository::listProfiles(const QString& searchQuery, ProfileSortOrder sortOrder) const { diff --git a/src/profile_repository.h b/src/profile_repository.h index 43a8754..94680b1 100644 --- a/src/profile_repository.h +++ b/src/profile_repository.h @@ -47,6 +47,7 @@ public: ProfileSortOrder sortOrder = ProfileSortOrder::NameAsc) const; std::vector listFolders() const; bool createFolder(const QString& folderPath) const; + bool deleteFolder(const QString& folderPath) const; std::optional getProfile(qint64 id) const; std::optional createProfile(const Profile& profile) const; bool updateProfile(const Profile& profile) const; diff --git a/src/profiles_window.cpp b/src/profiles_window.cpp index 1538d29..f1ac30e 100644 --- a/src/profiles_window.cpp +++ b/src/profiles_window.cpp @@ -587,6 +587,8 @@ void ProfilesWindow::showTreeContextMenu(const QPoint& pos) const QString contextFolder = folderPathForItem(item); const bool isProfileItem = item != nullptr && item->data(0, kProfileIdRole).isValid(); + const bool isFolderItem = + item != nullptr && !isProfileItem && item->data(0, kFolderPathRole).isValid(); QMenu menu(this); QAction* newConnectionAction = menu.addAction(QStringLiteral("New Connection")); @@ -594,12 +596,16 @@ void ProfilesWindow::showTreeContextMenu(const QPoint& pos) QAction* connectAction = nullptr; QAction* editAction = nullptr; QAction* deleteAction = nullptr; + QAction* deleteFolderAction = nullptr; if (isProfileItem) { menu.addSeparator(); connectAction = menu.addAction(QStringLiteral("Connect")); editAction = menu.addAction(QStringLiteral("Edit")); deleteAction = menu.addAction(QStringLiteral("Delete")); + } else if (isFolderItem) { + menu.addSeparator(); + deleteFolderAction = menu.addAction(QStringLiteral("Delete Folder")); } QAction* chosen = menu.exec(m_profilesTree->viewport()->mapToGlobal(pos)); @@ -629,6 +635,10 @@ void ProfilesWindow::showTreeContextMenu(const QPoint& pos) deleteSelectedProfile(); return; } + if (isFolderItem && chosen == deleteFolderAction) { + deleteFolderInContext(contextFolder); + return; + } } void ProfilesWindow::createFolderInContext(const QString& baseFolderPath) @@ -666,6 +676,72 @@ void ProfilesWindow::createFolderInContext(const QString& baseFolderPath) loadProfiles(); } +void ProfilesWindow::deleteFolderInContext(const QString& folderPath) +{ + const QString normalized = normalizeFolderPathForView(folderPath); + if (normalized.isEmpty()) { + return; + } + + // Deleting a folder never deletes profiles -- everything inside it + // (profiles and subfolders) shifts up to take its place. Count what's + // affected so the confirmation is honest about that, rather than + // reading like the usual destructive "Delete" action. + const QString prefix = normalized + QStringLiteral("/"); + int profileCount = 0; + for (const Profile& profile : m_repository->listProfiles()) { + if (profile.folderPath == normalized || profile.folderPath.startsWith(prefix)) { + ++profileCount; + } + } + int subfolderCount = 0; + for (const QString& folder : m_repository->listFolders()) { + if (folder != normalized && folder.startsWith(prefix)) { + ++subfolderCount; + } + } + + if (profileCount > 0 || subfolderCount > 0) { + QStringList parentParts = splitFolderPath(normalized); + parentParts.removeLast(); + const QString destination = parentParts.isEmpty() + ? QStringLiteral("the top level") + : QStringLiteral("'%1'").arg(parentParts.join(QStringLiteral("/"))); + + QStringList details; + if (profileCount > 0) { + details.push_back(QStringLiteral("%1 profile(s)").arg(profileCount)); + } + if (subfolderCount > 0) { + details.push_back(QStringLiteral("%1 subfolder(s)").arg(subfolderCount)); + } + + const QMessageBox::StandardButton confirm = QMessageBox::question( + this, + QStringLiteral("Delete Folder"), + QStringLiteral("Delete folder '%1'?\n\nNothing will be deleted: %2 currently inside " + "it will move up to %3.") + .arg(normalized, details.join(QStringLiteral(" and ")), destination), + QMessageBox::Yes | QMessageBox::No, + QMessageBox::No); + if (confirm != QMessageBox::Yes) { + return; + } + } + + if (!m_repository->deleteFolder(normalized)) { + QMessageBox::warning(this, + QStringLiteral("Delete Folder"), + QStringLiteral("Failed to delete folder: %1") + .arg(m_repository->lastError().isEmpty() + ? QStringLiteral("unknown error") + : m_repository->lastError())); + return; + } + + loadProfiles(); +} + void ProfilesWindow::persistFolderAssignmentsFromTree() { if (!isFolderViewEnabled() || m_profilesTree == nullptr) { diff --git a/src/profiles_window.h b/src/profiles_window.h index 6727137..fa3ed5d 100644 --- a/src/profiles_window.h +++ b/src/profiles_window.h @@ -65,6 +65,7 @@ private: QString folderPathForItem(const QTreeWidgetItem* item) const; void showTreeContextMenu(const QPoint& pos); void createFolderInContext(const QString& baseFolderPath); + void deleteFolderInContext(const QString& folderPath); void persistFolderAssignmentsFromTree(); void collectProfileAssignments(const QTreeWidgetItem* item, const QString& parentFolderPath, diff --git a/tests/test_profile_repository.cpp b/tests/test_profile_repository.cpp index 4d533ce..78ba71e 100644 --- a/tests/test_profile_repository.cpp +++ b/tests/test_profile_repository.cpp @@ -61,6 +61,12 @@ private slots: void folderCreateAndListRoundTrips(); void folderCreateIgnoresDuplicates(); void folderPathIsNormalized(); + void deleteEmptyFolderRemovesIt(); + void deleteFolderMovesDirectProfilesToParent(); + void deleteRootLevelFolderMovesProfilesToRoot(); + void deleteFolderShiftsSubfoldersAndTheirProfilesUp(); + void deleteFolderRejectsEmptyPath(); + void deleteNonexistentFolderSucceedsAsNoOp(); private: std::unique_ptr m_tempDir; @@ -284,5 +290,86 @@ void TestProfileRepository::folderPathIsNormalized() QCOMPARE(folders[0], QStringLiteral("Work/Servers")); } +void TestProfileRepository::deleteEmptyFolderRemovesIt() +{ + QVERIFY(m_repo->createFolder(QStringLiteral("Empty"))); + QVERIFY(m_repo->deleteFolder(QStringLiteral("Empty"))); + QCOMPARE(m_repo->listFolders().size(), size_t(0)); +} + +void TestProfileRepository::deleteFolderMovesDirectProfilesToParent() +{ + QVERIFY(m_repo->createFolder(QStringLiteral("Work"))); + Profile profile = makeSshProfile(); + profile.folderPath = QStringLiteral("Work"); + const std::optional created = m_repo->createProfile(profile); + QVERIFY(created.has_value()); + + QVERIFY(m_repo->deleteFolder(QStringLiteral("Work"))); + QCOMPARE(m_repo->listFolders().size(), size_t(0)); + + const std::optional fetched = m_repo->getProfile(created->id); + QVERIFY(fetched.has_value()); + // Deleting a folder is never destructive to profiles -- they shift up + // to take its place, here landing at the root since "Work" had no + // parent of its own. + QCOMPARE(fetched->folderPath, QString()); +} + +void TestProfileRepository::deleteRootLevelFolderMovesProfilesToRoot() +{ + Profile profile = makeRdpProfile(); + profile.folderPath = QStringLiteral("Solo"); + const std::optional created = m_repo->createProfile(profile); + QVERIFY(created.has_value()); + + QVERIFY(m_repo->deleteFolder(QStringLiteral("Solo"))); + const std::optional fetched = m_repo->getProfile(created->id); + QVERIFY(fetched.has_value()); + QCOMPARE(fetched->folderPath, QString()); +} + +void TestProfileRepository::deleteFolderShiftsSubfoldersAndTheirProfilesUp() +{ + QVERIFY(m_repo->createFolder(QStringLiteral("Work/Servers"))); + + Profile inTarget = makeSshProfile(QStringLiteral("InWork")); + inTarget.folderPath = QStringLiteral("Work"); + const std::optional createdInTarget = m_repo->createProfile(inTarget); + QVERIFY(createdInTarget.has_value()); + + Profile inSubfolder = makeRdpProfile(QStringLiteral("InServers")); + inSubfolder.folderPath = QStringLiteral("Work/Servers"); + const std::optional createdInSubfolder = m_repo->createProfile(inSubfolder); + QVERIFY(createdInSubfolder.has_value()); + + QVERIFY(m_repo->deleteFolder(QStringLiteral("Work"))); + + // "Work/Servers" shifts up to become root-level "Servers"; the profile + // that was directly in "Work" moves to root; nothing is deleted. + const auto folders = m_repo->listFolders(); + QCOMPARE(folders.size(), size_t(1)); + QCOMPARE(folders[0], QStringLiteral("Servers")); + + const std::optional fetchedInTarget = m_repo->getProfile(createdInTarget->id); + QVERIFY(fetchedInTarget.has_value()); + QCOMPARE(fetchedInTarget->folderPath, QString()); + + const std::optional fetchedInSubfolder = m_repo->getProfile(createdInSubfolder->id); + QVERIFY(fetchedInSubfolder.has_value()); + QCOMPARE(fetchedInSubfolder->folderPath, QStringLiteral("Servers")); +} + +void TestProfileRepository::deleteFolderRejectsEmptyPath() +{ + QVERIFY(!m_repo->deleteFolder(QString())); + QVERIFY(!m_repo->lastError().isEmpty()); +} + +void TestProfileRepository::deleteNonexistentFolderSucceedsAsNoOp() +{ + QVERIFY(m_repo->deleteFolder(QStringLiteral("Never/Created"))); +} + QTEST_GUILESS_MAIN(TestProfileRepository) #include "test_profile_repository.moc"