Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change local collision to follow the ownCloud convention #11117

Merged
merged 1 commit into from
Aug 15, 2023

Conversation

erikjv
Copy link
Collaborator

@erikjv erikjv commented Aug 14, 2023

If the default folder name exists, a different one was offered in the form of "ownCloud2". This is now changed to follow the ownCloud convention, and "ownCloud (2)" is now offered.

@erikjv erikjv requested review from TheOneRing and fmoc August 14, 2023 14:16
@erikjv erikjv self-assigned this Aug 14, 2023
@@ -786,8 +786,8 @@ QString FolderMan::checkPathValidityForNewFolder(const QString &path) const

QString FolderMan::findGoodPathForNewSyncFolder(const QString &basePath, const QString &newFolder) const
{
// reserve 3 characters to allow appending of a number 0-100
const QString normalisedPath = FileSystem::createPortableFileName(basePath, FileSystem::pathEscape(newFolder), 3);
// reserve 3 characters to allow appending of a number 0-100, plus 2 for " (" and 1 for ")"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we could use std::stringview(" (100)").size() to have a compile time placeholder?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

if (!QFileInfo::exists(folder)
&& FolderMan::instance()->checkPathValidityForNewFolder(folder).isEmpty()) {
return canonicalPath(folder);
}
folder = normalisedPath + QString::number(attempt);
folder = normalisedPath + QStringLiteral(" (") + QString::number(attempt) + QStringLiteral(")");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
folder = normalisedPath + QStringLiteral(" (") + QString::number(attempt) + QStringLiteral(")");
folder = normalisedPath + QStringLiteral(" (%1)").arg(QString::number(attempt));

?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@TheOneRing
Copy link
Contributor

Looks like we have unit tests for this :D

If the default folder name exists, a different one was offered in the
form of "ownCloud2". This is now changed to follow the ownCloud
convention, and "ownCloud (2)" is now offered.
@erikjv erikjv force-pushed the work/change-local-collision-name branch from 6de1497 to 1fa982f Compare August 15, 2023 11:45
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@erikjv erikjv requested a review from TheOneRing August 15, 2023 12:43
@TheOneRing TheOneRing merged commit 9e8820f into master Aug 15, 2023
@delete-merged-branch delete-merged-branch bot deleted the work/change-local-collision-name branch August 15, 2023 13:55
@saw-jan
Copy link
Member

saw-jan commented Aug 16, 2023

Tested with ownCloud 5.0.0.11734-daily20230816 9e8820

Screenshot from 2023-08-16 18-39-28

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement local collissions following the ownCloud convention
3 participants