Refactor MixxxMainWindow. #941

rryan · 2016-05-04T21:18:55Z

Splits MixxxMainWindow into two pieces: CoreServices and WMainWindow.

This will allow us to iterate on new frontends that do not make use of
QtWidgets / QtGui (such as an Android or iOS port) and generally
enforces a cleaner separation between Mixxx services (provided by
individual manager classes) and the Mixxx UI.

This is also a step towards refactoring the bootup process (which
prompted us to add a loading" launch screen because it's so
slow). With a cleaner concept of "phases" of bootup (construction,
initialize, configure, finalize, destruction) we can start the UI
after construction/initialization and delay the longer tasks
(e.g. connecting audio devices and controllers) or push them onto a
different thread.

Ideally all manager classes and Core will remain buildable with the
QtWidgets module disabled.

rryan · 2016-05-05T01:47:29Z

This is step 2 (step 1 was WMainMenuBar) on a long-term refactor journey for me. I wrote down some of the goals in this design doc:
http://mixxx.org/wiki/doku.php/mixxx_init_refactor

daschuer · 2016-05-05T19:05:02Z

I have just read the linked document and I like the underlying ideas. The multi-threading approach will help to utilize the n-cores of recent CPUs.

However, I think it is not a good idea to present the GUI before Mixxx is ready to used.
This is one of the annoyances (old) Windows compared to Linux, Windows predicts to be ready, but you cannot use it util the HDD has stopped seeking.

An other general thought is: Multitheading increased the complexity significant. If you aim to reduce the complexity this might be a bad deal. But we will see, when it comes to the implementation.

daschuer · 2016-05-05T19:09:41Z

src/main.cpp

