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

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

Merged
merged 7 commits into from Sep 19, 2023
Merged

Conversation

Holzhaus
Copy link
Member

This PR is part of the ongoing effort to remove offensive language from the Mixxx codebase (#11931).

This takes care of the skin control renamings discussed in #11960 (comment).

Old Name New Name Alias?
[Master],maximize_library [Skin],show_maximized_library ✔️
[Master],show_mixer [Skin],show_mixer
[Master],skin_settings [Skin],show_settings

@Holzhaus Holzhaus added code quality control objects Issues and bugs specifically in regard to mixxx `ControlObjects` labels Sep 17, 2023
@Holzhaus Holzhaus added this to the 2.4.0 milestone Sep 17, 2023
Copy link
Member

@JoergAtGithub JoergAtGithub left a comment

Choose a reason for hiding this comment

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

Please add these 3 legacy CO names to ControlObjectAliasTest too.

@Holzhaus
Copy link
Member Author

Holzhaus commented Sep 17, 2023

Please add these 3 legacy CO names to ControlObjectAliasTest too.

We can't, CoreServicesTest is broken on CI. See #11984.

Also, only maximize_library has been aliased, the others are undocumented.

@github-actions github-actions bot added the build label Sep 17, 2023
@Holzhaus
Copy link
Member Author

Alright, I moved the CO creation into a separate class and added a test for the maximize_library alias.

Copy link
Member

@ronso0 ronso0 left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM
Except we could move the VC skin control, too.

Comment on lines 56 to 57
ConfigKey(QStringLiteral("[VinylControl]"),
QStringLiteral("show_vinylcontrol")),
Copy link
Member

Choose a reason for hiding this comment

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

controlpickermenu.cpp still uses VINYL_PREF_KEY
Shouldn't we move this to the Skin group as well?

Copy link
Member Author

@Holzhaus Holzhaus Sep 17, 2023

Choose a reason for hiding this comment

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

Sounds reasonable. The same applies to the other COs in uicontrols.cpp.

I only renamed the COs that had to be renamed, but if we want to make stuff consistent, I would propose this:

Old Name New Name
[EffectRack1],show [Skin],show_effectrack
[Library],show_coverart [Skin],show_library_coverart
[Microphone],show_microphone [Skin],show_microphone
[PreviewDeck],show_previewdeck [Skin],show_previewdeck
[Samplers],show_samplers [Skin],show_samplers
[VinylControl],show_vinylcontrol [Skin],show_vinylcontrol

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, sure, go ahead! IMO it doesn't make much sense binding skin controls to these engine groups.
I didn't dare to propose changing all those atm to not slow down your renaming rush ; )

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, sure, go ahead!

If everyone agrees of course!

Copy link
Member Author

@Holzhaus Holzhaus Sep 19, 2023

Choose a reason for hiding this comment

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

Done. Note that used show_microphones and show_previewdecks (with an additional s at the end) to match the plural in show_samplers. Our current skins show multiple microphones anyway, so it fits better. Only a single preview deck is used, but If some skin designer wants to use multiple, why not (we support 4 IIRC)?

Copy link
Member

@Swiftb0y Swiftb0y left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you.

@Swiftb0y Swiftb0y merged commit ad3bba7 into mixxxdj:2.4 Sep 19, 2023
11 of 13 checks passed
@Holzhaus Holzhaus added blocker and removed blocker labels Sep 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build code quality control objects Issues and bugs specifically in regard to mixxx `ControlObjects` controller mappings skins ui
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants