Start to untangle CoreServices/MixxxMainWindow initialization #4110

Holzhaus · 2021-07-17T15:30:57Z

Based on #4109. This moves some required stuff like GuiTick/VisualsManager into CoreServices (without them, CoreServices initialization will fail with a debug assertion). It also moves the call to CoreServices::initialize out of MixxxMainWindow, because even without a MixxxMainWindow the CoreServices should be initialized (this will be necessary when we to a pure QML interface instead of embedding the QML UI inside a QQuickWidget.)

This is not yet complete, for example the sound setup (setupDevices()) should also happen in main, but this requires moving the preferences out of MixxxMainWindow, too. Unfortunately, these depend on WaveformWidgetFactory which is not needed for QML. Hence, this will need some more refacoring in later PRs.

Be-ing · 2021-07-17T18:48:23Z

CoreServicesTest.TestInitialization failed.

Holzhaus · 2021-07-18T10:18:18Z

I guess the CI doesn't have everything available for such a test. Does it work locally?

Holzhaus · 2021-07-19T16:54:52Z

Does it work locally?

Ping. This decides whether I'll remove the test or remove it entirely.

coveralls · 2021-07-22T21:29:06Z

Pull Request Test Coverage Report for Build 1081813635

0 of 2 (0.0%) changed or added relevant lines in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-2.6%) to 26.034%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/coreservices.cpp	0	2	0.0%

Totals
Change from base Build 1081645070:	-2.6%
Covered Lines:	20015
Relevant Lines:	76880

💛 - Coveralls

src/main.cpp

Be-ing · 2021-07-23T00:49:39Z

I'm not sure about the first commit. VisualsManager and GuiTick are only for rendering the legacy skins, so I think it is appropriate to leave them in MixxxMainWindow.

Be-ing · 2021-07-23T00:51:18Z

Without the GuiTick class, some COs like the cue_indicator and play_indicator can't be created, and the VisualsManager is in charge of COs like end_of_track and visual_bpm.

This seems to be hinting at a larger architectural issue that should probably be solved first...

Be-ing · 2021-07-23T03:26:55Z

Would it be feasible to rebase to remove the first commit and continue working on decoupling the indicator ControlObjects from the QWidget GUI rendering?

uklotzde · 2021-07-23T07:55:21Z

Without the GuiTick class, some COs like the cue_indicator and play_indicator can't be created, and the VisualsManager is in charge of COs like end_of_track and visual_bpm.

This seems to be hinting at a larger architectural issue that should probably be solved first...

Or move them and leave a TODO about the required architectural refactoring and their removal. Just to be able to move forward.

Holzhaus · 2021-07-23T08:38:50Z

Tbh I'm not really sure what the scope of these classes actually is. From the source code:

