Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Effects: read effect data for decks added after initial setup #12282

Merged
merged 1 commit into from
Nov 14, 2023

Conversation

ronso0
Copy link
Member

@ronso0 ronso0 commented Nov 9, 2023

Closes #12277

EffectsManager::readEffectsXml is responsible for initialising/restoring Quick Effects, though it only cares about existing decks when it's called (once) via CoreServices::initialize -> EffectsManager::setup. The deck count is still the initial 2 then, the default count PlayerManager is initialized with.
If any decks are added later on, e.g. when the default (4-deck) skin LateNight is loaded, effects.xml is not read again and the Quick Effect slots remain empty.
On next start it uses the deck_count attribute from soundconfig.xml.

IMO only EffectsManager should be responsible for effect defaults, not the Preferences.

Fixed by creating a separate, stripped down version of readEffectsXml that extracts only the Quick Effect for one deck, and call that each time a deck is added after the initial setup() call.

@ronso0 ronso0 force-pushed the effects.xml-decks-after-init branch from 74cb0d7 to b6e6d00 Compare November 11, 2023 00:24
@ronso0 ronso0 marked this pull request as ready for review November 11, 2023 00:24
@@ -202,6 +215,25 @@ void EffectsManager::readEffectsXml() {
m_pVisibleEffectsList->readEffectsXml(doc, m_pBackendManager);
}

void EffectsManager::readEffectsXmlSingleDeck(const QString& deckGroup) {
QDir settingsPath(m_pConfig->getSettingsPath());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Normally we prefer initialization with = QDir settingsPath = m_pConfig->getSettingsPath();
To have a clear visible distinguishing from a function call.

Recently we have discussed to prefer initialization lists like:
QDir settingsPath{m_pConfig->getSettingsPath()};

This is just a hint, because that never made the way into our style guides.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ackknowldeged.
It's copy/paste from readEffectsXml(), and actually it's mixed in the vaious effect managers already:
QString xyPath(m_pConfig->getSettingsPath() + ...);
QDir xyDir(xyPath);
vs
QFile xyFile(m_pConfig->getSettingsPath() + ...);

Copy link
Member

@daschuer daschuer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just code style hints.

Comment on lines +804 to +805
for (int i = 0; i < quickEffectNodeList.count(); ++i) {
QDomElement presetNameElement = quickEffectNodeList.at(i).toElement();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can become a range based loop, to get rid of i.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's QList::iterator, but none for QDomeNodeList.

Copy link
Member

@daschuer daschuer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok than let's merge. Thank you. LGTM.

@daschuer daschuer merged commit 98e8ac9 into mixxxdj:2.4 Nov 14, 2023
12 checks passed
@ronso0 ronso0 deleted the effects.xml-decks-after-init branch November 14, 2023 10:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

no Quick Effect loaded for decks 3 & 4 by default
2 participants