Remove offensive language (Pt. 3) #11960

Holzhaus · 2023-09-11T14:16:31Z

Continuation of #11959 which is part of #11931.

This renames the [Master] control group to [Main] and [MasterOutput] to [MainOutput]. Pre-commit failure is expected unless we reformat and fix all controller scripts.

Swiftb0y

small nitpick, we renamed EngineMaster to EngineMixer, but [Master] to [Main], wouldn't [Mixer] have been better?

Swiftb0y · 2023-09-11T16:31:39Z

src/control/control.cpp

+        s_qCOAliasHash.insert(originalKey, aliasKey);
+        s_qCOHash.insert(aliasKey, pControl);


could this get expensive if the group is large? would it be better to do the aliasing for the group on lookup instead?

That way, as part of the lookup we could also print a warning if the alias was used instead of the non-aliased group.

could this get expensive if the group is large?

You mean memory-wise? It adds a regular alias for every member in a group, but it shouldn't be that much of an issue. In terms of runtime performance, it shouldn't be an issue because lookups are only performed when a new CO is created and it's O(1).

would it be better to do the aliasing for the group on lookup instead? That way, as part of the lookup we could also print a warning if the alias was used instead of the non-aliased group.

True, this could be worth considering. I'll have a look.

You mean memory-wise? It adds a regular alias for every member in a group, but it shouldn't be that much of an issue.

Right, if the group is large, this could bloat the table quite a bit. Yes, lookups are $O(1)$ but creating a group alias and creating COs becomes $O(n)$ (where $n$ is size of the group in the former case and the number of alias groups in the latter). The increased table size would cause the usual slowdowns associated with high memory consumption (data cache misses, more yields to the OS for more memory, more table relocations since it grows faster / more often, etc).

Doing the aliasing lookup would avoid that and only be worse performing on the unhappy path (probing via alias group, if CO exists return (happy path) otherwise lookup primary group name, probe again using primary group name, print warning, etc).

Swiftb0y · 2023-09-11T16:33:05Z

src/control/control.cpp

+            s_qCOHashMutex.unlock();
+            insertAlias(aliasKey, key);
+        } else {
+            s_qCOHashMutex.unlock();
+        }


I'd prefer if we used a locker and just manually unlocked just before.

Holzhaus · 2023-09-11T19:46:54Z

small nitpick, we renamed EngineMaster to EngineMixer, but [Master] to [Main], wouldn't [Mixer] have been better?

I first though the same, but on the other hand the group is also used for the effects framework. Something like [OutputEffectRack1_[Mixer]_Effect1] does not really make sense.

Maybe we can use this as an oppurtunity to split the [Master] group into multiple subgroups with better names? For example, [Master],skin_settings could become [Skin],settings?

Swiftb0y · 2023-09-11T19:53:05Z

Maybe we can use this as an oppurtunity to split the [Master] group into multiple subgroups with better names? For example, [Master],skin_settings could become [Skin],settings?

Yes, I think that would be a good idea, but I'm a bit worried about the complexity (diff size) this entails.

Holzhaus · 2023-09-11T21:16:52Z

Here's a proposal for the rename:

