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

with files client automated locking we need a sane default value #164

Closed
wants to merge 1 commit into from

Conversation

mgallien
Copy link
Contributor

@mgallien mgallien commented Sep 6, 2023

would prevent having locks with 0 tiemout because the config value is never changed on server

there will be more locks since desktop files client will automatically lock office files making this more visible

would prevent having locks with 0 tiemout because the config value is
never changed on server

there will be more locks since desktop files client will automatically
lock office files making this more visible

Signed-off-by: Matthieu Gallien <matthieu.gallien@nextcloud.com>
@mgallien mgallien requested a review from juliusknorr September 6, 2023 10:06
@mgallien
Copy link
Contributor Author

mgallien commented Sep 6, 2023

discussed with @allexzander
following tests of the release candidate of desktop client next feature release

@mgallien
Copy link
Contributor Author

mgallien commented Sep 6, 2023

related change on desktop files client nextcloud/desktop#6027

@juliusknorr
Copy link
Member

juliusknorr commented Sep 7, 2023

I tend to agree that having a somewhat reasonable lock timeout is better, especially if automatic locking on clients happens more often now. We used to have this in the past but switched to the infinite one per discussion in #49.

One issue with the default timeout is that if users intentionally lock because they go on a longer flight for example, then the lock would just timeout and no longer prevent other users from editing. However this seems more like and edge case compared to day to day usage and is still safeguarded by versions that one could restore to get changes back. Also admins might still be able to unlock anyways. There should also be a conflict dialog anyways so all predictable behaviour.

Fine with me but involving @AndyScherzinger and @tobiasKaminsky as involved in the initial decision.

Copy link
Member

@juliusknorr juliusknorr left a comment

Choose a reason for hiding this comment

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

Change seems good to me, waiting for more comments before approving regarding changing the default

@mgallien
Copy link
Contributor Author

mgallien commented Sep 8, 2023

I tend to agree that having a somewhat reasonable lock timeout is better, especially if automatic locking on clients happens more often now. We used to have this in the past but switched to the infinite one per discussion in #49.

One issue with the default timeout is that if users intentionally lock because they go on a longer flight for example, then the lock would just timeout and no longer prevent other users from editing. However this seems more like and edge case compared to day to day usage and is still safeguarded by versions that one could restore to get changes back. Also admins might still be able to unlock anyways. There should also be a conflict dialog anyways so all predictable behaviour.

Fine with me but involving @AndyScherzinger and @tobiasKaminsky as involved in the initial decision.

The fact that 0 means infinite was definitely a misunderstanding from us desktop files client team
We always assumed that 0 meant 0
We may also adjust our code a little bit more to have proper handling of the 0 value @tobiasKaminsky ?

@mgallien
Copy link
Contributor Author

another related fix for desktop client
nextcloud/desktop#6059

@AndyScherzinger
Copy link
Member

AndyScherzinger commented Sep 14, 2023

Can you give me some additional context on why you would change this away from infinite? That just works around the problem of improper unlocking or at least that is how it sounds to me but I might be completely wrong here.

Also checking the referenced decision, I expect:

  • clients aren't handling server-unlock via timeouts (yet)
  • admin-unlock is not implemented (yet)

@mgallien
Copy link
Contributor Author

mgallien commented Oct 6, 2023

assuming that the main reason for this PR was a misunderstanding over the use of 0 as an infinite timeout, I would like to close this one in favor of #175 (just improving the API doc around the lock-timeout)

@mgallien
Copy link
Contributor Author

mgallien commented Oct 6, 2023

Can you give me some additional context on why you would change this away from infinite? That just works around the problem of improper unlocking or at least that is how it sounds to me but I might be completely wrong here.

Also checking the referenced decision, I expect:

* clients aren't handling server-unlock via timeouts (yet)

desktop client will schedule a server sync font expired locks to ensure we do not miss files being unlock after the timeout expired
that was not properly handling the 0 value

but as I said, I am all for closing this one in favor of doc improvements

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.

3 participants