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

Sync lock toggle #954

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Sync lock toggle #954

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
bc60ca9
SyncLock
NcxyZero Jul 30, 2024
8263546
SyncLock
NcxyZero Jul 30, 2024
4f7d3a2
modified: CHANGELOG.md
NcxyZero Jul 30, 2024
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
3 changes: 3 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
# Rojo Changelog

## [7.4.3] - 31 July, 2024
Copy link
Member

Choose a reason for hiding this comment

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

You should put this in the ## Unreleased Changes section, you are not making a new release.

* Added Sync lock toggle

## Unreleased Changes
* Projects may now manually link `Ref` properties together using `Attributes`. ([#843])
This has two parts: using `id` or `$id` in JSON files or a `Rojo_Target` attribute, an Instance
Expand Down
2 changes: 1 addition & 1 deletion Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
[package]
name = "rojo"
version = "7.4.0"
version = "7.4.3"
Copy link
Member

Choose a reason for hiding this comment

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

Undo version change.

rust-version = "1.70.0"
authors = ["Lucien Greathouse <[email protected]>"]
description = "Enables professional-grade development tools for Roblox developers"
Expand Down
2 changes: 1 addition & 1 deletion plugin/Version.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
7.4.0
7.4.3
Copy link
Member

Choose a reason for hiding this comment

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

Undo version change.

8 changes: 8 additions & 0 deletions plugin/src/App/StatusPages/Settings/init.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,14 @@ function SettingsPage:render()
layoutOrder = layoutIncrement(),
}),

SyncLock = e(Setting, {
id = "syncLock",
name = "Sync Lock",
description = "Toggle sync lock",
Copy link
Member

Choose a reason for hiding this comment

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

This description is not clear at all, and doesn't explain any more than the title already says.

Suggested change
description = "Toggle sync lock",
description = "Enables Team Create sync locking to prevent conflicts and overwrites between multiple users.",

Copy link
Member

Choose a reason for hiding this comment

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

Add tag = "caution" and add that tag type to the TAG_TYPES here.

transparency = self.props.transparency,
layoutOrder = layoutIncrement(),
}),

SyncReminder = e(Setting, {
id = "syncReminder",
name = "Sync Reminder",
Expand Down
29 changes: 16 additions & 13 deletions plugin/src/App/init.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Copy link
Member

Choose a reason for hiding this comment

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

Remove whitespace. Make sure to always format your changes with StyLua.

if #Players:GetPlayers() == 0 then
-- Team Create is not active, so no one can be holding the lock
return true
Expand Down Expand Up @@ -408,21 +408,24 @@ function App:useRunningConnectionInfo()
end

function App:startSession()
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")
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

@boatbomber boatbomber Aug 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.


Log.warn(msg)
self:addNotification(msg, 10)
self:setState({
appStatus = AppStatus.Error,
errorMessage = msg,
toolbarIcon = Assets.Images.PluginButtonWarning,
})
if syncLockEnabled then
local claimedLock, priorOwner = self:claimSyncLock()
if not claimedLock then
local msg = string.format("Could not sync because user '%s' is already syncing", tostring(priorOwner))

return
end
Log.warn(msg)
self:addNotification(msg, 10)
self:setState({
appStatus = AppStatus.Error,
errorMessage = msg,
toolbarIcon = Assets.Images.PluginButtonWarning,
})

return
end
end
local host, port = self:getHostAndPort()

local baseUrl = if string.find(host, "^https?://")
Expand Down
1 change: 1 addition & 0 deletions plugin/src/Settings.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ local defaultSettings = {
openScriptsExternally = false,
twoWaySync = false,
showNotifications = true,
syncLock = true,
syncReminder = true,
autoConnectPlaytestServer = false,
confirmationBehavior = "Initial",
Expand Down