Old Name	New Name	Add Alias?
`[EffectRack1_EffectUnit*],group_[MasterOutput]_enable`	`[EffectRack1_EffectUnit*],group_[MainOutput]_enable`	✔️
`[Master],audio_latency_overload`	`[App],audio_latency_overload`	✔️
`[Master],audio_latency_overload_count`	`[App],audio_latency_overload_count`	✔️
`[Master],audio_latency_usage`	`[App],audio_latency_usage`	✔️
`[Master],balance`	`[MainOutput],balance`	✔️
`[Master],boothDelay`	`[Mixer],booth_delay`	❌
`[Master],booth_enabled`	`[Mixer],booth_enabled`	✔️
`[Master],booth_gain`	`[Mixer],booth_gain`	✔️
`[Master],crossfader`	`[Mixer],crossfader`	✔️
`[Master],delay`	`[Mixer],delay`	❌
`[Master],duckStrength`	`[Mixer],duck_strength`	✔️
`[Master],enabled`	`[MainOutput],enabled`	✔️
`[Master],gain`	`[MainOutput],gain`	✔️
`[Master],guiTick50ms`	`[App],gui_tick_50ms`	❌
`[Master],guiTickTime`	`[App],gui_tick_time`	❌
`[Master],headDelay`	`[Mixer],head_delay`	❌
`[Master],headEnabled`	`[Mixer],head_enabled`	✔️
`[Master],headGain`	`[Mixer],head_gain`	✔️
`[Master],headMix`	`[Mixer],head_mix`	✔️
`[Master],headSplit`	`[Mixer],head_split`	✔️
`[Master],indicator_250millis`	`[App],indicator_250millis`	✔️
`[Master],indicator_500millis`	`[App],indicator_500millis`	✔️
`[Master],keylock_engine`	`[App],keylock_engine`	❌
`[Master],latency`	`[App],latency`	✔️
`[Master],maximize_library`	`[Skin],show_maximized_library`	✔️
`[Master],microphoneLatencyCompensation`	`[App],microphone_latency_compensation`	❌
`[Master],mono_mixdown`	`[Mixer],mono_mixdown`	❌
`[Master],num_auxiliaries`	`[App],num_auxiliaries`	❌ (?)
`[Master],num_decks`	`[App],num_decks`	✔️
`[Master],num_effectsavailable`	`[App],num_effectsavailable`	✔️
`[Master],num_microphones`	`[App],num_microphones`	❌ (?)
`[Master],num_mics_configured`	`[App],num_mics_configured`	❌ (?)
`[Master],num_preview_decks`	`[App],num_preview_decks`	✔️
`[Master],num_samplers`	`[App],num_samplers`	✔️
`[Master],PeakIndicator`	`[Mixer],peak_indicator`	✔️
`[Master],PeakIndicatorL`	`[Mixer],peak_indicator_left`	✔️
`[Master],PeakIndicatorR`	`[Mixer],peak_indicator_right`	✔️
`[Master],samplerate`	`[App],samplerate`	✔️
`[Master],show_mixer`	`[Skin],show_mixer`	❌
`[Master],skin_settings`	`[Skin],show_settings`	❌
`[Master],talkoverDucking`	`[Mixer],talkover_ducking`	✔️
`[Master],talkover_mix`	`[Mixer],talkover_mix`	❌
`[Master],VuMeter`	`[Mixer],vu_meter`	✔️
`[Master],VuMeterL`	`[Mixer],vu_meter_left`	✔️
`[Master],VuMeterR`	`[Mixer],vu_meter_right`	✔️
`[OutputEffectRack_[Master]],*`	`[OutputEffectRack_[MainOutput]],*`	❌
`[OutputEffectRack_[Master]_Effect1],*`	`[OutputEffectRack_[MainOutput]_Effect1],*`	❌

Maybe we can use this as an oppurtunity to split the [Master] group into multiple subgroups with better names? For example, [Master],skin_settings could become [Skin],settings?

Yes, I think that would be a good idea, but I'm a bit worried about the complexity (diff size) this entails.

Well, we won't know until we try 😉 If we add the simple group alias now and we want to rename the COs later on, we will have to maintain two aliases, so let's check if we can do it now.

Swiftb0y · 2023-09-11T21:48:12Z

I agree with most of these, though I feel like [MainOutput] needs some discussion, what is that supposed to represent? It's only occurring once in mixxx as far as I can tell and is only used internally. So either we define more clearly what its supposed to mean or we remove it.

daschuer · 2023-09-11T21:48:25Z

For me it is clear what App means. For non programmers this term is strictly connected to mobile phone Apps.
On the other hand normal users are not bothered with it.

daschuer · 2023-09-11T21:50:08Z

Are the remaining [Masters] intentional?

Swiftb0y · 2023-09-11T21:51:45Z

While I personally don't like that Applications on desktop are being called "Apps" (especially because its a term that has been coined by apple for their mobile devices), I think I've lost the fight since even Microsoft calls them Apps now (see MS Store). I think regular end-users are used to calling desktop applications Apps as well by now.

Swiftb0y · 2023-09-11T21:51:52Z

Are the remaining [Masters] intentional?

Which ones?

JoergAtGithub

If we change all official mappings to the new COs, we will not notice if we break our backward compatibility in a future code change by accident.
Therefore, please add a testcase, that ensures the backward compatibility of the legacy CO names.

daschuer · 2023-09-11T21:52:08Z

[App] -> [Mixxx]? or [Info]?

daschuer · 2023-09-11T21:52:55Z

[Master],delay [Master],enabled

ronso0 · 2023-09-11T21:53:42Z

👍 for [App] -> [Mixxx]

Swiftb0y · 2023-09-11T21:55:48Z

[App] -> [Mixxx]? or [Info]?

[Info],gui_tick_50ms doesn't work. [Mixxx] does not really say much. With [App] it's at least implied that it's about the currently running instance...

JoergAtGithub · 2023-09-13T18:28:17Z

Please think about users without programming skills, which have downloaded, or will download mappings from our mappings forum. Now these mappings are useable - even if the mapping is unmaintained. We will loose these users for Mixxx, if we can't ensure backwards compatibility.