// This class updates the controls used for widgets and
// controller indicator, in a CPU saving way and outside the engine thread
class DeckVisuals {

// A helper class that manages the "guiTickTime" COs, that drive updates of the
// GUI from the VsyncThread at the user's configured FPS (possibly downsampled).
class GuiTick {

So it sounds like both classes only manage COs anyway?

@Be-ing Are you sure you're not confusing them with WaveformWidgetFactory, that has a startVSync(GuiTick*, VisualsManager*) method? That is indeed for legacy skins only.

Be-ing · 2021-07-23T11:23:16Z

I'll have to look deeper into the code which I don't have time to do at the moment (hopefully I will next week). In the meantime the other commits look good to me.

Be-ing · 2021-07-23T23:32:13Z

DeckVisuals and GuiTick are both coupled to the rendering of the legacy QGLWidget waveforms. I don't think it is a good idea to try to get them to work with QML somehow. I think the ControlObjects should be reimplemented some other way for QML. We don't need to know what that new implementation will be yet. I think we should remove the first commit from this branch, merge the others, and add a to-do card to the QML project board for reimplementing those COs.

Holzhaus · 2021-07-24T03:14:14Z

DeckVisuals and GuiTick are both coupled to the rendering of the legacy QGLWidget waveforms. I don't think it is a good idea to try to get them to work with QML somehow.

It already "works" with QML right now. These are initialized because we use MixxxMainWindow at the moment.

I think the ControlObjects should be reimplemented some other way for QML. We don't need to know what that new implementation will be yet.

OK.

I think we should remove the first commit from this branch, merge the others, and add a to-do card to the QML project board for reimplementing those COs.

Right now CoreServices is basically completely broken, because it cannot be initialized without GuiTick/VisualsManager although it should be self-contained. I suggest to leave the commit in and just add a comment. Initializing something that may be not be needed later is IMHO the lesser evil compared to not being able to initialize coreservices at all because some dependencies are missing.

If you insist, I can also remove the commit. But this will block me from any further decoupling of the MixxxMainWindow class and the core mixxx initialization.

Be-ing · 2021-07-26T23:45:46Z

It already "works" with QML right now. These are initialized because we use MixxxMainWindow at the moment.

It works because MixxxMainWindow calls WaveformWidgetFactory::startVSync. CoreServices should not have anything to do with WaveformWidgetFactory. It shouldn't start VSyncThread either, but I would be willing to accept that as temporary technical debt with a FIXME comment to keep these ControlObjects working.

Right now CoreServices is basically completely broken, because it cannot be initialized without GuiTick/VisualsManager although it should be self-contained.

I don't understand what you mean. CoreServices has nothing to do with these classes now; they are not required to initialize CoreServices. And that is how it should be because those classes are coupled to the QWidgets GUI.

src/coreservices.h

Holzhaus · 2021-07-28T09:57:03Z

Right now CoreServices is basically completely broken, because it cannot be initialized without GuiTick/VisualsManager although it should be self-contained.

I don't understand what you mean. CoreServices has nothing to do with these classes now; they are not required to initialize CoreServices. And that is how it should be because those classes are coupled to the QWidgets GUI.

This is wrong. CoreServices fails to initialize without them.

If you look here, you can see that the GuiTick class creates the the guiTick COs:

mixxx/src/waveform/guitick.cpp

Lines 7 to 8 in f4e4807

    
           m_pCOGuiTickTime = std::make_unique<ControlObject>(ConfigKey("[Master]", "guiTickTime")); 
        
           m_pCOGuiTick50ms = std::make_unique<ControlObject>(ConfigKey("[Master]", "guiTick50ms"));

These controls are used by ControlIndicator:

mixxx/src/control/controlindicator.cpp

Lines 12 to 13 in f4e4807

    
           m_pCOTGuiTickTime = new ControlProxy("[Master]", "guiTickTime", this); 
        
           m_pCOTGuiTick50ms = new ControlProxy("[Master]", "guiTick50ms", this);

ControlIndicator is used by CueControl for example:

mixxx/src/engine/controls/cuecontrol.cpp

Lines 172 to 173 in f4e4807

    
           m_pCueIndicator = new ControlIndicator(ConfigKey(group, "cue_indicator")); 
        
