Lock file when editing locally

[claucambra]

Closes #5101

Signed-off-by: Claudio Cambra [email protected]

[tobiasKaminsky]

Does this then also cover on all platforms:

• user is in web browser
• user click edit locally, .eg. on a psd
• Desktop receives this via nc:// link
• Desktop locks file for 30min
  --> please check referenced issue for details (also how we already did marketing around it)

Right now it seems that it is the other way around:
If a file is edited locally on VFS, then it is also locked on server?

[mgallien]

Does this then also cover on all platforms:

* user is in web browser

* user click edit locally, .eg. on a psd

* Desktop receives this via nc:// link

* Desktop locks file for 30min
  --> please check referenced issue for details (also how we already did marketing around it)

Right now it seems that it is the other way around: If a file is edited locally on VFS, then it is also locked on server?

the modifications from @claucambra would be to lock always a file before calling the external editor software (they are unrelated to VFS, the comment is a bit misleading there)

what would be missing from the specification (if I understand correctly) would be:

• setting a 30 minutes timeout
• warning the people that they can unlock manually when they are done editing the file

do we want to do more ?

[claucambra]

Does this then also cover on all platforms:

* user is in web browser

* user click edit locally, .eg. on a psd

* Desktop receives this via nc:// link

* Desktop locks file for 30min
  --> please check referenced issue for details (also how we already did marketing around it)

Right now it seems that it is the other way around: If a file is edited locally on VFS, then it is also locked on server?

the modifications from @claucambra would be to lock always a file before calling the external editor software (they are unrelated to VFS, the comment is a bit misleading there)

what would be missing from the specification (if I understand correctly) would be:

• setting a 30 minutes timeout
• warning the people that they can unlock manually when they are done editing the file

do we want to do more ?

I will add a notification for this, seems like we should tell the user this

About the 30 minute timeout, do we want to defy the server default on this?

[claucambra]

@tobiasKaminsky @mgallien added a notification, screenshot in PR description

[allexzander]

@claucambra Besides my other 2 comments, there is one more thing that came to my mind. What happens if the file we are opening is already locked by someone else? The LockJob will then fail for sure if we run it, right? But, this also means we could have checked the lock state of that file before running other steps and could have displayed an error dialog `Could not open a file %fileName% for local editing as it is locked by %lockOwnereDisplayName%!"?

[claucambra]

@claucambra Besides my other 2 comments, there is one more thing that came to my mind. What happens if the file we are opening is already locked by someone else? The LockJob will then fail for sure if we run it, right? But, this also means we could have checked the lock state of that file before running other steps and could have displayed an error dialog `Could not open a file %fileName% for local editing as it is locked by %lockOwnereDisplayName%!"?

We already sync the folder when we start the edit locally process, so we know whether it is locked or not at the point of beginning the edit

We don't try to lock the file if we know at the point of edit that it is locked, and we only run the folder sync again after starting edit if we know we are locking it

[allexzander]

@claucambra Besides my other 2 comments, there is one more thing that came to my mind. What happens if the file we are opening is already locked by someone else? The LockJob will then fail for sure if we run it, right? But, this also means we could have checked the lock state of that file before running other steps and could have displayed an error dialog `Could not open a file %fileName% for local editing as it is locked by %lockOwnereDisplayName%!"?

We already sync the folder when we start the edit locally process, so we know whether it is locked or not at the point of beginning the edit

We don't try to lock the file if we know at the point of edit that it is locked, and we only run the folder sync again after starting edit if we know we are locking it

Ok, I also forgot that we check if the file is locked or not on the server, and, by the way, we have a server bug when we can't see an "Edit locally" menu if we locked a file. But this is for the server team :)

[allexzander]

• I am still facing an assert when I have a file already synced and up-to-date in a folder that points to a remote subfolder (e.g. /docs). In my case, I can see that &SyncEngine::itemCompleted has been triggered, but, the SyncJournal record has not been updated yet, and this is the way you are checking the locked state in the assert. So, I recommend taking the parameter of &SyncEngine::itemCompleted and asserting its lockstate as it has already proper data.
• While I've been working on my recent PR for file local editing, I have noticed also that &SyncEngine::itemCompleted will not get called if the file was up-to-date, so I had to also subscribe to &SyncEngine::itemDiscovered to catch that behavior by checking if item->_instruction == CSYNC_INSTRUCTION_NONE, which means the &SyncEngine::itemCompleted will never get emitted. Kindly make sure this situation is covered. The easiest is to create a second folder connection and just sync one small folder with some files so you can test it then without having to sync the entire cloud.

[claucambra]

• I am still facing an assert when I have a file already synced and up-to-date in a folder that points to a remote subfolder (e.g. /docs). In my case, I can see that &SyncEngine::itemCompleted has been triggered, but, the SyncJournal record has not been updated yet, and this is the way you are checking the locked state in the assert. So, I recommend taking the parameter of &SyncEngine::itemCompleted and asserting its lockstate as it has already proper data.
• While I've been working on my recent PR for file local editing, I have noticed also that &SyncEngine::itemCompleted will not get called if the file was up-to-date, so I had to also subscribe to &SyncEngine::itemDiscovered to catch that behavior by checking if item->_instruction == CSYNC_INSTRUCTION_NONE, which means the &SyncEngine::itemCompleted will never get emitted. Kindly make sure this situation is covered. The easiest is to create a second folder connection and just sync one small folder with some files so you can test it then without having to sync the entire cloud.

Thanks @allexzander I was unaware of this potential case, have addressed it now

[allexzander]

@claucambra I am not convinced this is the right place to call something that starts network requests. Probably, it is a limitation by Qt (IIRC network calls must be made from the main thread). I have been testing this PR and I can see that files are not always locked remotely.

See which signals are triggered when you put breakpoints in the void Account::setLockFileState(const QString &serverRelativePath, SyncJournalDb * const journal, const SyncFileItem::LockStatus lockStatus).
When locking/unlocking files via context menu - those signals will get triggered. When locking the file via Edit Locally, Lock job never finishes. It just starts and the finished signal is never called.
If you move this lockFile() call outside of the QtConcurrent::run, you will have noticed the signals are now getting emitted properly.
I propose you do the locking as a last step in the sequence, and then call
QtConcurrent::run([localFilePathUrl, this]() { QDesktopServices::openUrl(localFilePathUrl); });
after the LockJob has finished (successfully) or if it never ran because the file was already locked.

So we go:

1. Make the local file up-to-date on disk (already implemented)
2. Lock it if needed and wait till LockJob succeeds
3. Call the openUrl function after the LockJob has succeeded or if the file was already locked.

Makes sense?

[nextcloud-desktop-bot]

AppImage file: nextcloud-PR-5226-58f91072a5eaa1ddfef321b8ec8f2b34ef2cb5ce-x86_64.AppImage

To test this change/fix you can simply download above AppImage file and test it.

Please make sure to quit your existing Nextcloud app and backup your data.

[allexzander]

Kindly clean up the history before merge

[sonarqubecloud]

SonarCloud Quality Gate failed.   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell

0.0% Coverage
0.0% Duplication

[claucambra]

/backport to stable-3.6