-        qDebug() << "Running Mixxx";
-        result = a.exec();
+    mixxx::Core::initializeStaticServices();
+    {


// scope for core

Done -- though this seems a bit obvious, no?

daschuer · 2016-05-05T19:44:03Z

OK, I assume there where nothing changed in the business logic, only moving functions around, right? I have only added some minor nits.

The other main issue is the release schedule. We have already published a release date for Mixxx 2.1
for February 2016. The current master version contains a lot of improvements worth a soon release without any further additions. So we should brake out a 2.1 branch and announce the beta phase now.

( The alpha seams to be blocked by the broken windows build server though
@Pegasus-RPG: how is your progress?
I have played with https://www.appveyor.com/ , will it be an alternative? )

These planed refactorings will not have a user impact, but have some degree of regression risk.
So the should go to a master. The same is true for the GSOC projects.

rryan · 2016-05-06T17:29:27Z

OK, I assume there where nothing changed in the business logic, only moving functions around, right? I have only added some minor nits.

Yep, that's right.

rryan · 2016-05-06T18:06:59Z

The other main issue is the release schedule. We have already published a release date for Mixxx 2.1
for February 2016. The current master version contains a lot of improvements worth a soon release without any further additions. So we should brake out a 2.1 branch and announce the beta phase now.

I agree this shouldn't go in 2.1.

Be-ing · 2017-12-27T22:38:10Z

@rryan are you interested in picking this up again? Now would be the time to do it. Our startup time has gotten really bad.

Be-ing · 2018-04-18T04:07:32Z

This pull request has not been updated in years so I am closing it to reduce the clutter of open PRs. If you solve the merge conflicts and would like to have it merged in the future, please reopen it. However, considering how many merge conflicts there are, it may be easier to start over from the current master branch.

rryan · 2018-04-18T04:11:21Z

@rryan are you interested in picking this up again? Now would be the time to do it. Our startup time has gotten really bad.

Yea, I am. I'll re-open when/if I get to it though.

rryan · 2018-09-17T22:28:42Z

Re-based against master, tests are passing on macOS and no shared_ptr leaks occur.

Be-ing · 2018-09-17T22:31:38Z

Thanks for picking up work on this again!

Be-ing · 2018-09-18T00:08:27Z

I think there will be a few small merge conflicts between this and #1705, but it looks like they will be easy to resolve.

Splits MixxxMainWindow into two pieces: CoreServices and WMainWindow. This will allow us to iterate on new frontends that do not make use of QtWidgets / QtGui (such as an Android or iOS port) and generally enforces a cleaner separation between Mixxx services (provided by individual manager classes) and the Mixxx UI. This is also a step towards refactoring the bootup process (which prompted us to add a loading" launch screen because it's so slow). With a cleaner concept of "phases" of bootup (construction, initialize, configure, finalize, destruction) we can start the UI after construction/initialization and delay the longer tasks (e.g. connecting audio devices and controllers) or push them onto a different thread. Ideally all manager classes and CoreServices will remain buildable with the QtWidgets module disabled.

This adds safety against use-after-free to the interaction between EngineMaster, SoundManager, EngineSideChain, VinylControlProcessor, EngineDeck, EngineMicrophone, and EngineAux. Note that this effectively reverts db3b503. The reason this was done initially was to avoid coupling between SoundManager and EngineMaster. Ideally neither of them should know about each other, and they could communicate via interfaces alone. They are currently coupled (SoundManager has a reference to EngineMaster) because of the callback, but I've always wanted to remove that so they are totally independent. PlayerManager (which could have been named EngineManager) as I originally imagined it, is where EngineMaster and SoundManager are bound together, since PlayerManager has a reference to both; for example, when a deck is added, it's added to EngineMaster and registered with SoundManager in PlayerManager. That's why the logic to register EngineMaster's non-EngineChannel AudioInputs/AudioOutputs originally went in PlayerManager. Thinking of PlayerManager as EngineManager, this kind of makes sense. In this PR, the logic can no longer live in EngineMaster for the simple reason that EngineMaster does not have a shared_ptr to itself, and so it can't call SoundManager::registerXXX(..., this) anymore. We could plumb a shared_ptr into EngineMaster, but that creates a reference leak.

…ence.

rryan · 2018-10-02T03:28:31Z

I think this is ready to go. It's a first step toward the plan for an init refactor that only moves code around without intentionally changing any logic. The ordering of some steps in bootup and shutdown have changed slightly, but it should only be incidental. Any comments?

rryan · 2018-10-02T03:29:09Z

I think there will be a few small merge conflicts between this and #1705, but it looks like they will be easy to resolve.

Yea, I tried to make this minimally invasive w.r.t. any changes made to the effects system or library.

Be-ing

Thanks for starting to clean this up!

Be-ing · 2018-10-08T01:10:22Z

src/main.cpp

+void clearHelper(std::shared_ptr<T>& ref_ptr, const char* name) {
+    std::weak_ptr<T> weak(ref_ptr);
+    ref_ptr.reset();
+    if (auto shared = weak.lock()) {


DEBUG_ASSERT?

Be-ing · 2018-10-08T01:21:30Z

src/widget/wmainwindow.cpp

@@ -778,7 +397,7 @@ void MixxxMainWindow::initializeKeyboard() {
    m_pKeyboard = new KeyboardEventFilter(keyboardShortcutsEnabled ? m_pKbdConfig : m_pKbdConfigEmpty);
 }

-QDialog::DialogCode MixxxMainWindow::soundDeviceErrorDlg(
+QDialog::DialogCode WMainWindow::soundDeviceErrorDlg(


This feels out of place in WMainWindow. Should it be moved into SoundManager?

Be-ing · 2018-10-08T01:25:16Z

src/coreservices.h

+namespace mixxx {
+
+class CoreServices : public QObject {
+    Q_OBJECT


Does this need to be a Q_OBJECT?

Be-ing · 2018-10-08T01:35:14Z

src/coreservices.cpp

+
+    m_pGuiTick = std::make_shared<GuiTick>();
+
+    // TODO(rryan): Move this back into EngineMaster. The only reason it got


I'm not sure I agree this should be moved back to EngineMaster. There some places throughout the code that rely on parsing strings to identify a channel. I think it would be better to make use of ChannelHandles more widely throughout Mixxx than confine them to EngineMaster.

Be-ing · 2018-10-08T01:41:31Z

src/coreservices.cpp

+    m_pEffectsManager = std::make_shared<EffectsManager>(this, pConfig, m_pChannelHandleFactory);
+
+    // Starting the master (mixing of the channels and effects):
+    m_pEngine = std::make_shared<EngineMaster>(


Should we drop the "Master" from "EngineMaster"? Or maybe call it "EngineMixer"?

Be-ing · 2018-10-08T01:45:28Z

src/coreservices.cpp

+            std::static_pointer_cast<GlobalTrackCacheSaver>(m_pLibrary));
+
+    // TODO(rryan): de-singleton
+    // TODO(rryan): Where does this belong?


LibraryManager?

Be-ing · 2018-10-08T01:58:31Z

src/coreservices.cpp

+    m_pPlayerManager->addSampler();
+    m_pPlayerManager->addPreviewDeck();
+
+    // TODO(rryan): Why is this here?


IIRC this has to be called after creating mics, aux channels, and samplers in case those are routed to any saved chains. (I could be wrong about that.)

Be-ing · 2018-10-08T02:40:07Z

src/main.cpp

+        auto core = std::make_shared<mixxx::CoreServices>(nullptr, app, args.getSettingsPath());
+        core->initialize();
+        {  // Scope for WMainWindow.
+            WMainWindow window(app, core);


This introduces a regression. No GUI is shown for about 3 seconds, then when the main window is first shown, the loading progress bar is already over half way.

Ping. I propose moving the construction of services in the CoreServices class from the constructor to a new method that would get called by WMainWindow. Then have CoreServices emit a signal after each service is started that triggers a progress bar update in WMainWindow.

Be-ing · 2018-11-21T00:14:32Z

I would like to merge this relatively soon before more merge conflicts develop.

Be-ing · 2018-12-27T23:32:53Z

@rryan will you have time to continue this soon? If not I might pick it up.

rryan · 2018-12-28T22:02:05Z

@rryan will you have time to continue this soon? If not I might pick it up.

Yea, it's on my list after #926.

github-actions · 2020-10-20T00:22:37Z

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

Be-ing · 2020-12-15T15:31:21Z

Superceeded by #3446

rryan force-pushed the core branch from bdae136 to 137eed0 Compare May 4, 2016 21:30

daschuer reviewed May 5, 2016
View reviewed changes

This was referenced Dec 11, 2016

Multithreaded analysis #1069

Closed

Highlight feature for WidgetGroup #1068

Merged

rryan force-pushed the core branch from 4ff946b to 2e5002c Compare December 31, 2016 04:27

rryan mentioned this pull request Jan 22, 2017

Introduce "parented_ptr" wrapper class #1142

Merged

rryan force-pushed the core branch from 2e5002c to 471cce6 Compare January 28, 2017 04:50

Be-ing mentioned this pull request Jan 31, 2017

Ability to show/hide menu bar #927

Closed

rryan mentioned this pull request Feb 5, 2017

CrateStorage: Prototype for future DB access #1099

Merged

Be-ing closed this Apr 18, 2018

rryan referenced this pull request Sep 16, 2018

Fix order of destruction on shutdown

cf43b08

rryan reopened this Sep 17, 2018

rryan force-pushed the core branch from 471cce6 to f5c0bc9 Compare September 17, 2018 22:28

rryan force-pushed the core branch 2 times, most recently from 19d9e56 to e7bebe3 Compare September 17, 2018 23:51

mixxxdj deleted a comment Sep 18, 2018

rryan force-pushed the core branch from 3be0fa9 to 5965430 Compare September 24, 2018 02:39

rryan added 5 commits September 30, 2018 13:21

Listen to [Master],num_decks in AutoDJProcessor.

142255e

Use a shared_ptr for EffectsManager in EngineChannel.

1b78c85

Fix some missing includes. Not sure why macOS builds without these.

8ac7e3e

rryan force-pushed the core branch from 5965430 to 8ac7e3e Compare September 30, 2018 20:21

rryan added 3 commits October 1, 2018 11:57

Don't use deleteLater on AnalyzerQueue in AnalysisFeature.

e3a99d8

Move reference check of DbConnectionPool. PlayerManager holds a refer…

859bb35

…ence.

Remove QObject parent from MixxxDb.

b13dae9

Be-ing suggested changes Oct 8, 2018

View reviewed changes

rryan added this to the 2.3.0 milestone Oct 16, 2018

rryan mentioned this pull request Oct 25, 2018

Render WKnob, WKnobComposed and WSliderComposed using the GuiTick timer. Bug #1793015 #1809

Merged

Be-ing mentioned this pull request Dec 28, 2018

Analysis Revamp #1624

Merged

Be-ing modified the milestones: 2.3.0, 2.4.0 Feb 7, 2019

Be-ing modified the milestones: 2.4.0, stalled Apr 6, 2020

github-actions bot added the stale Stale issues that haven't been updated for a long time. label Oct 20, 2020

Be-ing changed the base branch from master to main October 23, 2020 23:54

Be-ing mentioned this pull request Dec 14, 2020

introduce CoreServices to decouple backend from QWidgets #3446

Merged

1 task

Be-ing closed this Dec 15, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor MixxxMainWindow. #941

Refactor MixxxMainWindow. #941

rryan commented May 4, 2016 •

edited

Loading

rryan commented May 5, 2016

daschuer commented May 5, 2016

daschuer May 5, 2016

rryan May 6, 2016

daschuer commented May 5, 2016

rryan commented May 6, 2016

rryan commented May 6, 2016

Be-ing commented Dec 27, 2017

Be-ing commented Apr 18, 2018

rryan commented Apr 18, 2018

rryan commented Sep 17, 2018

Be-ing commented Sep 17, 2018

Be-ing commented Sep 18, 2018

rryan commented Oct 2, 2018

rryan commented Oct 2, 2018

Be-ing left a comment

Be-ing Oct 8, 2018

Be-ing Oct 8, 2018

Be-ing Oct 8, 2018

Be-ing Oct 8, 2018

Be-ing Oct 8, 2018

Be-ing Oct 8, 2018

Be-ing Oct 8, 2018

Be-ing Oct 8, 2018

Be-ing Nov 21, 2018

Be-ing commented Nov 21, 2018

Be-ing commented Dec 27, 2018

rryan commented Dec 28, 2018

github-actions bot commented Oct 20, 2020

Be-ing commented Dec 15, 2020


		m_pGuiTick = std::make_shared<GuiTick>();

		// TODO(rryan): Move this back into EngineMaster. The only reason it got

Refactor MixxxMainWindow. #941

Refactor MixxxMainWindow. #941

Conversation

rryan commented May 4, 2016 • edited Loading

rryan commented May 5, 2016

daschuer commented May 5, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daschuer commented May 5, 2016

rryan commented May 6, 2016

rryan commented May 6, 2016

Be-ing commented Dec 27, 2017

Be-ing commented Apr 18, 2018

rryan commented Apr 18, 2018

rryan commented Sep 17, 2018

Be-ing commented Sep 17, 2018

Be-ing commented Sep 18, 2018

rryan commented Oct 2, 2018

rryan commented Oct 2, 2018

Be-ing 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Be-ing commented Nov 21, 2018

Be-ing commented Dec 27, 2018

rryan commented Dec 28, 2018

github-actions bot commented Oct 20, 2020

Be-ing commented Dec 15, 2020

rryan commented May 4, 2016 •

edited

Loading