           m_pPlayIndicator = new ControlIndicator(ConfigKey(group, "play_indicator"));

If you try to initialize CoreServices without initializing GuiTick, you will always get a debug assertion due to missing control objects. And if we remove the debug assertions, the cue_indicator/play_indicator COs would stop working.

Be-ing · 2021-07-28T13:45:22Z

CoreServices is basically completely broken

This hyperbolic language does not help move forward a technical discussion.

If you try to initialize CoreServices without initializing GuiTick, you will always get a debug assertion

That is far from "completely broken". It's just a warning. Pedantically silencing warnings without addressing their cause isn't really helpful. Creating these ControlObjects is pointless without VSyncThread running. So please either remove the first commit and leave these COs broken for now or move the initialization of VSyncThread to CoreServices with a FIXME comment explaining why it does not belong there because it exists for the legacy GUI but those COs do not function without it.

Holzhaus · 2021-07-28T19:49:39Z

Rebased on #4157.

Right now CoreServices is basically completely broken, because it cannot be initialized without GuiTick/VisualsManager although it should be self-contained.

I don't understand what you mean. CoreServices has nothing to do with these classes now; they are not required to initialize CoreServices. And that is how it should be because those classes are coupled to the QWidgets GUI.

[...]

This hyperbolic language does not help move forward a technical discussion.

And neither does denying that CoreServices cannot be initialized when GuiTick is missing.

If you try to initialize CoreServices without initializing GuiTick, you will always get a debug assertion

That is far from "completely broken". It's just a warning. Pedantically silencing warnings without addressing their cause isn't really helpful.

A DEBUG_ASSERT is not just a warning. It's always a bug. Either it shouldn't be possible to trigger it, or the DEBUG_ASSERT itself is wrong. In this case it's the former. That assertion is triggered every time you try to start Mixxx without GuiTick, which always aborts the process when using our recommended build config that we use for all alpha/beta builds. That's pretty broken in my book.

Anyway, I fixed this in #4157 so it should work now without pulling in GuiTick and VisualsManager. We still need to decide what we do with the COs, but that is out of scope here.

The `MixxxMainWindow` class should not be responsible for initializing `CoreServices`. Mixxx should still be able to work when there is no `MixxxMainWindow` (e.g. when we switch to a `QQmlApplication` for QML).

Before, CoreServices would cause a debug assertion when initializing it without other stuff from `MixxxMainWindow`. This is just a smoke test to verify that it's possible to initialize the `CoreServices` class on its own, without `MixxxMainWindow.

Holzhaus · 2021-07-30T10:00:04Z

Rebased on latest main.

Be-ing · 2021-07-30T11:35:55Z

Do you plan to decouple DeckVisuals from QWidgets before or after this PR?

Be-ing · 2021-07-30T11:38:38Z

src/main.cpp

+            &mixxx::CoreServices::initializationProgressUpdate,
+            &mainWindow,
+            &MixxxMainWindow::initializationProgressUpdate);
+    pCoreServices->initialize(app);


Can this be moved to the CoreServices constructor by passing the QApplication* as another argument?

I guess we need the main window for monitoring progress during the initialization?

I guess we need the main window for monitoring progress during the initialization?

Indeed. CoreServices are MixxxMainWindow are classes that communicate with each other via signals, but neither does MixxxMainWindow own CoreServices (as it did before) nor vice versa.

Hence, we need to construct both first, then connect the signals, then initialize them.

uklotzde

How does this interfere or conflict with #4058?

@Holzhaus @Be-ing @daschuer Please comment and propose how to move on.

Holzhaus · 2021-07-30T18:55:41Z

This PR is ready IMHO. The other PR still has open review comments. I suggest to merge this first.

Holzhaus · 2021-07-30T18:57:45Z

Do you plan to decouple DeckVisuals from QWidgets before or after this PR?

The COs from DeckVisuals are not strictly necessary for CoreServices, just needed in case some controller mappings use them. So we can do it later, or deprecate them if we deem necessary.

daschuer

Yes that will conflict with my branch. Hopefully no problem to solve the conflicts.

LGTM, Thank you.

Be-ing · 2021-07-31T00:06:05Z

So we can do it later

I added a to-do card to the QML project for decoupling DeckVisuals from VSyncThread.

Holzhaus added ui code quality labels Jul 17, 2021

github-actions bot added the build label Jul 17, 2021

Holzhaus force-pushed the mixxxmainwindow-init-refactor branch from 780df49 to 1470d14 Compare July 17, 2021 15:38

Holzhaus force-pushed the mixxxmainwindow-init-refactor branch 2 times, most recently from b1c6215 to 26f2bf1 Compare July 22, 2021 10:46

Holzhaus mentioned this pull request Jul 22, 2021

Fix CoreServices initalisation order #4058

Merged

Holzhaus force-pushed the mixxxmainwindow-init-refactor branch from 26f2bf1 to 9eba44f Compare July 22, 2021 20:32

uklotzde reviewed Jul 22, 2021

View reviewed changes

src/main.cpp Outdated Show resolved Hide resolved

Be-ing requested a review from daschuer July 23, 2021 03:27

Be-ing reviewed Jul 26, 2021

View reviewed changes

src/coreservices.h Show resolved Hide resolved

Holzhaus force-pushed the mixxxmainwindow-init-refactor branch from 3173457 to 30538a2 Compare July 28, 2021 19:32

Holzhaus marked this pull request as draft July 28, 2021 19:32

Holzhaus force-pushed the mixxxmainwindow-init-refactor branch 2 times, most recently from e38b6a0 to cdb40c1 Compare July 28, 2021 22:40

Holzhaus added 4 commits July 30, 2021 11:55

main: Move some generic initialization out of MixxxMainWindow

964d905

The `MixxxMainWindow` class should not be responsible for initializing `CoreServices`. Mixxx should still be able to work when there is no `MixxxMainWindow` (e.g. when we switch to a `QQmlApplication` for QML).

CoreServices: Move settings/keyboard initialization to constructor

5635101

CoreServices: Remove unused method

b1017e0

Holzhaus force-pushed the mixxxmainwindow-init-refactor branch from cdb40c1 to b1017e0 Compare July 30, 2021 09:59

Holzhaus marked this pull request as ready for review July 30, 2021 09:59

Be-ing reviewed Jul 30, 2021

View reviewed changes

uklotzde approved these changes Jul 30, 2021

View reviewed changes

daschuer approved these changes Jul 30, 2021

View reviewed changes

daschuer merged commit 143896d into mixxxdj:main Jul 30, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Start to untangle CoreServices/MixxxMainWindow initialization #4110

Start to untangle CoreServices/MixxxMainWindow initialization #4110

Holzhaus commented Jul 17, 2021

Be-ing commented Jul 17, 2021

Holzhaus commented Jul 18, 2021

Holzhaus commented Jul 19, 2021

coveralls commented Jul 22, 2021 •

edited

Loading

Be-ing commented Jul 23, 2021

Be-ing commented Jul 23, 2021

Be-ing commented Jul 23, 2021

uklotzde commented Jul 23, 2021

Holzhaus commented Jul 23, 2021

Be-ing commented Jul 23, 2021

Be-ing commented Jul 23, 2021

Holzhaus commented Jul 24, 2021 •

edited

Loading

Be-ing commented Jul 26, 2021

Holzhaus commented Jul 28, 2021

Be-ing commented Jul 28, 2021 •

edited

Loading

Holzhaus commented Jul 28, 2021

Holzhaus commented Jul 30, 2021

Be-ing commented Jul 30, 2021

Be-ing Jul 30, 2021

uklotzde Jul 30, 2021

Holzhaus Jul 30, 2021

uklotzde left a comment

Holzhaus commented Jul 30, 2021

Holzhaus commented Jul 30, 2021

daschuer left a comment

Be-ing commented Jul 31, 2021

Start to untangle CoreServices/MixxxMainWindow initialization #4110

Start to untangle CoreServices/MixxxMainWindow initialization #4110

Conversation

Holzhaus commented Jul 17, 2021

Be-ing commented Jul 17, 2021

Holzhaus commented Jul 18, 2021

Holzhaus commented Jul 19, 2021

coveralls commented Jul 22, 2021 • edited Loading

Pull Request Test Coverage Report for Build 1081813635

💛 - Coveralls

Be-ing commented Jul 23, 2021

Be-ing commented Jul 23, 2021

Be-ing commented Jul 23, 2021

uklotzde commented Jul 23, 2021

Holzhaus commented Jul 23, 2021

Be-ing commented Jul 23, 2021

Be-ing commented Jul 23, 2021

Holzhaus commented Jul 24, 2021 • edited Loading

Be-ing commented Jul 26, 2021

Holzhaus commented Jul 28, 2021

Be-ing commented Jul 28, 2021 • edited Loading

Holzhaus commented Jul 28, 2021

Holzhaus commented Jul 30, 2021

Be-ing commented Jul 30, 2021

Be-ing Jul 30, 2021

Choose a reason for hiding this comment

uklotzde Jul 30, 2021

Choose a reason for hiding this comment

Holzhaus Jul 30, 2021

Choose a reason for hiding this comment

uklotzde left a comment

Choose a reason for hiding this comment

Holzhaus commented Jul 30, 2021

Holzhaus commented Jul 30, 2021

daschuer left a comment

Choose a reason for hiding this comment

Be-ing commented Jul 31, 2021

coveralls commented Jul 22, 2021 •

edited

Loading

Holzhaus commented Jul 24, 2021 •

edited

Loading

Be-ing commented Jul 28, 2021 •

edited

Loading