Only setup controller devices after UI has loaded to prevent control object dependency loop by ywwg · Pull Request #4651 · mixxxdj/mixxx

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

Alternative to #4647

…object dependency loop

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

I don't have qt6 working so I can't test with QML

ywwg · 2022-01-29T16:58:08Z

                &MixxxMainWindow::initializationProgressUpdate);
        pCoreServices->initialize(pApp);
        mainWindow.initialize();
+        pCoreServices->getControllerManager()->setUpDevices();


this line could alternatively go in mixxxmainwindow

At least there is an asymetie for the QMl and widget case. Can we move both to main, or do we have a good reason for the current places?

Holzhaus · 2022-01-29T21:18:48Z

Not that this does not solve the problem. It only works in certain circumstances, i.e. the skin in use needs to define the same COs that the mapping uses.

Use a custom skin that doesn't support maximize_library? -> The controller mapping becomes unusable.

The same goes for the QML UI, which doesn't support skin-defined COs at all.

IMHO, built-in mapping mustn't use any non-standard COs, so that they always work, regardless of the skin that is used. So we either need to make these COs official (as proposed in #4647) or we need to remove usage of non-standard COs like maximize_library from all built-in mappings.

uklotzde · 2022-01-29T22:35:24Z

Couldn't get my controller working with the QML version so I cannot really comment on this PR.

The legacy skins at least work.

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

ok I see what you mean. Let's keep #4647 and make these official COs

ronso0 · 2022-01-29T23:58:31Z

Use a custom skin that doesn't support maximize_library? -> The controller mapping becomes unusable.

Please explain what "unusable" means @Holzhaus
Is A the entire mapping rejected or
B just that particular button/knob/LED mapping?
I did not find an explanation, neither in https://bugs.launchpad.net/mixxx/+bug/1914969
nor https://bugs.launchpad.net/mixxx/+bug/1941027
or https://mixxx.zulipchat.com/#narrow/stream/113295-controller-mapping/topic/.5BPreviewDeck.5D.2C.20show_previewdeck
or https://mixxx.zulipchat.com/#narrow/stream/109171-development/topic/initialize.20skin.20controls

A would affect mappings that use unofficial skin COs for connectControl/makeConnection, right?
Because unofficial skin COs used for <outputs> in xml will throw a warning but besides that the mapping "works".

ronso0 · 2022-01-30T00:03:06Z

Besides:
thanks @ywwg for the quick fix, I didn't notice it's that easy. I think this is what I will use personally
(FWIW building commit with ControllerManager is 1cebbb7)

Also, I think we should have both
#4647 for the 'unofficial' controls we offer in the View menu
#4651 so other (skin) controls can be used with custom mappings

Holzhaus · 2022-01-30T00:14:12Z

A. Trying to use a non-existing CO from JavaScript results in a JS exception, making the whole mapping unusable.

