introduce CoreServices to decouple backend from QWidgets #3446

Be-ing · 2020-12-14T18:46:28Z

This is a prerequisite for using a QQuickWindow (or QOpenGLWindow) as Mixxx's GUI.

Inspired by #941.

TODO:

cleanup sloppy usage of std::shared_ptr::get

Be-ing · 2020-12-14T19:00:10Z

src/coreservices.cpp

+    // Create effect backends. We do this after creating EngineMaster to allow
+    // effect backends to refer to controls that are produced by the engine.
+    BuiltInBackend* pBuiltInBackend = new BuiltInBackend(m_pEffectsManager.get());
+    m_pEffectsManager->addEffectsBackend(pBuiltInBackend);
+#ifdef __LILV__
+    m_pLV2Backend = new LV2Backend(m_pEffectsManager.get());
+    // EffectsManager takes ownership
+    m_pEffectsManager->addEffectsBackend(m_pLV2Backend);
+#else
+    m_pLV2Backend = nullptr;
+#endif


Note that this mess is cleaned up in #2618

Be-ing · 2020-12-14T20:03:34Z

src/mixxx.cpp

@@ -1489,15 +932,19 @@ void MixxxMainWindow::rebootMixxxView() {
 }

 bool MixxxMainWindow::loadConfiguredSkin() {
+    // TODO: use std::shared_ptr throughout skin widgets instead of these hacky get() calls


This would require changes in tons of files throughout the GUI code. Let's leave it for another PR.

Be-ing · 2020-12-14T20:04:19Z

Please review and merge quickly. This changes lots of files and will develop merge conflicts quickly.

uklotzde

How about using typedefs for all thosee pointer types instead of scattering std::shared_ptr everywhere in the code?

src/coreservices.h

uklotzde · 2020-12-14T20:41:19Z

Thank you for taking over! I agree that we should reduce the changes in this initial step to a bar minimum. Just split the classes and hand out managed pointers.

Be-ing · 2020-12-14T20:42:05Z

How about using typedefs for all thosee pointer types instead of scattering std::shared_ptr everywhere in the code?

I'm not really a fan of this pattern. If the number of characters in the type name are a concern, I'd rather use using namespace std than typedef everything.

uklotzde · 2020-12-14T20:43:47Z

How about using typedefs for all thosee pointer types instead of scattering std::shared_ptr everywhere in the code?

I'm not really a fan of this pattern. If the number of characters in the type name are a concern, I'd rather use using namespace std than typedef everything.

Ok, I also don't have a strong opinion about this. std::shared_ptr<ClassName> is transparent and unambiguous.

daschuer

Some comments.

daschuer · 2020-12-14T21:11:02Z

src/preferences/dialog/dlgpreferences.h

 #include "preferences/dlgpreferencepage.h"
 #include "preferences/settingsmanager.h"
+#include "preferences/usersettings.h"


Forward declaration of
class DlgPrefController;
below is not used.

Please don't ask me to change code I did not touch. I am keeping this PR as small as possible to get it merged ASAP.

OK. I don't mind. But it is only a matter of removing a line.

src/preferences/dialog/dlgpreferences.cpp

src/coreservices.h

daschuer · 2020-12-14T21:53:28Z

src/mixxx.cpp

-
-    for (int i = 0; i < kAuxiliaryCount; ++i) {
-        m_pPlayerManager->addAuxiliary();
+    for (const auto& group : m_pCoreServices->getPlayerManager()->getVisualPlayerGroups()) {


We should at least assert if these rows of pointers is not null.

I don't understand what you mean. This is a QStringList that is being iterated.

Can you replace auto with QString to make that obvious?

Good Idea.

I meant original:
DEBUG_ASSERT(m_pCoreServices && m_pCoreServices->getPlayerManager())

~~Please don't suggest to combine multiple asserts with &&. Use separate statements instead.~~ Ok, maybe a special case here. Still not recommended.

okay
6fee01f

Asserting that m_pCoreServices isn't null would be pointless. The application would have crashed before that if it was.

Yes, so the assert can be placed at the very beginning of the function.

That seems superfluous, but okay, done:
2b1eb59

Be-ing · 2020-12-14T22:28:20Z

I don't know how to deal with this clazy error:

Error: /home/runner/work/mixxx/mixxx/src/mixxx.cpp:123:5: error: c++11 range-loop might detach Qt container (QStringList) [-Wclazy-range-loop]
    for (const auto& group : m_pCoreServices->getPlayerManager()->getVisualPlayerGroups()) {
    ^

Neither std::as_const nor qAsConst work with QStringList

uklotzde · 2020-12-14T22:32:32Z

I don't know how to deal with this clazy error:

Error: /home/runner/work/mixxx/mixxx/src/mixxx.cpp:123:5: error: c++11 range-loop might detach Qt container (QStringList) [-Wclazy-range-loop]
    for (const auto& group : m_pCoreServices->getPlayerManager()->getVisualPlayerGroups()) {
    ^

Neither std::as_const nor qAsConst work with QStringList

I remember that Jan introduced temporary variables for these cases.

Be-ing · 2020-12-14T23:34:26Z

CI passed. Merge?

uklotzde · 2020-12-14T23:44:35Z

LGTM

Be-ing · 2020-12-14T23:45:24Z

Thanks everyone for being quick with this review so it actually got merged without building up tons of conflicts.

src/mixxx.cpp

uklotzde · 2020-12-16T16:24:16Z

Another bug appeared: Tool tips are now enabled on every restart. Anyone else experiences that?

jospezial · 2021-01-03T17:57:36Z

src/coreservices.h

+    std::shared_ptr<RecordingManager> getRecordingManager() const {
+        return m_pRecordingManager;
+    }
+


I think it needs #ifdef __BROADCAST__
with -DBROADCAST=off:
In file included from /var/tmp/portage/media-sound/mixxx-9999/work/mixxx-9999/src/mixxx.h:8,
from /var/tmp/portage/media-sound/mixxx-9999/work/mixxx-9999/src/mixxx.cpp:1:
/var/tmp/portage/media-sound/mixxx-9999/work/mixxx-9999/src/coreservices.h: In member function ‘std::shared_ptr mixxx::CoreServices::getBroadcastManager() const’:
/var/tmp/portage/media-sound/mixxx-9999/work/mixxx-9999/src/coreservices.h:64:16: error: ‘m_pBroadcastManager’ was not declared in this scope; did you mean ‘getBroadcastManager’?
64 | return m_pBroadcastManager;
| ^~~~~~~~~~~~~~~~~~~
| getBroadcastManager

Yes. Right, I will take care.

ywwg · 2021-02-20T03:53:16Z

It looks like this PR is the source of a bunch of regressions, is there a plan to fix them?

Be-ing · 2021-02-20T03:58:59Z

What are they?

ronso0 · 2021-02-20T10:39:17Z

tooltips visibility & vinyl lead-in time not saved, a few skin COs not available for controllers

The settings restoration is currently done in the preferences dialog. This was dropped accidentally with the introduction of coreservices in mixxxdj#3446. Strictly speaking it would be cleaner to not rely on a temporary widget here. For now this enures modplug tracker files can be loaded and play correctly after application startup.

introduce CoreServices to decouple backend from QWidgets

github-actions bot added build code quality labels Dec 14, 2020

Be-ing commented Dec 14, 2020

View reviewed changes

github-actions bot added the ui label Dec 14, 2020

Be-ing removed build ui labels Dec 14, 2020

Be-ing added 2 commits December 14, 2020 14:02

introduce CoreServices to decouple backend from QWidgets

f754682

use std::shared_ptr for managers in preferences

f90e4ec

Be-ing force-pushed the init_refactor branch from 54a95da to f90e4ec Compare December 14, 2020 20:02

github-actions bot added build ui labels Dec 14, 2020

Be-ing commented Dec 14, 2020

View reviewed changes

uklotzde reviewed Dec 14, 2020

View reviewed changes

src/coreservices.h Outdated Show resolved Hide resolved

Be-ing force-pushed the init_refactor branch from 8141956 to b1bb003 Compare December 14, 2020 20:43

CoreServices: move constants to anonymous namespace

60c7a53

Be-ing force-pushed the init_refactor branch 4 times, most recently from 6875322 to 528053c Compare December 14, 2020 21:26

fix Ubuntu 18.04 build

af3c656

Be-ing force-pushed the init_refactor branch from 528053c to af3c656 Compare December 14, 2020 21:43

daschuer requested changes Dec 14, 2020

View reviewed changes

Be-ing added 3 commits December 14, 2020 16:04

fix build without lilv

9faf5ad

DlgPreferences: rename controllers -> pControllerManager

08aab17

CoreServices: move #includes above forward declarations

06f3c28

auto -> QString

6fee01f

Be-ing added 3 commits December 14, 2020 16:33

add debug assertion

b0b823f

make clazy happy

bd8f0f4

add more debug assertions

2b1eb59

daschuer approved these changes Dec 14, 2020

View reviewed changes

uklotzde merged commit 3c9435d into mixxxdj:main Dec 14, 2020

Be-ing deleted the init_refactor branch December 14, 2020 23:45

Be-ing mentioned this pull request Dec 15, 2020

Refactor MixxxMainWindow. #941

Closed

Holzhaus reviewed Dec 15, 2020

View reviewed changes

src/mixxx.cpp Show resolved Hide resolved

jospezial reviewed Jan 3, 2021

View reviewed changes

uklotzde mentioned this pull request Feb 28, 2021

effect chain preset saving/loading, custom effect defaults, and parameter hiding/rearrangement #2618

Closed

15 tasks

ywwg mentioned this pull request May 13, 2021

Fix separation of MixxxMainWindow and CoreServices #3854

Merged

daschuer mentioned this pull request Oct 14, 2021

Fix switching from Shade to other skins #4421

Merged

snue mentioned this pull request May 2, 2022

Fix loading of modplug files #4736

Merged

This was referenced Aug 23, 2022

control object creation order broken in main branch: cos created by skin not available to controller scripts #10315

Closed

mixxx forgetting tooltip setting in main branch #10316

Closed

napaalm pushed a commit to napaalm/mixxx that referenced this pull request Mar 24, 2023

Merge pull request mixxxdj#3446 from Be-ing/init_refactor

d7bf71e

introduce CoreServices to decouple backend from QWidgets

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

introduce CoreServices to decouple backend from QWidgets #3446

introduce CoreServices to decouple backend from QWidgets #3446

Be-ing commented Dec 14, 2020 •

edited

Loading

Be-ing Dec 14, 2020

Be-ing Dec 14, 2020

Be-ing commented Dec 14, 2020

uklotzde left a comment

uklotzde commented Dec 14, 2020

Be-ing commented Dec 14, 2020

uklotzde commented Dec 14, 2020

daschuer left a comment

daschuer Dec 14, 2020

Be-ing Dec 14, 2020

daschuer Dec 14, 2020

daschuer Dec 14, 2020

Be-ing Dec 14, 2020

Holzhaus Dec 14, 2020

daschuer Dec 14, 2020

uklotzde Dec 14, 2020 •

edited

Loading

Be-ing Dec 14, 2020

Be-ing Dec 14, 2020

daschuer Dec 14, 2020

Be-ing Dec 14, 2020

Be-ing commented Dec 14, 2020

uklotzde commented Dec 14, 2020

Be-ing commented Dec 14, 2020

uklotzde commented Dec 14, 2020

Be-ing commented Dec 14, 2020

uklotzde commented Dec 16, 2020

jospezial Jan 3, 2021 •

edited

Loading

daschuer Jan 3, 2021

daschuer Jan 3, 2021

ywwg commented Feb 20, 2021

Be-ing commented Feb 20, 2021

ronso0 commented Feb 20, 2021

introduce CoreServices to decouple backend from QWidgets #3446

introduce CoreServices to decouple backend from QWidgets #3446

Conversation

Be-ing commented Dec 14, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Be-ing commented Dec 14, 2020

uklotzde left a comment

Choose a reason for hiding this comment

uklotzde commented Dec 14, 2020

Be-ing commented Dec 14, 2020

uklotzde commented Dec 14, 2020

daschuer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uklotzde Dec 14, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Be-ing commented Dec 14, 2020

uklotzde commented Dec 14, 2020

Be-ing commented Dec 14, 2020

uklotzde commented Dec 14, 2020

Be-ing commented Dec 14, 2020

uklotzde commented Dec 16, 2020

jospezial Jan 3, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ywwg commented Feb 20, 2021

Be-ing commented Feb 20, 2021

ronso0 commented Feb 20, 2021

Be-ing commented Dec 14, 2020 •

edited

Loading

uklotzde Dec 14, 2020 •

edited

Loading

jospezial Jan 3, 2021 •

edited

Loading