ronso0 · 2023-09-13T18:33:15Z

Please keep in mind that the documentation always lags behind the implementation, for example the newly added effect controls. If someone would fork an official beta skin that may stop working.
mixxxdj/manual#261

Which controls are we talking about?

ronso0 · 2023-09-13T18:35:14Z

@JoergAtGithub Losing these users is a bit if an exaggeration IMHO, I guess they'll either come to the forum asking for help or sinply switch to another skin.

JoergAtGithub · 2023-09-13T18:53:18Z

The documentation of the COs in the manual is incomplete, therefore it's not suitable as criteria. But there are also COs, which are not useful for mappings or skins, there I see no need for backward compatibility. For example these here: mixxxdj/manual#549

Swiftb0y · 2023-09-13T18:59:07Z

The documentation of the COs in the manual is incomplete, therefore it's not suitable as criteria. But there are also COs, which are not useful for mappings or skins, there I see no need for backward compatibility.

Right so lets complete it and then take that as authority on what we deem public and private. Or we build the concept of visibilty into COs outright...

Holzhaus · 2023-09-13T19:38:31Z

I added a column in the CO table above. Please have a look. If you think that any COs which are marked with ❌ should be aliased, let's add documentation for it.

JoergAtGithub · 2023-09-13T19:54:58Z

I added a column in the CO table above. Please have a look. If you think that any COs which are marked with ❌ should be aliased, let's add documentation for it.

I just googled for these COs. I found the following mentioned to be used in mappings:
[Master],show_mixer
[Master],guiTick50ms
[Master],num_microphones

Holzhaus · 2023-09-13T22:57:11Z

Please have a look at #11980. This superseedes this PR. It's only a partial refactor to keep the diff small.

[Master],num_microphones

Alright, I added an alias in that PR.

[Master],guiTick50ms

I struggle to see why any mapping should use that. What does have a mapping to do with vsync? Why not use a regular JS timer? If we don't add an alias, the worst that happens is that the mapping will print a warning on the terminal and some callback is not triggered for that CO.

And I'd rather not add documentation for that CO to the manual, because it sounds like an incredible ugly hack to use this from a CO.

Swiftb0y · 2023-09-13T23:06:29Z

I struggle to see why any mapping should use that. What does have a mapping to do with vsync? Why not use a regular JS timer?

The three mappings I've seen this in they all bind this for polling a CO that would otherwise often (playposition for jogleds, VuMeter, etc). A timer would likely suffice, though there might be more jitter with those. Whether this practice is valid or needed at all, I don't know. I don't know how often these frequently updated COs trigger and whether that causes congestion on the USB connection or within the controller engine. This should probably investigated, though I'd imagine for that to be quite time-consuming...

JoergAtGithub · 2023-09-13T23:09:33Z

The three mappings I've seen this in they all bind this for polling a CO that would otherwise often (playposition for jogleds, VuMeter, etc). A timer would likely suffice, though there might be more jitter with those. Whether this practice is valid or needed at all, I don't know.

At least we recommended mapping developers to use it in the past: https://mixxx.discourse.group/t/how-do-i-use-scripting-to-make-leds-flash/14418/2

daschuer · 2023-09-14T06:12:52Z

The advantage of using [Master],guiTick50ms for blinking LEFs over a J's timer is that it is in sync with the GIU.

Swiftb0y · 2023-09-14T13:43:51Z

Sure, but no LED should blink with a period of 50ms in practice IMO. that's what indicator_250ms and indicator_500ms are for. For VuMeters and other indicators which are less distracting a higher update-rate is valid, but that should rather depend on JSengine and bus load instead of GUI framerate IMO.

ywwg · 2023-11-27T19:30:21Z

I propose to remove this (still draft) PR from the 2.4 release milestone. I would really like to get the language fixed but changing these values has already caused regressions in controller configs, so it's high-risk. We have a good track record of moving in the right direction on this issue so it's not critical to make the statement in this release.

ywwg · 2024-01-08T00:49:26Z

I will bump this to 2.4.1 for now but the scale of the change feels like it might need to bump to 2.5. Open to input / opinions!

JoergAtGithub · 2024-01-08T00:54:18Z

We shouldn't change the behavior in a bugfix release. Especiallly as this requires changes of the Manual, and we've only one common Manual branch for all 2.4.x releases.
I would be fine with 2.4.0 or 2.5.0, but not 2.4.1.

ywwg · 2024-01-08T14:28:41Z

Agree. changed to 2.5

daschuer · 2024-05-08T06:00:08Z