And even if we changed that and just logged a warning (which we shouldn't, because this really helps detecting mapping bugs), it would lead to weird behavior. Because if the CO doesn't exist when the mapping is loaded, then the script won't be notified when the CO is created. So even after switching skins, the button does not work unless the mapping is reloaded.

Holzhaus · 2022-01-30T00:16:15Z

Also, I think we should have both
#4647 for the 'unofficial' controls we offer in the View menu
#4651 so other (skin) controls can be used with custom mappings

I agree. #4651 is not a replacement for this PR, but it would fix the case that someone uses a custom skin + custom mapping and wants to use custom COs in both.

Swiftb0y · 2022-01-30T00:25:19Z

A. Trying to use a non-existing CO from JavaScript results in a JS exception, making the whole mapping unusable.

IIRC thats only the case in controllerDebug mode, but the this solution suffers from other major issues so it doesn't matter.

ronso0 · 2022-01-30T00:52:39Z

And even if we changed that and just logged a warning (which we shouldn't, because this really helps detecting mapping bugs), it would lead to weird behavior. Because if the CO doesn't exist when the mapping is loaded, then the script won't be notified when the CO is created. So even after switching skins, the button does not work unless the mapping is reloaded.

It's all about this, right?
throwJSError(err) means the js engine stops evaluating the script?

mixxx/src/controllers/scripting/legacy/controllerscriptinterfacelegacy.cpp

Lines 230 to 241 in d0449ae

    
           ControlObjectScript* coScript = getControlObjectScript(group, name); 
        
           if (coScript == nullptr) { 
        
               // The test setups do not run all of Mixxx, so ControlObjects not 
        
               // existing during tests is okay. 
        
               if (!m_pScriptEngineLegacy->isTesting()) { 
        
                   m_pScriptEngineLegacy->throwJSError( 
        
                           "script tried to connect to " 
        
                           "ControlObject (" + 
        
                           group + ", " + name + ") which is non-existent."); 
        
               } 
        
               return QJSValue(); 
        
           }

Okay, log a warning (= popup message) would be plausible, as well as that particular control not working if there is no counterpart in Mixxx, that's natural. But the "rest" of the mapping should work -- compared to "control non-existant, mapping disabled", open the js file, fix/disable the respective part, save, reload mapping.

What I'm trying to say:
It would be nice if the situation for users with custom mappings could be improved with simple, straight-forward fixes like this PR, #4647 and another one for the snippet quoted above? (if that actually disables mapping when not in controllerDebug mode

Note: it should still not be permitted to use 'inofficial' COs in built-in mappings because the UX could be impaired.

ywwg · 2022-02-05T18:40:33Z

I think I agree with ronso0 -- we should allow controllers to invent new control objects to easily enable new features. We could make sure that in debug or controller developer mode, we pop up a warning but continue to load the mapping. In release mode, it should be fine to silently create the control and log a warning. that would mean a little more work for this PR.

So I think maybe we should include this fix as well as #4661 -- we have a new list of official GUI controls that should be fully supported, and we can make life easier for script developers.

Holzhaus · 2022-02-05T19:18:32Z

I think I agree with ronso0 -- we should allow controllers to invent new control objects to easily enable new features.

Controllers we never capable of inventing new control objects, only skins.

I can see why someone thought it was a good idea to let skins create new COs, but this leads to all kinds of issues and also incompatibilities with controller mappings (skin lock-in). Anyway, now that we have that feature, we cannot remove it until we drop legacy skins.

We could make sure that in debug or controller developer mode, we pop up a warning but continue to load the mapping. In release mode, it should be fine to silently create the control and log a warning.

It may be tempting to just log a warnings, but IMHO it would be wrong for 2 reasons:

It's harder to detect broken mappings when only a warning is logged. We cannot verify if a mapping contributor actually tried the mapping in "developer mode" or not.
I'd rather notice issues upfront when starting Mixxx, rather than noticing that my controller mapping only works partially and having to deal with that halfway into a set.

So I think we should not change that.

that would mean a little more work for this PR.

If we really decide to change the JS exception to a log message, that should be a separate PR anyway.

So I think maybe we should include this fix as well as #4661.

Yes, fixing the dependency issue for people who want to use skins and mappings with custom, unofficial COs makes sense.

ronso0 · 2022-02-05T23:21:49Z

I was only referring to the current state of this PR: skin controls available for mappings -- no new features.

ywwg · 2022-02-06T22:15:08Z

so @ronso0 do you think we still need something like this now that #4661 is in?

Holzhaus · 2022-02-06T23:01:22Z

so @ronso0 do you think we still need something like this now that #4661 is in?

What does "something like this" refer to? If it refers to this PR (i.e. if you're asking if we should fix the initialization order to fix the dependency issues), then the answer is "yes we should fix it".

Currently this PR does not build due to the issue that Uwe pointed out above. After that is fixed, I'd be in favor of merging it.

ronso0 · 2022-02-06T23:10:30Z

so @ronso0 do you think we still need something like this now that #4661 is in?

Yes, I'd appreciate this fix here. It's very simple and I don't spot what could go wrong.
I linked the building commit with the missing inlcude Uwe noticed 1cebbb7, but I didn't properly test it yet with my mapping.

Potential CON:
it takes slightly longer until controllers are ready to go? Or is that cancelled out by less parallel jobs while the UI is loading?

ywwg · 2022-02-10T04:17:43Z

ok I will fix the issue so we can merge this as well.

I don't think the delay in controllers being available is particularly noticeable to the user.

ronso0 · 2022-03-17T20:38:36Z

ping @ywwg

ywwg · 2022-03-18T14:55:08Z

ah I'll fix the build issue and repush

ywwg · 2022-03-18T15:12:19Z

confirmed builds with qt6

ywwg · 2022-03-19T20:07:59Z

@ronso0 anything else needed for this?

ronso0 · 2022-03-22T20:35:14Z

Code LGTM.
If this wasn't yet tested with both legacy skins and QML I can test this with my controller tomorrow night, though only with legacy skins.

github-actions · 2022-06-21T00:06:47Z

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

ronso0 · 2022-10-15T12:36:22Z

Appearantly also relevant for COs created by the preferences, for example [Controls], ShowDurationRemaining
#10967

Did someone test it with QML? (I can't, haven't setup Qt6 yet)

robbert-vdh · 2022-12-19T01:42:38Z

This patch also fixes EQ parameter changes not going through when they're read from a controller on Mixxx startup.

ronso0 · 2022-12-19T11:45:56Z

@robbert-vdh Thanks for testing. Could you also test with Qt 6, for sake of completeness?
If that works as well we can finally merge this.

robbert-vdh · 2022-12-19T11:47:23Z

@ronso0 I'm using Qt 6.4.1.

robbert-vdh · 2022-12-19T11:48:27Z

Wait, never mind, Mixxx is built against Qt5. Let me try building against Qt6.

robbert-vdh · 2022-12-19T12:00:41Z

Mixxx doesn't build against Qt 6.4.3 for me. Form the 3000 lines of compiler errors it looks like something that previously implicitly converted no longer does, but I sadly don't have the time to decipher this right now.

ywwg · 2023-01-16T23:19:32Z

would be cool to get this merged in! This is tested with legacy skins, can someone check against QML?

ronso0 · 2023-01-16T23:35:21Z

would be cool to get this merged in! This is tested with legacy skins, can someone check against QML?

@mixxxdj/developers

ronso0 · 2023-03-14T08:22:20Z

Hitting merge now, finally! Thanks for the fix.

Only setup controller devices after UI has loaded to prevent control …

b0cdcdd

…object dependency loop

ywwg mentioned this pull request Jan 29, 2022

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

Merged

ywwg commented Jan 29, 2022

View reviewed changes

uklotzde suggested changes Jan 29, 2022

View reviewed changes

Comment thread src/qml/qmlapplication.cpp

ywwg closed this Jan 29, 2022

ywwg reopened this Feb 5, 2022

ywwg added 2 commits March 18, 2022 10:55

Merge branch 'main' into controllers-after-window

c7a56a0

controllers after window: fix includes for qt6 / qml

8508269

github-actions Bot added the stale Stale issues that haven't been updated for a long time. label Jun 21, 2022

ronso0 mentioned this pull request Jul 5, 2022

Numark Scratch: add initial mapping #4834

Merged

ronso0 mentioned this pull request Oct 15, 2022

controller script error due to ShowDurationRemaining Mixxx control #10967

Open

ronso0 added this to the 2.4.0 milestone Dec 4, 2022

daschuer added needs review and removed stale Stale issues that haven't been updated for a long time. labels Dec 8, 2022

daschuer changed the base branch from main to 2.4 January 12, 2023 07:03

robbert-vdh mentioned this pull request Jan 13, 2023

Traktor Kontrol S3 script improvements, vanilla-like FX behavior, control initialization, better scratching, and more #11199

Merged

ronso0 merged commit b3a199f into mixxxdj:2.4 Mar 14, 2023

acolombier removed the needs review label Mar 24, 2025

Uh oh!

Conversation

ywwg commented Jan 29, 2022

Uh oh!

ywwg commented Jan 29, 2022

Uh oh!

ywwg Jan 29, 2022

Choose a reason for hiding this comment

Uh oh!

daschuer Jan 17, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Holzhaus commented Jan 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

uklotzde commented Jan 29, 2022

Uh oh!

ywwg commented Jan 29, 2022

Uh oh!

ronso0 commented Jan 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ronso0 commented Jan 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Holzhaus commented Jan 30, 2022

Uh oh!

Holzhaus commented Jan 30, 2022

Uh oh!

Swiftb0y commented Jan 30, 2022

Uh oh!

ronso0 commented Jan 30, 2022

Uh oh!

ywwg commented Feb 5, 2022

Uh oh!

Holzhaus commented Feb 5, 2022

Uh oh!

ronso0 commented Feb 5, 2022

Uh oh!

ywwg commented Feb 6, 2022

Uh oh!

Holzhaus commented Feb 6, 2022

Uh oh!

ronso0 commented Feb 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ywwg commented Feb 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ronso0 commented Mar 17, 2022

Uh oh!

ywwg commented Mar 18, 2022

Uh oh!

ywwg commented Mar 18, 2022

Uh oh!

ywwg commented Mar 19, 2022

Uh oh!

ronso0 commented Mar 22, 2022

Uh oh!

github-actions Bot commented Jun 21, 2022

Uh oh!

ronso0 commented Oct 15, 2022

Uh oh!

robbert-vdh commented Dec 19, 2022

Uh oh!

ronso0 commented Dec 19, 2022

Uh oh!

robbert-vdh commented Dec 19, 2022

Uh oh!

robbert-vdh commented Dec 19, 2022

Uh oh!

robbert-vdh commented Dec 19, 2022

Uh oh!

ywwg commented Jan 16, 2023

Uh oh!

ronso0 commented Jan 16, 2023

Uh oh!

ronso0 commented Mar 14, 2023

Uh oh!

Holzhaus commented Jan 29, 2022 •

edited

Loading

ronso0 commented Jan 29, 2022 •

edited

Loading

ronso0 commented Jan 30, 2022 •

edited

Loading

ronso0 commented Feb 6, 2022 •

edited

Loading

ywwg commented Feb 10, 2022 •

edited

Loading