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

Add CBA setting for noRadio #5571

Merged
merged 3 commits into from
Nov 12, 2017
Merged

Add CBA setting for noRadio #5571

merged 3 commits into from
Nov 12, 2017

Conversation

PabstMirror
Copy link
Contributor

Close #2346

@PabstMirror PabstMirror added the kind/feature Release Notes: **ADDED:** label Sep 29, 2017
@PabstMirror PabstMirror added this to the 3.11.0 milestone Sep 29, 2017
jonpas
jonpas previously approved these changes Sep 29, 2017
@AACO
Copy link
Contributor

AACO commented Sep 29, 2017

Just curious, why is the DC event handler added, even if the module is disabled? It's not going to fire a lot, but I'm always a fan of not adding handlers if the component is disabled.

@jonpas
Copy link
Member

jonpas commented Sep 29, 2017

To support changing it on the fly. More and more things that don't it like that will likely be converted as we start manually moving components to CBA Settings. It's not any overhead really on an event handler like that. For things that execute more often it might be wiser to add/remove handler as required.

@AACO
Copy link
Contributor

AACO commented Sep 29, 2017

@jonpas the main player changed EH won't get added if it's changed on the fly. I guess I don't see how handling these two items separately will add anything (besides confusion ;) )

I'm not sure how ACE will convert to CBA settings, but (as you know) CBA settings can have a chunk of code executed on change, which should allow adding/removing these kinds of event handlers without too much pain (only problem will probably come from handling class event handlers, and the messiness of having to track the EHIDs through GVARs).

@jonpas
Copy link
Member

jonpas commented Sep 29, 2017

Ah I didn't notice that, good point!

And yes, that's why I said it will probably only be used for performance heavy EHs. We'll see how we'll pursue that, for starters ACE Settings will be converted on the fly to CBA Settings, there will definitely be a post going live with the update about that.

@jonpas jonpas dismissed their stale review September 29, 2017 18:22

Point was brought up

@commy2
Copy link
Contributor

commy2 commented Sep 29, 2017

I'd like to make this the first CBA setting and have it support changing it on the fly :)

@commy2 commy2 removed the status/WIP label Sep 29, 2017
@commy2 commy2 changed the title Add ace_setting for noRadio Add CBA setting for noRadio Sep 29, 2017
@PabstMirror PabstMirror modified the milestones: 3.11.0, CBA Settings Oct 11, 2017
@PabstMirror PabstMirror modified the milestones: CBA Settings, 3.12.0 Nov 12, 2017
@PabstMirror
Copy link
Contributor Author

I think we can pick this up for 3.12
It's new setting so there should be no bwc to worry about

@PabstMirror PabstMirror merged commit 3c8d5d6 into master Nov 12, 2017
@PabstMirror PabstMirror deleted the noRadioSetting branch November 12, 2017 17:34
BaerMitUmlaut pushed a commit that referenced this pull request Aug 5, 2019
* Add ace_setting for noRadio

* Delay adding server EH

* make noradio a cba setting
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Release Notes: **ADDED:**
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants