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.

Sign up for GitHub

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

[stable-3.2] Temporary! Windows. VFS. Block Virtual Files for partition root sync folders. #3359

Merged
merged 1 commit into from
Jun 7, 2021
Merged

[stable-3.2] Temporary! Windows. VFS. Block Virtual Files for partition root sync folders. #3359

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions src/common/utility.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -676,6 +676,26 @@ QByteArray Utility::conflictFileBaseNameFromPattern(const QByteArray &conflictNa
return conflictName.left(tagStart) + conflictName.mid(tagEnd);
}

bool Utility::isPathWindowsDrivePartitionRoot(const QString &path)
{
Q_UNUSED(path)
#ifdef Q_OS_WIN
// should be 2 or 3 characters length
Copy link
Collaborator

Choose a reason for hiding this comment

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

why are you doing a hand made implementation of that instead of using Qt API ?

Copy link
Contributor

@allexzander allexzander Jun 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

why are you doing a hand made implementation of that instead of using Qt API ?

@mgallien

  • there is no known API for achieving this or similar behavior and I am not surprised, as, Qt offers some cross-platform APIs, but, the concept of a root partition is only present in Windows
  • Qt API has proven to bring issues (e.g. QFileInfo leads to a freeze when calling its methods on .lnk files on Windows, while Windows API works with no problems in that case
  • Algorithm is simple enough to be confident of custom code will work as expected
  • Are you aware which Qt API allows achieving this, or at least something close to this that I could use? Could you provide a link?

Copy link
Collaborator

Choose a reason for hiding this comment

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

why are you doing a hand made implementation of that instead of using Qt API ?

@mgallien

* there is no known API for achieving this or similar behavior and I am not surprised, as, Qt offers some cross-platform APIs, but, the concept of a root partition is only present in Windows

* Qt API has proven to bring issues (e.g. QFileInfo leads to a freeze when calling its methods on **.lnk** files on Windows, while Windows API works with no problems in that case

That was specific to the case of VFS, implicit hydration of a file linked by a .lnk file and the current behavior of QFileInfo that consider that .lnk are not real file and jump to the linked file.
So we had this scenario:

  1. We are within a sync folder with VFS CfApi enabled
  2. We build a QFileInfo on a .lnk file
  3. QFileInfo try to access the linked file (let's suppose it is within the same sync folder)
  4. CfApi triggers an implicit hydration of the file
  5. A callback is called in a thread within desktop client
  6. An HydrationJob is created and moved to the main thread
  7. We have a deadlock due to the main thread being blocked within QFileInfo waiting for the job to complete and HydrationJob cannot start because the thread is blocked

In the case of Windows drives, we will have nothing of that. We are safe.

* Algorithm is simple enough to be confident of custom code will work as expected

Yes but we add a few more branches in the code adding complexity that we will have to maintain by ourselves. Including some code that is specific to one platform and cannot really be tested when testing other platforms.

* Are you aware which Qt API allows achieving this, or at least something close to this that I could use? Could you provide a link?

I am talking about that QDir::drives(). Did you try to use it ?

Copy link
Contributor

Choose a reason for hiding this comment

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

@mgallien that's exactly what I've tried. The problem with that method is, it always returns "C" even if I am calling it for a directory that is in "D". How can you explain such behavior? Maybe the documentation is not clear?

Copy link
Contributor

Choose a reason for hiding this comment

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

@mgallien This method returns all drive letters available in the system. What I need instead is, to check if the path I choose is any drive letter without a subfolder. Let's say I choose "d:/subfolder" - then it is not a drive letter (not a drive partition root). Now, if I choose just "d:/" or "d:", or "d:" - then - it is a drive letter (or a drive partition root). More detailed explanation and a screenshot below:

So, let's say, I choose "d:" as my sync root. This is perfectly fine, and, this will work. However, this does not compare exactly with ["C:/", "D:/"], I'd still have to compare if any of ["C:/, "D:/"] starts with my "d:". Then, I could also choose "d:\" as my local sync dir (which is perfectly fine under Windows as it is using a backslash instead of slash by default). In that case, I am having even more problems, I still have to convert backslash to a slash before comparing it against ["C:/", "D:/"].

Now, I am not interested in checking whether the partition I choose is a valid partition on the current system. For example, I don't want to check if "b:/" is incorrect by trying to find it in ["C:/", "D:/"].

Last but not least, imagine, a system has many partitions ["C:/", "D:/", "E:/", "F:/", G:/", ...]

if using this API and a method and looping through a list of partitions trying to find if any of those equal to my sync folder that I choose to be say "S:/", I will have to loop every time, through entire list of partitions.

My goal is to just compare if my path equals to a pattern "AlphaBeticCharacter" + ":" + "slash or backslash or no slash at all".

image

Copy link
Contributor

Choose a reason for hiding this comment

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

@mgallien yes, I've now noticed, dir() is a static method. Then again, there is no way to "just check if a path is a drive partition root" via Qt API. This method does not provide me this information. I still have to compare strings.

if (!(path.size() >= 2 && path.size() <= 3)) {
return false;
}

// must mutch a pattern "[A-Za-z]:"
if (!(path.at(1) == QLatin1Char(':') && path.at(0).isLetter())) {
return false;
}

// final check - last character should be either slash/backslash, or, it should be missing
return path.size() < 3 || path.at(2) == QLatin1Char('/') || path.at(2) == QLatin1Char('\\');
#endif
return false;
}

QString Utility::sanitizeForFileName(const QString &name)
{
const auto invalid = QStringLiteral(R"(/?<>\:*|")");
Expand Down
5 changes: 5 additions & 0 deletions src/common/utility.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -235,6 +235,11 @@ namespace Utility {
*/
OCSYNC_EXPORT QByteArray conflictFileBaseNameFromPattern(const QByteArray &conflictName);

/**
* @brief Check whether the path is a root of a Windows drive partition ([c:/, d:/, e:/, etc.)
*/
OCSYNC_EXPORT bool isPathWindowsDrivePartitionRoot(const QString &path);

#ifdef Q_OS_WIN
OCSYNC_EXPORT bool registryKeyExists(HKEY hRootKey, const QString &subKey);
OCSYNC_EXPORT QVariant registryGetKeyValue(HKEY hRootKey, const QString &subKey, const QString &valueName);
Expand Down
3 changes: 3 additions & 0 deletions src/gui/accountsettings.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -571,6 +571,9 @@ void AccountSettings::slotCustomContextMenuRequested(const QPoint &pos)
const auto mode = bestAvailableVfsMode();
if (mode == Vfs::WindowsCfApi || ConfigFile().showExperimentalOptions()) {
ac = menu->addAction(tr("Enable virtual file support %1 …").arg(mode == Vfs::WindowsCfApi ? QString() : tr("(experimental)")));
// TODO: remove when UX decision is made
ac->setEnabled(!Utility::isPathWindowsDrivePartitionRoot(folder->path()));
//
connect(ac, &QAction::triggered, this, &AccountSettings::slotEnableVfsCurrentFolder);
}
}
Expand Down
15 changes: 15 additions & 0 deletions src/gui/folderwizard.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -536,6 +536,21 @@ void FolderWizardSelectiveSync::initializePage()
initialBlacklist = QStringList("/");
}
_selectiveSync->setFolderInfo(targetPath, alias, initialBlacklist);

if (_virtualFilesCheckBox) {
// TODO: remove when UX decision is made
if (Utility::isPathWindowsDrivePartitionRoot(wizard()->field(QStringLiteral("sourceFolder")).toString())) {
_virtualFilesCheckBox->setChecked(false);
_virtualFilesCheckBox->setEnabled(false);
_virtualFilesCheckBox->setText(tr("Virtual files are not supported for Windows partition roots as local folder. Please choose a valid subfolder under drive letter."));
} else {
_virtualFilesCheckBox->setChecked(bestAvailableVfsMode() == Vfs::WindowsCfApi);
_virtualFilesCheckBox->setEnabled(true);
_virtualFilesCheckBox->setText(tr("Use virtual files instead of downloading content immediately %1").arg(bestAvailableVfsMode() == Vfs::WindowsCfApi ? QString() : tr("(experimental)")));
}
//
}

QWizardPage::initializePage();
}

Expand Down
21 changes: 21 additions & 0 deletions src/gui/wizard/owncloudadvancedsetuppage.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -221,6 +221,24 @@ void OwncloudAdvancedSetupPage::fetchUserData()
_ui.userNameLabel->setText(userName);
}

void OwncloudAdvancedSetupPage::refreshVirtualFilesAvailibility(const QString &path)
{
// TODO: remove when UX decision is made
if (!_ui.rVirtualFileSync->isVisible()) {
return;
}

if (Utility::isPathWindowsDrivePartitionRoot(path)) {
_ui.rVirtualFileSync->setText(tr("Virtual files are not supported for Windows partition roots as local folder. Please choose a valid subfolder under drive letter."));
setRadioChecked(_ui.rSyncEverything);
_ui.rVirtualFileSync->setEnabled(false);
} else {
_ui.rVirtualFileSync->setText(tr("Use &virtual files instead of downloading content immediately %1").arg(bestAvailableVfsMode() == Vfs::WindowsCfApi ? QString() : tr("(experimental)")));
_ui.rVirtualFileSync->setEnabled(true);
}
//
}

void OwncloudAdvancedSetupPage::setServerAddressLabelUrl(const QUrl &url)
{
if (!url.isValid()) {
Expand Down Expand Up @@ -411,6 +429,9 @@ void OwncloudAdvancedSetupPage::slotSelectFolder()
{
QString dir = QFileDialog::getExistingDirectory(nullptr, tr("Local Sync Folder"), QDir::homePath());
if (!dir.isEmpty()) {
// TODO: remove when UX decision is made
refreshVirtualFilesAvailibility(dir);

setLocalFolderPushButtonPath(dir);
wizard()->setProperty("localFolder", dir);
updateStatus();
Expand Down
3 changes: 3 additions & 0 deletions src/gui/wizard/owncloudadvancedsetuppage.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,9 @@ private slots:
void fetchUserAvatar();
void fetchUserData();

// TODO: remove when UX decision is made
void refreshVirtualFilesAvailibility(const QString &path);

Ui_OwncloudAdvancedSetupPage _ui;
bool _checking = false;
bool _created = false;
Expand Down