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

Create oc_weld #944

Closed
wants to merge 2 commits into from
Closed

Conversation

Yosty7B3
Copy link
Collaborator

@Yosty7B3 Yosty7B3 commented Apr 3, 2023

Replaces the functionality of Weld but in it's on script now (was spread between oc_core and oc_settings).

Using LinksetData instead of prim description for settings, which also allows the Weld button from being dynamically added and removed.

Most weld dependencies in oc_settings should now be redundant and could be removed. Already removed (commented out) weld from oc_core in #943

Replaces the functionality of Weld but in it's on script now (was spread between oc_core and oc_settings).

Using LinksetData instead of prim description for settings, which also allows the Weld button from being dynamically added and removed. 
Added global_hideweld flag to hides the Weld button (1=hide).

Most weld dependencies in oc_settings should now be redundant and could be removed. 
Already removed (commented out) weld from oc_core in OpenCollarTeam#943
@Medea-Destiny Medea-Destiny added Do Not Merge (yet) One reason or another for not merging OC9.x Target v9 with LSD revisions labels Apr 8, 2023
@Medea-Destiny
Copy link
Collaborator

Hi Yosty, great stuff, separating out weld into its own script is much needed. We're likely to need some minor changes when we've finalized the changes to settings management and dynamic menus.

As part of the process of moving over to LSD for this (and other settings) we have to take some care with dependencies and redundant code, so this will also rely on changes to OC_settings as you say. We're also going to have to think about the update process here -- I'd anticipate the updater will need to ensure the weld script is added if there's a weld present.

On specifics of implementation: I'd suggest we want this script in the Apps menu, and maintain the unweld code within this script. I think we could have a button in the menu that explains the unweld process too -- "how do I unweld" is one of the most commonly asked support questions. I'm not sure we actually need to use the linkset_data event here at all -- it's something we should avoid if we can for the sake of script efficiency, as as our LSD is easily read globally I'd be inclined to manage the LOCK button behaviour in main menu by checking the intern_weld value locally in the oc_core script.

@Yosty7B3
Copy link
Collaborator Author

Yosty7B3 commented Apr 9, 2023

The LOCK button behavior is still controlled by the core script. and reading the weld flag like you suggest. It's one of the few pieces of weld code I left in core. But thelinkset_data event is for adding/removing the Weld button. I wouldn't know how else to do it tbh.

@Yosty7B3
Copy link
Collaborator Author

Yosty7B3 commented May 17, 2023

Come to think of it, if we put Weld in it's own Apps menu, and no longer show the weld button in the main menu, then there no longer would be a need for the linkset_data event.
Catching the event was specifically intended to update the main menu after a respring so that it reflected changes .

* no longer adding/removing Weld button in main menu but instaed use it's own Weld menu in Apps
* don't have to rely on linkset_data  anylonger so removed that part
* added warning in Weld menu prompt. And a button that links to the Weld help page: https://opencollar.cc/docs/Weld
@Medea-Destiny Medea-Destiny self-assigned this Nov 15, 2023
@Medea-Destiny Medea-Destiny added 8.3 Target version 8.,3 and removed OC9.x Target v9 with LSD revisions labels Nov 15, 2023
@Medea-Destiny
Copy link
Collaborator

So this is an important and valuable project, and I think we should target it for 8.3. Weld in its current form causes an awful lot of the support load for OpenCollar.

At the moment this app brings the huge advantage that moving it to an app means we're taking the button a little further away from unwise temptation (we've all seen the "I accidentally welded my collar, I was curious to see what it does and now I can't unlock" complaints), and means we can clear some space in the core and api, which is certainly useful.

Once we've taken the step of providing an app it makes sense to me that the weld app also handles unwelding, and we do without the necessity of an external welder altogether. This can work in exactly the same was as the weld function: the unweld has to be initiated with someone with CMD_OWNER auth, and then the wearer gets a consent popup. This entirely removes the complexity of an external welder.

I have prepared a modification of @Yosty7B3 's OC_weld to do this and remove llLinkSetData dependencies for full compatibility with the 8.3 API which can be found here (as yet untested) https://github.com/OpenCollarTeam/OpenCollar/blob/Medea_destiny_weld_patch/src/Apps/oc_weld . Further work that needs to be done here for the 8.3 inclusion is to remove the weld bits that are no longer necessary in oc_core/api. @Yosty7B3 I assume you've done some work on that already?

I have two extra considerations we could use some input into:

  1. Installer. It might make sense to modify the installer so that the oc_weld app is automatically installed into collars that are welded. That means that welded collars can be unwelded by the normal process without modification. Alternatively, we can simply instruct people who wish to unweld to run the updater and make sure they accept the optional oc_weld app. On the downside this is an extra step, on the upside people might actually prefer not having the unweld option right there.
  2. We might decide that having an external unweld object is actually a good thing, because the requirement of using an external tool makes the weld process "feel" more real. If so we could simply scan for any object called "Open Collar Unwelder" within 5 meters and not proceed to the unweld consent stage unless it's present. This way we don't even have to bother with a scripted unwelder.

@Medea-Destiny Medea-Destiny added the Discussion wanted Request for community feedback label Nov 15, 2023
@Yosty7B3
Copy link
Collaborator Author

@Yosty7B3 I assume you've done some work on that already?

Yes, I did for the LSD version. It only needed an if (llLinksetDataRead(... to enable the 'welded by' message in the main menu. For 8.3 that would need to keep more. But all in all it should be pretty simple to make it work with your adjustments.
I assume we do still want that to show in the main menu when someone is welded.

@Medea-Destiny
Copy link
Collaborator

@Yosty7B3 Yes, so long as the plugin is there, it should show in the menu. People can always remove the plugin after welding, and add back in if they need to weld, if they wish!

@Medea-Destiny
Copy link
Collaborator

Discussion on non-technical aspects of this (decisions on how it all works for the user etc) here: #1009

@Medea-Destiny Medea-Destiny added 8.4 and removed 8.3 Target version 8.,3 labels Aug 1, 2024
@SilkieSabra SilkieSabra deleted the branch OpenCollarTeam:8.3_Features-branch October 5, 2024 16:03
@SilkieSabra SilkieSabra closed this Oct 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
8.4 Discussion wanted Request for community feedback Do Not Merge (yet) One reason or another for not merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants