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

Skins: move skin control hack to c++ (spinny/cover controls, mic/ducking controls) #11183

Merged
merged 4 commits into from
Jan 8, 2023

Conversation

ronso0
Copy link
Member

@ronso0 ronso0 commented Jan 3, 2023

... so we can get rid of the xml hack in LateNight which has been unreliable lately, see #11126

That WSizeAwareStack / WWidgetStack hack helped to keep the LateNight xml decks "compact" and rather easy to maintain (IMO : ):

  • [Latenight],show_spinny_cover was set to 1 if either spinnies or deck cover arts were enabled
  • that was used to a) cover/reveal the skin setting "Big spinny / cover" and b) simplified toggling the respective xml elements a lot (via [LateNight],show_big_spinny_cover and [LateNight],show_small_spinny_cover

In the same way the mic ducking controls were hidden if no mic was configured.

This PR moves this basic scripting to c++. It's set up once in SkinLoader::loadConfiguredSkin when the first skin is loaded and all mic controls have been created.
All controls are now in the 'official' [Skin] group and should also be documented in the manual. The spinny and coverart controls are now created in CoreServices like other common skin controls.

Even though these control are now hardcoded, I consider this a robust solution for official skins (low risk, no/little maintenance) until QML allows true skin scripting.

Let me know what you think.

@github-actions github-actions bot added the skins label Jan 3, 2023
@ronso0
Copy link
Member Author

ronso0 commented Jan 3, 2023

This built and works flawlessly with Qt 5.12.8 on ubuntu, I'm curious was CI says...

Copy link
Member

@daschuer daschuer left a comment

Choose a reason for hiding this comment

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

Regarding the getCintrol() calls here, is work in progress to get rid of all of them, because of lifetime an threading issues.

It is a none owning pointer and must only be used when when we are sure it is not deleted.

Here they are stored in the lamb code segment, which is at least it is against the rule that owning raw pointers must not be stored.

You can replace them by PollingControlProxy which is a basically a Shared pointer.

The connect calls can be done with getControl() since they are created by the GUI thread with the skin. However it would be more clean to create them here, or use ControlProxiies. Unsure about the implications though. .

src/skin/skinloader.cpp Outdated Show resolved Hide resolved
src/skin/skinloader.cpp Outdated Show resolved Hide resolved
src/skin/skinloader.cpp Outdated Show resolved Hide resolved
@ronso0
Copy link
Member Author

ronso0 commented Jan 6, 2023

Thanks for the hints @daschuer
I tried to avoid getControl() and experimented with ControlProxy, but if I only want to set up the toggles and function if a skin created the controls it gets rather complex. Nevertheless, as always I learned something, this time about function pointers.

I decided to simply create the controls unconditionally here in SkinLoader (didn't find a more suitable place) and everything is pretty straightforward now.

src/skin/skinloader.cpp Outdated Show resolved Hide resolved
src/skin/skinloader.cpp Outdated Show resolved Hide resolved
src/skin/skinloader.cpp Show resolved Hide resolved
src/skin/skinloader.cpp Outdated Show resolved Hide resolved
@ronso0 ronso0 marked this pull request as ready for review January 6, 2023 15:28
@ronso0
Copy link
Member Author

ronso0 commented Jan 6, 2023

I'll create a manual PR to document all skin controls which are already listed in ControlPickerMenu.

Copy link
Member

@daschuer daschuer left a comment

Choose a reason for hiding this comment

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

Thank you. LGTM

@daschuer daschuer merged commit 9e43b25 into mixxxdj:main Jan 8, 2023
@ronso0 ronso0 deleted the skin-controls-scripting branch January 8, 2023 22:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants