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

Jump to bottom

Properly fetch shares for files with spaces/other special characters in filenames #5884

Closed
wants to merge 4 commits into from
Closed

Properly fetch shares for files with spaces/other special characters in filenames #5884

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f2d5988
Modernise ShareManager::slotSharesFetched
claucambra Jul 12, 2023
cc95a1e
Make an early return when canShare() is false in ShareModel::initShar…
claucambra Jul 12, 2023
48f1dfa
Ensure _sharePath is properly percent encoded by using QUrl::toPercen…
claucambra Jul 12, 2023
7f1216e
Move share path setting in sharemodel into separate private method
claucambra Jul 14, 2023
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: 12 additions & 8 deletions src/gui/filedetails/sharemodel.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,7 +253,7 @@ void ShareModel::updateData()
qCDebug(lcShareModel) << "Updating share model data now.";

const auto relPath = _localPath.mid(_folder->cleanPath().length() + 1);
_sharePath = _folder->remotePathTrailingSlash() + relPath;
updateRelativeSharePath(relPath);

SyncJournalFileRecord fileRecord;
auto resharingAllowed = true; // lets assume the good
Expand Down Expand Up @@ -321,22 +321,26 @@ void ShareModel::updateData()
initShareManager();
}

void ShareModel::updateRelativeSharePath(const QString &relativePath)
{
const auto unsanitisedSharePath = _folder->remotePathTrailingSlash() + relativePath;
const auto sanitisedSharePath = QUrl::toPercentEncoding(unsanitisedSharePath);
_sharePath = QString::fromUtf8(sanitisedSharePath);
}

void ShareModel::initShareManager()
{
if (!_accountState || _accountState->account().isNull()) {
return;
}

bool sharingPossible = true;
if (!canShare()) {
} else if (!canShare()) {
qCWarning(lcSharing) << "The file cannot be shared because it does not have sharing permission.";
sharingPossible = false;
return;
}

if (_manager.isNull() && sharingPossible) {
if (_manager.isNull()) {
_manager.reset(new ShareManager(_accountState->account(), this));
connect(_manager.data(), &ShareManager::sharesFetched, this, &ShareModel::slotSharesFetched);
connect(_manager.data(), &ShareManager::shareCreated, this, [&] {
connect(_manager.data(), &ShareManager::shareCreated, this, [this] {
_manager->fetchShares(_sharePath);
});
connect(_manager.data(), &ShareManager::linkShareCreated, this, &ShareModel::slotAddShare);
Expand Down
1 change: 1 addition & 0 deletions src/gui/filedetails/sharemodel.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,6 +187,7 @@ private slots:
void handleSecureFileDropLinkShare();
void handleLinkShare();
void setupInternalLinkShare();
void updateRelativeSharePath(const QString &relativePath);
void setSharePermissionChangeInProgress(const QString &shareId, const bool isInProgress);
void setHideDownloadEnabledChangeInProgress(const QString &shareId, const bool isInProgress);

Expand Down
30 changes: 13 additions & 17 deletions src/gui/sharemanager.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,11 @@
*/

#include "sharemanager.h"
#include "ocssharejob.h"
#include "account.h"
#include "folderman.h"
#include "accountstate.h"
#include "folderman.h"
#include "ocssharejob.h"
#include "sharee.h"

#include <QUrl>
#include <QJsonDocument>
Expand Down Expand Up @@ -508,29 +509,24 @@ void ShareManager::fetchShares(const QString &path)

void ShareManager::slotSharesFetched(const QJsonDocument &reply)
{
qDebug() << reply;
auto tmpShares = reply.object().value("ocs").toObject().value("data").toArray();
const QString versionString = _account->serverVersion();
qCDebug(lcSharing) << reply;
const auto tmpShares = reply.object().value("ocs").toObject().value("data").toArray();
const auto versionString = _account->serverVersion();
qCDebug(lcSharing) << versionString << "Fetched" << tmpShares.count() << "shares";
claucambra marked this conversation as resolved.
Show resolved Hide resolved

QList<SharePtr> shares;

foreach (const auto &share, tmpShares) {
auto data = share.toObject();

auto shareType = data.value("share_type").toInt();

SharePtr newShare;
for (const auto &share : tmpShares) {
const auto data = share.toObject();
const auto shareType = data.value("share_type").toInt();

if (shareType == Share::TypeLink) {
newShare = parseLinkShare(data);
} else if (Share::isShareTypeUserGroupEmailRoomOrRemote(static_cast <Share::ShareType>(shareType))) {
newShare = parseUserGroupShare(data);
shares.append(parseLinkShare(data));
Copy link
Contributor

Choose a reason for hiding this comment

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

@claucambra I don't see a good reason for this refactoring, besides removing space after static_cast you are basically doing the same but introducing more code for review

Copy link
Contributor

Choose a reason for hiding this comment

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

while fixing the issue @claucambra would it be possible to write a test for the new method you're adding?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The main reason for this change was to eliminate the likelihood of appending a null newShare to shares, and making the code a little shorter -- it doesn't seem like we need the newShare here.

As for the space I think this was auto-done by clang-format but I can re-add

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Looking into the other points now

Copy link
Contributor

Choose a reason for hiding this comment

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

@claucambra No, I am fine with removing the space. I can see your point about newShare being not null, but there is an else statement so it should never be null, that said, you can just add a check if (newShare) prior to adding it if that makes sense

} else if (Share::isShareTypeUserGroupEmailRoomOrRemote(static_cast<Share::ShareType>(shareType))) {
shares.append(parseUserGroupShare(data));
} else {
newShare = parseShare(data);
shares.append(parseShare(data));
}

shares.append(SharePtr(newShare));
}

qCDebug(lcSharing) << "Sending " << shares.count() << "shares";
Expand Down