Fix "Always" confirmation behavior setting which currently works like "Initial"#1216
Fix "Always" confirmation behavior setting which currently works like "Initial"#1216warsam wants to merge 3 commits intorojo-rbx:masterfrom
Conversation
…ngoing sync patches - Add "Always" option to confirmationBehavior setting - Route WebSocket patches through the confirmation callback - Fix UI not transitioning back to Connected after confirming ongoing patches - Fix DomLabel Flipper motor cleanup on unmount to prevent errors
|
The purpose of this setting is mostly to prevent users from unknowingly syncing the wrong project on initial sync. If I'm understanding correctly, the new "Always" option in this PR requires the user to accept any and all incoming changes from the Rojo server, even those that occur after the initial sync. I don't think I've ever seen anyone request this, and I'm unsure about its utility. Can you elaborate further on the purpose? |
The default option in that setting is "Initial" which works as you are describing. The "Always" option already existed in the confirmation behavior setting and I noticed that it works like "Initial" rather than actually prompting confirmation always as described. This PR just fixes it so it actually works. Previously, selecting "Always" had no effect on ongoing sync patches; they would still be applied automatically without confirmation. This PR routes WebSocket patches through the confirmation callback so the setting is respected as users would expect. If the "Always" option shouldn't apply to ongoing patches (only initial sync), that's a separate design decision, but currently the UI presents it as an option without it functioning. |
Corrected the description of the 'Always' confirmation behavior setting to clarify its functionality.
plugin/src/App/init.lua
Outdated
| if serverInfo.expectedPlaceIds then | ||
| local isListed = table.find(serverInfo.expectedPlaceIds, game.PlaceId) ~= nil | ||
| if isListed then | ||
| if confirmationBehavior ~= "Always" then |
There was a problem hiding this comment.
This is to make this confirmation behavior setting explicit rather than implicitly falling through this block so there's no actual behavior change here.
plugin/src/Settings.lua
Outdated
| checkForPrereleases = false, | ||
| autoConnectPlaytestServer = false, | ||
| confirmationBehavior = "Initial" :: "Never" | "Initial" | "Large Changes" | "Unlisted PlaceId", | ||
| confirmationBehavior = "Initial" :: "Never" | "Initial" | "Always" | "Large Changes" | "Unlisted PlaceId", |
There was a problem hiding this comment.
This is adding handling for the "Always" setting which already exists in UI drop down in the plugin:
https://github.com/warsam/rojo/blob/master/plugin/src/Settings.lua#L23
|
My understanding is that the confirmation behavior setting is only meant to guard the initial sync. I don't believe that Rojo users want manually confirm every change they make, and if we shipped this change, I'd worry that some users would forget/not realize they turned it on and be confused about the behavior. I agree that the current behavior of the "Always" option doesn't really make sense, but I don't think this PR captures the original intent, and does significantly change the mental model of live sync: Rojo has never made users confirm every change. Maybe @boatbomber can weigh in here since he is the original implementer. |
I understand the concern, but this doesn't change the default behavior as "Initial" remains the default. Users would have to deliberately go into settings and select "Always" to enable this behavior. I'm a user who wanted this functionality, found the setting, switched to it, and discovered it didn't work. The option exists in the UI with a clear label. If it's not supposed to do what the label says, it's misleading to have it there. If the concern is users accidentally enabling it, that's a UX/discoverability issue with having the option at all, not with making it function as labeled. |
|
The Initial setting has some cases where it doesn't prompt, so Always was added as a stricter Initial. I've never head of someone wanting Rojo to halt and popup after every single change. Why would you need that? |
To clarify - it doesn't pop up for every individual change. Changes are batched and the confirmation dialog updates in real-time. You approve once when you're ready.
What are those cases? If they both only apply to initial sync, why have two options instead of just fixing "Initial"? |
It's actually the other way around, Initial was added because Always was annoying people. Initial only prompts you on the first time you sync a particular project per Studio session. |
|
Thanks for the context @boatbomber! Looking at #773 and #774, the goal was making confirmation less annoying by default, not removing it. I see that Dekkonot noted it's worth having "for new games" and you said disabling it entirely "is just avoiding addressing the root issue." That's why "Always" was kept as an option. I can confirm that by my own desire for it as a new rojo user. This PR just makes that option work as labeled when someone deliberately enables it. Right now selecting "Always" silently behaves like "Initial", which I found confusing. It also addresses the "hot workflows of constant connect/disconnect" you mentioned in #774. Patches now merge while the confirmation dialog is open, so you see the final merged state when you hit "accept". So you now only need to accept once at the end of multiple changes. The dialog auto-dismisses if all changes revert (as I didn't think it was useful to accept a change that does nothing), so it shouldn't be as noisy as the old "Always" behavior was. If "Always" shouldn't exist at all, I think the right fix is removing it from the UI rather than keeping a broken option. Happy to do that instead if preferred? |
As a point of clarity, the setting isn't broken, it's just misleading. It's behaving exactly as it was implemented to do, which is prompting the user for the initial sync every time. In hindsight though, we probably could have named the setting something better. I don't really see the use case of a setting for prompting on every sync. I'd much rather we just rename the setting to make it more obvious what it does. |
|
I agree, it's behaving exactly as intended. We just need a better name for it. If we changed its behavior that would be broken since people who had Always selected would suddenly have confirmation popups unexpectedly. |
|
Gotcha, makes sense. I'll leave "Always" as-is. I've reworked this to add "Every Change" as a new option instead. No existing behavior is changed. |
4 participants
Summary
Implements the "Always" confirmation behavior setting, which prompts for confirmation on every sync patch (not just the initial sync). Changes that arrive during confirmation are merged into the pending patch and the UI updates in real-time.
Implementation
Test Plan