Create wmainmenubar controls in coreservices so they are available to controllers on startup #4647

ywwg · 2022-01-25T02:15:42Z

The coreservices refactor broke controllers that bind a button to maximize_library. This is because controllers are initialized before the main window, because main window initialization depends on core services, and core services sets up controllers. So it's a catch-22.

I have solved this by creating those controls in coreservices, much like touch_shift is created. This solves the problem with my S3 controller.

Holzhaus · 2022-01-25T14:11:39Z

I agree that something like this is necessary. With QML, all mappings that use those COs throw an error because they don't exist.

However, currently the menu bar checks if the COs exist and Grey out the corresponding menu option if not. I think this is unreliable already (e. g. if you switch from a skin that has these COs to one that doesn't, the COs persist anyway). Maybe we can add an XML element to skin.xml that indicates support <featureSupported>maximize_library</featureSupported> and grey out entries based on that.

@ronso0 what do you think?

In any case, the issue with broken mappings is arguably worse than the non-working menu bar items with certain skins, so I think this does not have to be fixed in this PR.

ywwg · 2022-01-25T19:10:29Z

could we detect support during skin creation, in the code that hooks up the COs, but testing to see if that CO is in a given list? Then skin authors wouldn't have to remember to add another item. Agree that this is best done in a followup

Holzhaus · 2022-01-25T19:42:23Z

Not sure, that sounds pretty complex and I'm unsure if it's worth to put that much work into this when the only downside is that the menu entry is greyed out when the skin designer forgets to add it.

Holzhaus · 2022-01-25T19:44:49Z

I think @ronso0 also worked on these COs (#4355), so I'd be interested in his feedback on this PR.

ronso0 · 2022-01-25T21:44:28Z

I think we should merge this.
It's step forward, and tbh the greyed out / unreliable checkbox... I don't care as long as it's resolving the mapping issue.
Does anyone still care about the renaming to [Library],maximized? #4355/files
__
I'm against the skin-supported controls manifest approach because 1. complexity, 2. maintainaaance and 3. inoffical / older skins are not covered. Btw @poelzi tried that approach in #3189, see last 5 commits 3189/commits (I did neither review nor test that!)

Though, for me neither this PR nor the feature list approach would work because in my mapping I use a few skin-specific controls (toggle mixxer, toggle waveforms, ...).

If I get it right the core issue is the race condition between loading skins and loading mappings, right?
Is there a way we can make use of m_skinCreatedControls?
https://github.com/mixxxdj/mixxx/blob/main/src/mixxxmainwindow.cpp#L1040-L1042

Really clumsy idea: have a prepared controls list per skin?
store all skin-created controls somewhere on shutdown, create them on startup in coreservices.
Like generate such a control list for each skins when it's loaded? (ship that for each official skin?)
First time a modified skin is loaded the issue would return, but only once. On next start the mapping would work again.
~~Or have a pre-commit hook that does it (which would exclude inoffical skins again)~~

ywwg · 2022-01-25T22:19:35Z

The issue is not a race condition, it's a dependency loop. mainwindow depends on coreservices, so coreservices is created first. But the COs that the controllers want to connect to are created in the main window.

lmk if I can merge or if I should wait for someone else to do so

ronso0 · 2022-01-25T22:32:14Z

Ah right, that's why I did #4355 as I did : )

What do you think about my proposal of having a prepared controls list to create the controls in coreservices?

ronso0 · 2022-01-25T22:32:49Z

@Holzhaus LGTY?

Holzhaus · 2022-01-25T22:49:00Z

src/coreservices.cpp

@@ -422,6 +422,21 @@ void CoreServices::initialize(QApplication* pApp) {

    m_pTouchShift = std::make_unique<ControlPushButton>(ConfigKey("[Controls]", "touch_shift"));

+    // The following wmainmenubar controls must be created here so that controllers can bind to them
+    // on startup.
+    m_pSkinSettingsControl = std::make_unique<ControlPushButton>(


This is a binary flag, not a button, right? Shouldn't this be

Suggested change

m_pSkinSettingsControl = std::make_unique<ControlPushButton>(

m_pSkinSettingsControl = std::make_unique<ControlObject>(

?

good catch thanks

I don't understand the difference between [Master],skin_settings and [Microphone],show_microphone for example. All those are used for binary pushbuttons.
May someone please elaborate?

This makes a difference when linking a midi button and is also used for the default behavior in skins.
A plain ControlObject has no hints how to link a midi button and should be only used for internal controls.

so is skin_settings not triggered by midi? Is that the only reason not to set as a pushbutton?

Holzhaus · 2022-01-27T09:38:28Z

Does anyone still care about the renaming to [Library],maximized? #4355/files

I do, but this doesn't need to happen in this PR IMHO. As of now, it's an unofficial CO. Before making it "official", we should rename it.

ywwg · 2022-01-28T04:04:52Z

What do you think about my proposal of having a prepared controls list to create the controls in coreservices?

let me see if this will work -- I agree it makes sense to reuse that mechanism. (I'll try to look at this tomorrow / this weekend)

ronso0 · 2022-01-28T21:25:10Z

Yes, it works.
Quick test with a fixed list 6186454
No warning when starting Mixxx with a MIDI-through-port mapping using these controls for outputs.

The tedious part is to set up the file im/export parser.
Use ConfigObject and add some methods that work without configkey values?

Holzhaus · 2022-01-28T21:49:44Z

Yes, it works. Quick test with a fixed list 6186454 No warning when starting Mixxx with a MIDI-through-port mapping using these controls for outputs.

The tedious part is to set up the file im/export parser. Use ConfigObject and add some methods that work without configkey values?

I don't understand how this approach is supposed to work tbh.

ronso0 · 2022-01-28T22:17:21Z

there is m_skinCreatedControls, populated when a skin is loaded
export that list to a file once on demand ¹ when we change or add a skin
coreservices always looks up that file for the configured skin and creates all controls
= all skin controls are available for controller mappings before the skin is loaded

¹ for custom skins that file export can be triggered with --exportSkinControls once when the skin is uodated, then new controls are available at the next start

Make sure m_skinCreatedControls is populated after all other controls are created (menubar, engine, effects, ..?) to avoid overlap?

Holzhaus · 2022-01-28T22:27:01Z

Hmm, this still doesn't fix the issue that any controller mapping that uses those COs will not work when a skin is used that doesn't support all of those COs. In that case, I think we need to remove usage of those COs in built-in skins then.

This also doesn't solve the issue for the QML UI which doesn't support skin-defined COs.

What is the problem with the simple solution in this PR?

ronso0 · 2022-01-28T23:45:11Z

What is the problem with the simple solution in this PR?

No problem, it works for the menu bar controls.
Though, [Skin],show_mixer or [LateNight],show_waveforms would still throw errors.

Thus my proposal (some months ago) to create all major skin controls in C++, e.g. show_mixer, show_samplers, show_waveforms etc.

ywwg · 2022-01-29T01:57:17Z

I see what you are saying but this solution seems over-complicated for this particular problem. And there are issues like these items are created by the menu system, not the skin, so it's not quite the right fit. Let's merge this fix and we can iterate on it later, especially as QML becomes more mature.

ywwg · 2022-01-29T02:00:43Z

(I also don't like the idea of saving a list to a file... I think there could be a more robust solution like moving controller loading after skin creation)

ronso0 · 2022-01-29T11:29:53Z

I see what you are saying but this solution seems over-complicated for this particular problem. And there are issues like these items are created by the menu system, not the skin, so it's not quite the right fit.

Okay. I already approved this PR, so I thought we were just discussing my proposal which is only for the skin COs.

moving controller loading after skin creation

Sure, that's the most obvious solution, though no one worked on it, yet.
Until someone does I'll try to use an simple txt file so I can connect some LEDs (without having to rebuild after changing my mapping). I use an oldschool mapping without components so I'm not really affected by the issue anyway.

ywwg · 2022-01-29T16:15:38Z

so are we self-merging PRs now? I let people do that with my approvals but I wasn't sure if it was reciprocal.

I will poke to see if we can init controllers after skins

ywwg · 2022-01-29T16:23:59Z

oh, pushed the controlobject type fix

ywwg · 2022-01-29T16:57:34Z

oh I think I might have a simpler fix #4651

uklotzde · 2022-01-29T18:59:40Z

Changed to draft to defer merging. Will test #4651.

ywwg · 2022-01-29T23:01:56Z

@Holzhaus makes a good point that it's probably better to make these COs official, in which case coreservices is a good place to put them

ronso0 · 2022-01-30T00:11:10Z

IMO there's no point in limiting this to the controls used in the View menu.
Let's make sure all controls we offer in the MIDI Learning Wizard are available -- Or remove them from the wizard.

I think it's only those from the top section that are missing -- everything else is created in c++ already (I think):

ywwg · 2022-01-30T15:03:07Z

effectrack show/hide is just [EffectRack1],show. I am unable to figure out where EffectRack1 is even created so I can't figure out if that CO is generated by code.

… controllers on startup

Add remaining skin controls

ywwg · 2022-01-30T15:15:46Z

I think we obsoleted effectracks, so it's correct to just create this CO manually

src/coreservices.h

src/coreservices.cpp

ywwg · 2022-01-31T19:13:49Z

Converted to a vector of pointers since the objects themselves don't need to be individually referenced anywhere. This makes Skin Settings a controlpushbutton again, which based on the discussion I think is correct. A user might have a midi button to open the skin settings in which case we'd want it to be a toggle like the others

src/coreservices.cpp

Co-authored-by: Daniel Schürmann <daschuer@mixxx.org>

ywwg · 2022-02-01T23:25:05Z

@daschuer please mark as approved if you're satisfied

daschuer

LGTM, Thank you for taking care.

ywwg · 2022-02-04T03:05:57Z

This is broken for some reason -- when I start mixxx the mixer doesn't show up. reverting.

Holzhaus requested a review from ronso0 January 25, 2022 19:37

ronso0 approved these changes Jan 25, 2022

View reviewed changes

ronso0 mentioned this pull request Jan 25, 2022

always create [Library],maximize_library #4355

Closed

Holzhaus reviewed Jan 25, 2022

View reviewed changes

ywwg mentioned this pull request Jan 29, 2022

Only setup controller devices after UI has loaded to prevent control object dependency loop #4651

Merged

uklotzde marked this pull request as draft January 29, 2022 18:59

uklotzde marked this pull request as ready for review January 29, 2022 22:31

ywwg added 2 commits January 30, 2022 10:08

Create wmainmenubar controls in coreservices so they are available to…

c611f25

… controllers on startup

Fix skinsettingscontrol object type

6cacbea

Add remaining skin controls

ywwg force-pushed the menu-controls branch from 447ab27 to 6cacbea Compare January 30, 2022 15:16

daschuer requested changes Jan 30, 2022

View reviewed changes

src/coreservices.h Outdated Show resolved Hide resolved

src/coreservices.cpp Outdated Show resolved Hide resolved

Use a list of ConfigKeys instead of individual objects

4a7a4dd

daschuer requested changes Feb 1, 2022

View reviewed changes

src/coreservices.cpp Show resolved Hide resolved

src/coreservices.cpp Show resolved Hide resolved

uklotzde approved these changes Feb 1, 2022

View reviewed changes

Update src/coreservices.cpp

71de54b

Co-authored-by: Daniel Schürmann <daschuer@mixxx.org>

daschuer approved these changes Feb 2, 2022

View reviewed changes

daschuer merged commit 2a54102 into mixxxdj:main Feb 2, 2022

ywwg mentioned this pull request Feb 4, 2022

Revert "Create wmainmenubar controls in coreservices so they are available to controllers on startup" #4660

Merged

daschuer mentioned this pull request May 11, 2023

mixxx control for Big Library #10501

Closed

	m_pSkinSettingsControl = std::make_unique<ControlPushButton>(
	m_pSkinSettingsControl = std::make_unique<ControlObject>(

Create wmainmenubar controls in coreservices so they are available to controllers on startup #4647

Create wmainmenubar controls in coreservices so they are available to controllers on startup #4647

Conversation

ywwg commented Jan 25, 2022

Holzhaus commented Jan 25, 2022

ywwg commented Jan 25, 2022

Holzhaus commented Jan 25, 2022

Holzhaus commented Jan 25, 2022

ronso0 commented Jan 25, 2022 • edited Loading

ywwg commented Jan 25, 2022 • edited Loading

ronso0 commented Jan 25, 2022

ronso0 commented Jan 25, 2022

Holzhaus Jan 25, 2022

Choose a reason for hiding this comment

ywwg Jan 28, 2022

Choose a reason for hiding this comment

uklotzde Jan 29, 2022

Choose a reason for hiding this comment

ronso0 Jan 30, 2022

Choose a reason for hiding this comment

daschuer Jan 31, 2022

Choose a reason for hiding this comment

ronso0 Jan 31, 2022

Choose a reason for hiding this comment

ywwg Jan 31, 2022

Choose a reason for hiding this comment

Holzhaus commented Jan 27, 2022

ywwg commented Jan 28, 2022

ronso0 commented Jan 28, 2022

Holzhaus commented Jan 28, 2022

ronso0 commented Jan 28, 2022 • edited Loading

Holzhaus commented Jan 28, 2022 • edited Loading

ronso0 commented Jan 28, 2022

ywwg commented Jan 29, 2022

ywwg commented Jan 29, 2022

ronso0 commented Jan 29, 2022

ywwg commented Jan 29, 2022

ywwg commented Jan 29, 2022

ywwg commented Jan 29, 2022

uklotzde commented Jan 29, 2022

ywwg commented Jan 29, 2022

ronso0 commented Jan 30, 2022 • edited Loading

ywwg commented Jan 30, 2022

ywwg commented Jan 30, 2022

ywwg commented Jan 31, 2022

ywwg commented Feb 1, 2022

daschuer left a comment

Choose a reason for hiding this comment

ywwg commented Feb 4, 2022

ronso0 commented Jan 25, 2022 •

edited

Loading

ywwg commented Jan 25, 2022 •

edited

Loading

ronso0 commented Jan 28, 2022 •

edited

Loading

Holzhaus commented Jan 28, 2022 •

edited

Loading

ronso0 commented Jan 30, 2022 •

edited

Loading