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

Preferences: show 'real' xfader configuration #14124

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ronso0
Copy link
Member

@ronso0 ronso0 commented Jan 6, 2025

This makes the Mixer preferences load the xfader configuration

  • from config on first load (if all xfader controls are still at their default values¹)
  • from controls each following update

This means touching the usual 3-state switch or curve knob on a controller will update the xfader graphic immedtiately.

¹Since mappings are loaded before DlgPreferences is instantiated this is supposed to detect if mappings have already set the xfader config in their init(), either explicitly or received updates via inputReport request.

Will do another test soon with the S4 Mk3 mapping which requests a status report on startup to sync all controls.

Comment on lines 45 to 52
const ConfigKey kXfaderModeKey = ConfigKey(EngineXfader::kXfaderGroup,
EngineXfader::kXfaderModeKey);
const ConfigKey kXfaderCurveKey = ConfigKey(EngineXfader::kXfaderGroup,
EngineXfader::kXfaderCurveKey);
const ConfigKey kXfaderCalibrationKey = ConfigKey(EngineXfader::kXfaderGroup,
EngineXfader::kXfaderCalibrationKey);
const ConfigKey kXfaderReverseKey = ConfigKey(EngineXfader::kXfaderGroup,
EngineXfader::kXfaderReverseKey);
Copy link
Member

Choose a reason for hiding this comment

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

This is UB and will very likely cause crashes on startup. https://en.cppreference.com/w/cpp/language/siof

Copy link
Member Author

@ronso0 ronso0 Jan 6, 2025

Choose a reason for hiding this comment

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

Hmm, so IIUC this is not specific to the diff here but also applies to the state before?
Like EngineXfader::kXfaderConfigKey being defined and initialized in the namespace here?
And making those members of DlgPrefMixer and move them to the initializer list would avoid it, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Or create a few getXYZCfgKey() helpers in dlgprefmixer.h?

Copy link
Member Author

@ronso0 ronso0 Jan 6, 2025

Choose a reason for hiding this comment

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

Or simply define them in enginexfader.h like this:
static const QString kXfaderGroup = QStringLiteral("[Mixer Profile]");
?

meeh..

error: in-class initialization of static data member ‘const QString EngineXfader::kXfaderGroup’ of non-literal type

Copy link
Member Author

Choose a reason for hiding this comment

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

okay, I use

const ConfigKey xfaderModeKey() {
    return ConfigKey(EngineXfader::kXfaderGroup, EngineXfader::kXfaderModeKey);
}

and alike in this namespace.
Should be fine since they're not called before DlgPrefMixer is constructed?
Please check my fixups.

(sorry I force-pushed earlier, some debug stuff slipped in which I had to remove)

Copy link
Member

Choose a reason for hiding this comment

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

The problem is not that its defined in the namespace (please do and keep that), the problem is that you expose the symbol via the header and then other translation units initialize their static data based on the static data of another translation unit. The solution in both cases is either some "construct on first use" idiom (there are multiple ways to achieve that in C++) or just don't depend on static data from other translation units. So duplicate the QString across both translation units. Its ugly but thats what we get for using QStrings for everything...

@ronso0 ronso0 force-pushed the pref-xfader-from-controls branch 3 times, most recently from 6025dbe to f00cfec Compare January 7, 2025 09:13
Comment on lines 22 to 26
static const QString kXfaderGroup;
static const QString kXfaderModeKey;
static const QString kXfaderCurveKey;
static const QString kXfaderCalibrationKey;
static const QString kXfaderReverseKey;
Copy link
Member

@Swiftb0y Swiftb0y Jan 7, 2025

Choose a reason for hiding this comment

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

These are the offenders. If you can come up with a solution that removes this static data declarations from the header, you should be good. Again, I recommend you simply duplicate the required QStrings/ConfigKeys across both files. The same technically applies to the other more trivial members (double and const char*) too, it just likely doesn't crash in the unitialized state because the code is able to handle the "unitialized" values. Its still UB though. Best would be to make them static constexpr and initialize right there in the header, but thats not an option for the QStrings.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the explanation, though I'm at a loss: I read about Construct on first Use but explanations and tutorials only refer to obejcts/pointers and classes with a constructor, which is not the case for EngineXfader.
I'll revert the commit and stick with the duplicate QStrings.

@ronso0 ronso0 force-pushed the pref-xfader-from-controls branch from f00cfec to b8d477a Compare January 9, 2025 14:25
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.

2 participants