Sync lock toggle #954
Sync lock toggle #954
Conversation
|
Howdy. Please revert the changes to the version included in the PR and put the changelog entry under the Unreleased Changes header instead. |
| @@ -1,5 +1,8 @@ | |||
| # Rojo Changelog | |||
|
|
|||
| ## [7.4.3] - 31 July, 2024 | |||
There was a problem hiding this comment.
You should put this in the ## Unreleased Changes section, you are not making a new release.
| @@ -1752,7 +1752,7 @@ dependencies = [ | |||
|
|
|||
| [[package]] | |||
| name = "rojo" | |||
| version = "7.4.0" | |||
| version = "7.4.3" | |||
| @@ -1,6 +1,6 @@ | |||
| [package] | |||
| name = "rojo" | |||
| version = "7.4.0" | |||
| version = "7.4.3" | |||
| 7.4.3 |
| SyncLock = e(Setting, { | ||
| id = "syncLock", | ||
| name = "Sync Lock", | ||
| description = "Toggle sync lock", |
There was a problem hiding this comment.
This description is not clear at all, and doesn't explain any more than the title already says.
| description = "Toggle sync lock", | |
| description = "Enables Team Create sync locking to prevent conflicts and overwrites between multiple users.", |
| SyncLock = e(Setting, { | ||
| id = "syncLock", | ||
| name = "Sync Lock", | ||
| description = "Toggle sync lock", |
There was a problem hiding this comment.
Add tag = "caution" and add that tag type to the TAG_TYPES here.
| @@ -299,7 +299,7 @@ function App:getHostAndPort() | |||
| return if #host > 0 then host else Config.defaultHost, if #port > 0 then port else Config.defaultPort | |||
| end | |||
|
|
|||
| function App:isSyncLockAvailable() | |||
| function App:isSyncLockAvailable() | |||
There was a problem hiding this comment.
Remove whitespace. Make sure to always format your changes with StyLua.
| local claimedLock, priorOwner = self:claimSyncLock() | ||
| if not claimedLock then | ||
| local msg = string.format("Could not sync because user '%s' is already syncing", tostring(priorOwner)) | ||
| local syncLockEnabled = Settings:get("syncLock") |
There was a problem hiding this comment.
This is the wrong way to go about this.
Consider the case of two users, Zack and Micah. Zack has locking enabled, Micah has it disabled. Zack is currently syncing and claimed the lock.
With your implementation, Micah can steal the lock from Zack but Zack cannot steal it back. Zack's settings made it clear that he did not want anyone else to sync over him. Micah doesn't mind if people sync over him, yet Zack still cannot because his setting is checking the lock.
The correct way to implement this setting would be that users with locking disabled simply do not claim the lock themselves, thus allowing other users to claim it. If everyone on your team disables it, then the lock is always unclaimed and anyone can sync whenever.
The setting is saying "I don't care about locking", but that should not override users who do care about locking. The lock must always be respected.
There was a problem hiding this comment.
Also, what do we do in the case where someone without locking is syncing, and then someone with locking starts syncing? The new user doesn't want other people syncing. Should the unlocked user be forced to stop syncing?
Your feature needs to consider all the cases, not just your use case of the entire team setting it the same way.
In the company where I work, we sometimes have trouble with the sync lock. We have to ask someone every 5 seconds to unsync just to test something. Everything we write is stored on GitHub, so even if something is overridden, we can just resync and get our files back. It can be really problematic when someone is on break, and we have to wait or kick them from Team Create just to do something. Sticking to an older version of Rojo is not a viable long-term solution, so I implemented a Sync Lock toggle. This allows you to toggle it on and off (it is on by default).