From 629ba0362b8f668ef0d3cf4ae85fe78a6d4ce40f Mon Sep 17 00:00:00 2001 From: Michael Gregorius Date: Mon, 15 Jan 2024 08:37:11 +0100 Subject: [PATCH] Fix mixer channel updates on solo/mute (#7055) * Fix mixer channel updates on solo/mute Fix the update of the mixer channels whenever a channel is soloed or muted. The solution is rather "brutal" as it updates all mixer channel views when one of the models changes. Introduce private helper method `MixerView::updateAllMixerChannels`. It calls the `update` method on each `MixerChannelView` so that they can get repainted. Call `updateAllMixerChannels` at the end of the existing method `toggledSolo`. Introduce a new method `MixerView::toggledMute` which is called whenever the mute model of a channel changes. It also updates all mixer channels. Fixes #7054. * Improve mixer channel update for mute events Improve the mixer channel update for mute events by not delegating to `MixerView` like for the solo case. Instead the `MixerChannelView` now has its own `toggledMute` slot which simply updates the widget on changes to the mute model. Also fix `MixerChannelView::setChannelIndex` by disconnecting from the signals of the previous mixer channel and connecting to the signals for the new one. Remove `toggledMute` from `MixerView`. The solo implementation is kept as is because it also triggers changes to the core model. So the chain seems to be: * Solo button clicked in mixer channel view * Button changes solo model * Solo model signals to mixer view which was connected via the mixer channel view * Mixer view performs changes in the other core models * All mixer channels are updated. Are better chain would first update the core models and then update the GUI from the changes: * Solo button clicked in mixer channel view * Button changes solo model * Solo model signals to core mixer, i.e. not a view! * Mixer view performs changes in the other core models * Changed models emit signal to GUI elements * Revert "Improve mixer channel update for mute events" This reverts commit ede65963ea1d6131944679a131641c18b97bce0b. * Add comment After the revert done with commit the code is more consistent again but not in a good way. Hence a comment is added which indicates that an inprovement is needed. * Abstract mixer retrieval Abstract the retrieval of the mixer behind the new method `getMixer`. This is done in preparation for some dependency injection so that the `MixerView` does not have to ask the `Engine` for the mixer but gets it injected, a.k.a. the "Hollywood principle": "Don't call us, we'll call you." It's called `getMixer` and not just mixer because it allows for locale variables to be called `mixer` without confusing it with the method. * Let MixerView connect directly to models Let the `MixerView` connect directly to the solo and mute models it is connected with. Remove the connections that are made in `MixerChannelView` which acted as a proxy which only complicated things. Add `connectToSoloAndMute` which connects the `MixerView` to the solo and mute models of a given channel. Call it whenever a new channel is created. Add `disconnectFromSoloAndMute` which disconnects the `MixerView` from the solo and mute models of a given channel. Call it when a channel is deleted. * Code cleanup Cleanup code related to the creation of the master channel view. * Inject the Mixer into the MixerView Inject the `Mixer` into the `MixerView` via the constructor. This makes it more explicit that the `Mixer` is the model of the view. It also implements the "Dependency Inversion Principle" in that the `MIxerView` does not have to ask the `Engine` for the `Mixer` anymore. The current changes should be safe in that the `Mixer` instance is static and does not change. * Fix connections on song load Disconnect and reconnect in `MixerView::refreshDisplay` which is called when a song is loaded. --- include/MixerView.h | 18 +++++++- src/gui/GuiApplication.cpp | 2 +- src/gui/MixerChannelView.cpp | 3 +- src/gui/MixerView.cpp | 88 ++++++++++++++++++++++++++++-------- 4 files changed, 87 insertions(+), 24 deletions(-) diff --git a/include/MixerView.h b/include/MixerView.h index a47786481..81287bc54 100644 --- a/include/MixerView.h +++ b/include/MixerView.h @@ -38,6 +38,11 @@ #include "embed.h" #include "EffectRackView.h" +namespace lmms +{ + class Mixer; +} + namespace lmms::gui { class LMMS_EXPORT MixerView : public QWidget, public ModelView, @@ -45,7 +50,7 @@ class LMMS_EXPORT MixerView : public QWidget, public ModelView, { Q_OBJECT public: - MixerView(); + MixerView(Mixer* mixer); void keyPressEvent(QKeyEvent* e) override; void saveSettings(QDomDocument& doc, QDomElement& domElement) override; @@ -97,7 +102,17 @@ protected: private slots: void updateFaders(); + // TODO This should be improved. Currently the solo and mute models are connected via + // the MixerChannelView's constructor with the MixerView. It would already be an improvement + // if the MixerView connected itself to each new MixerChannel that it creates/handles. void toggledSolo(); + void toggledMute(); + +private: + Mixer* getMixer() const; + void updateAllMixerChannels(); + void connectToSoloAndMute(int channelIndex); + void disconnectFromSoloAndMute(int channelIndex); private: QVector m_mixerChannelViews; @@ -109,6 +124,7 @@ private: QWidget* m_channelAreaWidget; QStackedLayout* m_racksLayout; QWidget* m_racksWidget; + Mixer* m_mixer; void updateMaxChannelSelector(); diff --git a/src/gui/GuiApplication.cpp b/src/gui/GuiApplication.cpp index 3370cbc6e..5c4bdd19a 100644 --- a/src/gui/GuiApplication.cpp +++ b/src/gui/GuiApplication.cpp @@ -148,7 +148,7 @@ GuiApplication::GuiApplication() connect(m_songEditor, SIGNAL(destroyed(QObject*)), this, SLOT(childDestroyed(QObject*))); displayInitProgress(tr("Preparing mixer")); - m_mixerView = new MixerView; + m_mixerView = new MixerView(Engine::mixer()); connect(m_mixerView, SIGNAL(destroyed(QObject*)), this, SLOT(childDestroyed(QObject*))); displayInitProgress(tr("Preparing controller rack")); diff --git a/src/gui/MixerChannelView.cpp b/src/gui/MixerChannelView.cpp index 9ea266238..928255806 100644 --- a/src/gui/MixerChannelView.cpp +++ b/src/gui/MixerChannelView.cpp @@ -110,8 +110,7 @@ namespace lmms::gui m_soloButton->setActiveGraphic(embed::getIconPixmap("led_red")); m_soloButton->setInactiveGraphic(embed::getIconPixmap("led_off")); m_soloButton->setCheckable(true); - m_soloButton->setToolTip(tr("Solo this channel")); - connect(&mixerChannel->m_soloModel, &BoolModel::dataChanged, mixerView, &MixerView::toggledSolo, Qt::DirectConnection); + m_soloButton->setToolTip(tr("Solo this channel")); QVBoxLayout* soloMuteLayout = new QVBoxLayout(); soloMuteLayout->setContentsMargins(0, 0, 0, 0); diff --git a/src/gui/MixerView.cpp b/src/gui/MixerView.cpp index a6ee2989c..93b4e1299 100644 --- a/src/gui/MixerView.cpp +++ b/src/gui/MixerView.cpp @@ -52,10 +52,11 @@ namespace lmms::gui { -MixerView::MixerView() : +MixerView::MixerView(Mixer* mixer) : QWidget(), ModelView(nullptr, this), - SerializingObjectHook() + SerializingObjectHook(), + m_mixer(mixer) { #if QT_VERSION < 0x50C00 // Workaround for a bug in Qt versions below 5.12, @@ -67,8 +68,7 @@ MixerView::MixerView() : using ::operator|; #endif - Mixer * m = Engine::mixer(); - m->setHook(this); + mixer->setHook(this); //QPalette pal = palette(); //pal.setColor(QPalette::Window, QColor(72, 76, 88)); @@ -100,12 +100,13 @@ MixerView::MixerView() : m_racksWidget->setLayout(m_racksLayout); // add master channel - m_mixerChannelViews.resize(m->numChannels()); - m_mixerChannelViews[0] = new MixerChannelView(this, this, 0); + m_mixerChannelViews.resize(mixer->numChannels()); + MixerChannelView * masterView = new MixerChannelView(this, this, 0); + connectToSoloAndMute(0); + m_mixerChannelViews[0] = masterView; m_racksLayout->addWidget(m_mixerChannelViews[0]->m_effectRackView); - MixerChannelView * masterView = m_mixerChannelViews[0]; ml->addWidget(masterView, 0, Qt::AlignTop); auto mixerChannelSize = masterView->sizeHint(); @@ -114,6 +115,7 @@ MixerView::MixerView() : for (int i = 1; i < m_mixerChannelViews.size(); ++i) { m_mixerChannelViews[i] = new MixerChannelView(m_channelAreaWidget, this, i); + connectToSoloAndMute(i); chLayout->addWidget(m_mixerChannelViews[i]); } @@ -175,7 +177,7 @@ MixerView::MixerView() : parentWidget()->move(5, 310); // we want to receive dataChanged-signals in order to update - setModel(m); + setModel(mixer); } @@ -184,10 +186,11 @@ MixerView::MixerView() : int MixerView::addNewChannel() { // add new mixer channel and redraw the form. - Mixer * mix = Engine::mixer(); + Mixer * mix = getMixer(); int newChannelIndex = mix->createChannel(); m_mixerChannelViews.push_back(new MixerChannelView(m_channelAreaWidget, this, newChannelIndex)); + connectToSoloAndMute(newChannelIndex); chLayout->addWidget(m_mixerChannelViews[newChannelIndex]); m_racksLayout->addWidget(m_mixerChannelViews[newChannelIndex]->m_effectRackView); @@ -204,6 +207,9 @@ void MixerView::refreshDisplay() // delete all views and re-add them for (int i = 1; iremoveWidget(mixerChannelView); m_racksLayout->removeWidget(mixerChannelView->m_effectRackView); @@ -213,10 +219,12 @@ void MixerView::refreshDisplay() m_channelAreaWidget->adjustSize(); // re-add the views - m_mixerChannelViews.resize(Engine::mixer()->numChannels()); + m_mixerChannelViews.resize(getMixer()->numChannels()); for (int i = 1; i < m_mixerChannelViews.size(); ++i) { m_mixerChannelViews[i] = new MixerChannelView(m_channelAreaWidget, this, i); + connectToSoloAndMute(i); + chLayout->addWidget(m_mixerChannelViews[i]); m_racksLayout->addWidget(m_mixerChannelViews[i]->m_effectRackView); } @@ -280,10 +288,46 @@ void MixerView::loadSettings(const QDomElement& domElement) void MixerView::toggledSolo() { - Engine::mixer()->toggledSolo(); + getMixer()->toggledSolo(); + + updateAllMixerChannels(); } +void MixerView::toggledMute() +{ + updateAllMixerChannels(); +} + +Mixer* MixerView::getMixer() const +{ + return m_mixer; +} + +void MixerView::updateAllMixerChannels() +{ + for (int i = 0; i < m_mixerChannelViews.size(); ++i) + { + m_mixerChannelViews[i]->update(); + } +} + +void MixerView::connectToSoloAndMute(int channelIndex) +{ + auto * mixerChannel = getMixer()->mixerChannel(channelIndex); + + connect(&mixerChannel->m_muteModel, &BoolModel::dataChanged, this, &MixerView::toggledMute, Qt::DirectConnection); + connect(&mixerChannel->m_soloModel, &BoolModel::dataChanged, this, &MixerView::toggledSolo, Qt::DirectConnection); +} + +void MixerView::disconnectFromSoloAndMute(int channelIndex) +{ + auto * mixerChannel = getMixer()->mixerChannel(channelIndex); + + disconnect(&mixerChannel->m_muteModel, &BoolModel::dataChanged, this, &MixerView::toggledMute); + disconnect(&mixerChannel->m_soloModel, &BoolModel::dataChanged, this, &MixerView::toggledSolo); +} + void MixerView::setCurrentMixerChannel(MixerChannelView* channel) { @@ -301,12 +345,12 @@ void MixerView::setCurrentMixerChannel(MixerChannelView* channel) void MixerView::updateMixerChannel(int index) { - Mixer * mix = Engine::mixer(); + Mixer * mix = getMixer(); // does current channel send to this channel? int selIndex = m_currentMixerChannel->channelIndex(); auto thisLine = m_mixerChannelViews[index]; - thisLine->setToolTip(Engine::mixer()->mixerChannel(index)->m_name); + thisLine->setToolTip(getMixer()->mixerChannel(index)->m_name); FloatModel * sendModel = mix->channelSendModel(selIndex, index); if (sendModel == nullptr) @@ -339,15 +383,19 @@ void MixerView::deleteChannel(int index) return; } + // Disconnect from the solo/mute models of the channel we are about to delete + disconnectFromSoloAndMute(index); + // remember selected line int selLine = m_currentMixerChannel->channelIndex(); + Mixer* mixer = getMixer(); // in case the deleted channel is soloed or the remaining // channels will be left in a muted state - Engine::mixer()->clearChannel(index); + mixer->clearChannel(index); // delete the real channel - Engine::mixer()->deleteChannel(index); + mixer->deleteChannel(index); chLayout->removeWidget(m_mixerChannelViews[index]); m_racksLayout->removeWidget(m_mixerChannelViews[index]); @@ -379,7 +427,7 @@ bool MixerView::confirmRemoval(int index) bool needConfirm = ConfigManager::inst()->value("ui", "mixerchanneldeletionwarning", "1").toInt(); if (!needConfirm) { return true; } - Mixer* mix = Engine::mixer(); + Mixer* mix = getMixer(); if (!mix->isChannelInUse(index)) { @@ -416,7 +464,7 @@ bool MixerView::confirmRemoval(int index) void MixerView::deleteUnusedChannels() { - Mixer* mix = Engine::mixer(); + Mixer* mix = getMixer(); // Check all channels except master, delete those with no incoming sends for (int i = m_mixerChannelViews.size() - 1; i > 0; --i) @@ -435,7 +483,7 @@ void MixerView::moveChannelLeft(int index, int focusIndex) // can't move master or first channel left or last channel right if (index <= 1 || index >= m_mixerChannelViews.size()) return; - Mixer *m = Engine::mixer(); + Mixer *m = getMixer(); // Move instruments channels m->moveChannelLeft(index); @@ -542,7 +590,7 @@ void MixerView::setCurrentMixerChannel(int channel) void MixerView::clear() { - Engine::mixer()->clear(); + getMixer()->clear(); refreshDisplay(); } @@ -552,7 +600,7 @@ void MixerView::clear() void MixerView::updateFaders() { - Mixer * m = Engine::mixer(); + Mixer * m = getMixer(); // apply master gain m->mixerChannel(0)->m_peakLeft *= Engine::audioEngine()->masterGain();