I am afraid we have missed the merging slot. Now that the 2.5-deadline has passed I propose to shift this to 2.6.
Please revert the milestone if you do not agree.

github-actions · 2024-10-19T00:09:19Z

This PR is marked as stale because it has been open 90 days with no activity.

Holzhaus added 10 commits September 11, 2023 16:12

refactor: Rename [Master] to [Main] and add group alias

d2f2eb8

refactor: Add group alias support for effect chains/slots

0e97149

refactor: Use [Main] group in entire C++ code

8a48f12

chore(keyboard): Use [Main] group in keyboard mappings

c68577a

chore(qml): Use [Main] group in QML skin

09dbc8f

refactor(skins): Use [Main] group in all skins

40bdb8b

refactor(tools): Use [Main] group in make_xone.py script

5ad20c2

refactor(controllers): Use [Main] group in built-in mappings

87763ce

refactor: Rename [MasterOutput] to [MainOutput] and add group alias

7722a5a

refactor(skins): Use [MainOutput] group in all skins

beb9ba1

github-actions bot added code quality controller mappings library skins ui labels Sep 11, 2023

Holzhaus added this to the 2.4.0 milestone Sep 11, 2023

Swiftb0y reviewed Sep 11, 2023

View reviewed changes

JoergAtGithub requested changes Sep 11, 2023

View reviewed changes

Holzhaus mentioned this pull request Sep 13, 2023

CO Renaming (Pt. 1) #11980

Merged

Holzhaus marked this pull request as draft September 15, 2023 13:23

Holzhaus mentioned this pull request Sep 17, 2023

CO Renaming (Pt. 2): Skin Controls #11996

Merged

ywwg modified the milestones: 2.4.0, 2.4.1 Jan 8, 2024

ywwg modified the milestones: 2.4.1, 2.5.0 Jan 8, 2024

ronso0 added controller backend and removed controller mappings labels Feb 20, 2024

daschuer modified the milestones: 2.5-beta, 2.6-beta May 8, 2024

github-actions bot added the stale Stale issues that haven't been updated for a long time. label Oct 19, 2024

		s_qCOAliasHash.insert(originalKey, aliasKey);
		s_qCOHash.insert(aliasKey, pControl);

Remove offensive language (Pt. 3) #11960

Are you sure you want to change the base?

Remove offensive language (Pt. 3) #11960

Conversation

Holzhaus commented Sep 11, 2023

Swiftb0y left a comment

Choose a reason for hiding this comment

Swiftb0y Sep 11, 2023

Choose a reason for hiding this comment

Swiftb0y Sep 11, 2023

Choose a reason for hiding this comment

Holzhaus Sep 11, 2023

Choose a reason for hiding this comment

Swiftb0y Sep 11, 2023 • edited Loading

Choose a reason for hiding this comment

Swiftb0y Sep 11, 2023

Choose a reason for hiding this comment

Holzhaus commented Sep 11, 2023

Swiftb0y commented Sep 11, 2023

Holzhaus commented Sep 11, 2023 • edited Loading

Swiftb0y commented Sep 11, 2023

daschuer commented Sep 11, 2023

daschuer commented Sep 11, 2023

Swiftb0y commented Sep 11, 2023

Swiftb0y commented Sep 11, 2023

JoergAtGithub left a comment

Choose a reason for hiding this comment

daschuer commented Sep 11, 2023

daschuer commented Sep 11, 2023

ronso0 commented Sep 11, 2023

Swiftb0y commented Sep 11, 2023 • edited Loading

JoergAtGithub commented Sep 13, 2023

ronso0 commented Sep 13, 2023

ronso0 commented Sep 13, 2023

JoergAtGithub commented Sep 13, 2023 • edited Loading

Swiftb0y commented Sep 13, 2023

Holzhaus commented Sep 13, 2023

JoergAtGithub commented Sep 13, 2023

Holzhaus commented Sep 13, 2023

Swiftb0y commented Sep 13, 2023 • edited Loading

JoergAtGithub commented Sep 13, 2023

daschuer commented Sep 14, 2023

Swiftb0y commented Sep 14, 2023

ywwg commented Nov 27, 2023

ywwg commented Jan 8, 2024

JoergAtGithub commented Jan 8, 2024

ywwg commented Jan 8, 2024

daschuer commented May 8, 2024

github-actions bot commented Oct 19, 2024

Swiftb0y Sep 11, 2023 •

edited

Loading

Holzhaus commented Sep 11, 2023 •

edited

Loading

Swiftb0y commented Sep 11, 2023 •

edited

Loading

JoergAtGithub commented Sep 13, 2023 •

edited

Loading

Swiftb0y commented Sep 13, 2023 •

edited

Loading