Internal
Public Access
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 <noreply@anthropic.com>
This commit is contained in:
@@ -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<std::pair<qint64, QString>> 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<Profile> ProfileRepository::listProfiles(const QString& searchQuery,
|
||||
ProfileSortOrder sortOrder) const
|
||||
{
|
||||
|
||||
@@ -47,6 +47,7 @@ public:
|
||||
ProfileSortOrder sortOrder = ProfileSortOrder::NameAsc) const;
|
||||
std::vector<QString> listFolders() const;
|
||||
bool createFolder(const QString& folderPath) const;
|
||||
bool deleteFolder(const QString& folderPath) const;
|
||||
std::optional<Profile> getProfile(qint64 id) const;
|
||||
std::optional<Profile> createProfile(const Profile& profile) const;
|
||||
bool updateProfile(const Profile& profile) const;
|
||||
|
||||
@@ -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) {
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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<QTemporaryDir> 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<Profile> 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<Profile> 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<Profile> created = m_repo->createProfile(profile);
|
||||
QVERIFY(created.has_value());
|
||||
|
||||
QVERIFY(m_repo->deleteFolder(QStringLiteral("Solo")));
|
||||
const std::optional<Profile> 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<Profile> createdInTarget = m_repo->createProfile(inTarget);
|
||||
QVERIFY(createdInTarget.has_value());
|
||||
|
||||
Profile inSubfolder = makeRdpProfile(QStringLiteral("InServers"));
|
||||
inSubfolder.folderPath = QStringLiteral("Work/Servers");
|
||||
const std::optional<Profile> 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<Profile> fetchedInTarget = m_repo->getProfile(createdInTarget->id);
|
||||
QVERIFY(fetchedInTarget.has_value());
|
||||
QCOMPARE(fetchedInTarget->folderPath, QString());
|
||||
|
||||
const std::optional<Profile> 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"
|
||||
|
||||
Reference in New Issue
Block a user