fix(Windows): set Nextcloud folder attribute as read only (rather than system) #8810
Conversation
nofishonfriday
requested review from
Aiiaiiio,
camilasan,
i2h3,
mgallien and
nilsding
as code owners
nofishonfriday
force-pushed
the
fix-win-system-readonly-folder
branch
from
5d7ee72 to
ad7dae4
Compare
…n system) closes nextcloud#5265 Signed-off-by: nofish <[email protected]>
nofishonfriday
force-pushed
the
fix-win-system-readonly-folder
branch
from
ad7dae4 to
ea71762
Compare
mgallien
left a comment
mgallien
left a comment
There was a problem hiding this comment.
I am really not convinced we need to change it because some closed source software as issues with the behavior of the desktop client
as an user I would be heavily confused (and yes I tested setting a custom icon) if I would see the folder being read-only when I want write access to my Nextcloud files
the big benefit of using the system flag is that we get a less confusing state
I also fail to see why we need to change that now while the current code was doing fine for years
| // https://msdn.microsoft.com/en-us/library/windows/desktop/cc144102 | ||
| DWORD folderAttrs = GetFileAttributesW((wchar_t *)folder.utf16()); | ||
| SetFileAttributesW((wchar_t *)folder.utf16(), folderAttrs | FILE_ATTRIBUTE_SYSTEM); | ||
| SetFileAttributesW((wchar_t *)folder.utf16(), folderAttrs | FILE_ATTRIBUTE_READONLY); |
There was a problem hiding this comment.
what if the folder was already setup to use system flag ?
| const auto folderAttrs = GetFileAttributesW(folder.toStdWString().c_str()); | ||
| if (!SetFileAttributesW(folder.toStdWString().c_str(), folderAttrs & ~FILE_ATTRIBUTE_SYSTEM)) { | ||
| qCWarning(lcUtility) << "Remove system file attribute failed for:" << folder; | ||
| if (!SetFileAttributesW(folder.toStdWString().c_str(), folderAttrs & ~FILE_ATTRIBUTE_READONLY)) { |
There was a problem hiding this comment.
what would happen if this folder was using the system flags ?
we would need to care about existing users
|
Thanks for the review and thoughts. I'd leave the implementation related issues aside for now until it's decided if this change will be accepted or not.
My motivation for this PR was that I (as Nextcloud user) was confused with the current implementation too. So what I want to say is, imo the current implementation is not unproblematic either. Anyway, as new contributer here, I'm fine with however the regular contributers (you and others) decide, so no problem if this PR gets closed unmerged. |
|
Hello there, We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process. Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6 Thank you for contributing to Nextcloud and we hope to hear from you soon! (If you believe you should not receive this message, you can add yourself to the blocklist.) |
2 participants
As per #5265 (comment)
closes #5265