Prevent wrong usage of Control Objects with assertions by Holzhaus · Pull Request #2896 · mixxxdj/mixxx

Holzhaus · 2020-06-21T12:21:34Z

This should prevent CO failures from going unnoticed. Right now, only a warning is displayed in the log, which can easily be overlooked.

Zulip discussion that prompted this PR: https://mixxx.zulipchat.com/#narrow/stream/109171-development/topic/Macros.2FSaved.20Hotcue.20Routines.2F.22Serato.20Flip.22/near/201478587

uklotzde · 2020-06-21T12:37:48Z


+enum class ControlFlag {
+    None = 0,
+    NoWarnIfMissing = 1,


The negation make the code harder to understand. Why not WarnIfMissing and WarnAndAssertIfMissing? Using flags is not necessary according to the code, because the assertion is nested in the warning path. Use a simple enum class instead.

I was planning on adding a few more flags, e.g. DelayedInitialization for ControlProxy objects that are created before the actual CO (e.g. [AutoDJ],enabled). Right now we just disable the warnings completely, but IMHO this is not a good solution.

Why not WarnIfMissing and WarnAndAssertIfMissing?

I wanted to make it explicit from the caller side that the warnings/assertions are disabled, so that don't set InitializeLater and accidently disable warnings/assertions because you forgot to OR this with the WarnAndAssertIfMissing flag.

Btw, I also had to add a flag to allow missing keys for WWidgetStack (which takes params from skins).

uklotzde · 2020-06-21T12:44:49Z

Renamed the PR to better reflect the intention.

Holzhaus · 2020-06-21T18:14:41Z

I hope everything is in order now. I feel like our CO stuff could use some refactoring, e.g. the skin system uses getControl to let skins create COs. I also removed the confusing method to change the key of a ControlProxy after creating it through the initialize method (just use a new object if you want that) and got rid of the ControlProxy constructor without key (which is apparently not needed and also not useful anymore).

daschuer · 2020-06-22T00:10:00Z

Instead of Implicit disable warnings, we can introduce another function that is called getOrCreateControl() or something and maybe tryGetControl()

daschuer · 2020-06-22T00:11:13Z

Is this really a 2.3 PR? This is not without a risk.

Holzhaus · 2020-06-23T15:38:07Z

Is this really a 2.3 PR? This is not without a risk.

I think it makes sense. It adds additional safety and the additional debug assertions will be disabled in release builds anyway.

uklotzde · 2020-06-23T22:20:32Z

Warning [Main]: ControlDoublePrivate::getControl returning NULL for ( "" , "visual_key_distance" )
Critical [Main]: DEBUG ASSERT: "flags.testFlag(ControlFlag::NoAssertIfMissing)" in function static QSharedPointer<ControlDoublePrivate> ControlDoublePrivate::getControl(const ConfigKey&, ControlFlags, ControlObject*, bool, bool, bool, double) at /tmp/mixxx/src/control/control.cpp:148

uklotzde

master: Multiple debug assertions while starting Mixxx

Holzhaus · 2020-06-23T22:29:01Z

master: Multiple debug assertions while starting Mixxx

You mean you pulled this branch into master?

uklotzde · 2020-06-23T22:31:21Z

master: Multiple debug assertions while starting Mixxx

You mean you pulled this branch into master?

Yes, my regular development version is based on master.

Holzhaus · 2020-06-23T22:34:46Z

I suppose this might be caused by the controller engine changes from #2682. Does this PR (based on 2.3) work for you?

uklotzde · 2020-06-29T22:19:40Z

Reminder for later: The changes in ControllerEngine have to be fixed manually when merging from 2.3 into master!

Remaining debug assertions in master on startup:

Warning [Main]: ControlDoublePrivate::getControl returning NULL for ( "" , "visual_key_distance" )
Critical [Main]: DEBUG ASSERT: "flags.testFlag(ControlFlag::NoAssertIfMissing)" in function static QSharedPointer<ControlDoublePrivate> ControlDoublePrivate::getControl(const ConfigKey&, ControlFlags, ControlObject*, bool, bool, bool, double) at /tmp/mixxx/src/control/control.cpp:148
Critical [Main]: DEBUG ASSERT: "m_pControl || flags.testFlag(ControlFlag::NoAssertIfMissing)" in function void ControlProxy::initialize(ControlFlags) at /tmp/mixxx/src/control/controlproxy.cpp:35
Critical [Main]: DEBUG ASSERT: "valid() || flags.testFlag(ControlFlag::NoAssertIfMissing)" in function void ControlProxy::initialize(ControlFlags) at /tmp/mixxx/src/control/controlproxy.cpp:36
Warning [Main]: ControlDoublePrivate::getControl returning NULL for ( "" , "visual_key_distance" )
Critical [Main]: DEBUG ASSERT: "flags.testFlag(ControlFlag::NoAssertIfMissing)" in function static QSharedPointer<ControlDoublePrivate> ControlDoublePrivate::getControl(const ConfigKey&, ControlFlags, ControlObject*, bool, bool, bool, double) at /tmp/mixxx/src/control/control.cpp:148
Critical [Main]: DEBUG ASSERT: "m_pControl || flags.testFlag(ControlFlag::NoAssertIfMissing)" in function void ControlProxy::initialize(ControlFlags) at /tmp/mixxx/src/control/controlproxy.cpp:35
Critical [Main]: DEBUG ASSERT: "valid() || flags.testFlag(ControlFlag::NoAssertIfMissing)" in function void ControlProxy::initialize(ControlFlags) at /tmp/mixxx/src/control/controlproxy.cpp:36
Warning [Main]: ControlDoublePrivate::getControl returning NULL for ( "" , "visual_key_distance" )
Critical [Main]: DEBUG ASSERT: "flags.testFlag(ControlFlag::NoAssertIfMissing)" in function static QSharedPointer<ControlDoublePrivate> ControlDoublePrivate::getControl(const ConfigKey&, ControlFlags, ControlObject*, bool, bool, bool, double) at /tmp/mixxx/src/control/control.cpp:148
Critical [Main]: DEBUG ASSERT: "m_pControl || flags.testFlag(ControlFlag::NoAssertIfMissing)" in function void ControlProxy::initialize(ControlFlags) at /tmp/mixxx/src/control/controlproxy.cpp:35
Critical [Main]: DEBUG ASSERT: "valid() || flags.testFlag(ControlFlag::NoAssertIfMissing)" in function void ControlProxy::initialize(ControlFlags) at /tmp/mixxx/src/control/controlproxy.cpp:36
Warning [Main]: ControlDoublePrivate::getControl returning NULL for ( "" , "visual_key_distance" )
Critical [Main]: DEBUG ASSERT: "flags.testFlag(ControlFlag::NoAssertIfMissing)" in function static QSharedPointer<ControlDoublePrivate> ControlDoublePrivate::getControl(const ConfigKey&, ControlFlags, ControlObject*, bool, bool, bool, double) at /tmp/mixxx/src/control/control.cpp:148
Critical [Main]: DEBUG ASSERT: "m_pControl || flags.testFlag(ControlFlag::NoAssertIfMissing)" in function void ControlProxy::initialize(ControlFlags) at /tmp/mixxx/src/control/controlproxy.cpp:35
Critical [Main]: DEBUG ASSERT: "valid() || flags.testFlag(ControlFlag::NoAssertIfMissing)" in function void ControlProxy::initialize(ControlFlags) at /tmp/mixxx/src/control/controlproxy.cpp:36

These have to be fixed in time if we decide to merge this PR. Otherwise starting Mixxx with development settings in master becomes annoying.

I assume that these assertions do not appear in 2.3 if you tested it. No need to recompile everything twice locally. But we need to have a plan how to fix those in master.

uklotzde · 2020-06-30T09:15:07Z

@daschuer Merge?

Holzhaus · 2020-06-30T10:21:12Z

Let me recheck this first, I'll make sure that none of the assertions from master are already present here. I could be that these assertions just don't fire because skin is not lacking a CO.

daschuer · 2020-07-01T14:47:05Z

The flaky test strikes again:

        Start  19: AutoDJProcessorTest.EnabledSuccess_DecksStopped_TrackLoadFails
 18/616 Test  #19: AutoDJProcessorTest.EnabledSuccess_DecksStopped_TrackLoadFails ..................***Failed    0.06 sec
qt.qpa.screen: QXcbConnection: Could not connect to display :99.0
Could not connect to any X display.

can't we call mixxx-test instead of ctest?

daschuer · 2020-07-01T14:47:43Z

The rest LGTM, Thank you.

Holzhaus · 2020-07-01T23:06:14Z

The NoWarnIfMissing flag is only used for the legacy skin parser that uses getControl to check if a CO already exists. We could drop it by adding a ControlObject::exists(key) or ControlObject::getOrCreateControl(key) function. Should I do that? If so, what do you prefer?

daschuer · 2020-07-02T06:08:05Z

Yes ControlObject::getOrCreateControl(key) would be much better express the intent.
I would prefer this, because it will require only one hash lookup.

daschuer · 2020-07-02T06:32:33Z

Oh sorry, scratch that.

I am fine with the current solution. It is performs good and is easy to understand.

Holzhaus · 2020-07-02T12:58:59Z

Ok, then it's ready to merge now.

daschuer · 2020-07-02T14:16:56Z

Thank you for taking care if this.

uklotzde · 2020-07-03T23:41:16Z

After merging 2.3 into master many tests trigger debug assertions:

Example:

ControlDoublePrivate::getControl returning NULL for ( "[Master]" , "num_decks" )
DEBUG ASSERT: "flags.testFlag(ControlFlag::NoAssertIfMissing)" in function static QSharedPointer<ControlDoublePrivate> ControlDoublePrivate::getControl(const ConfigKey&, ControlFlags, ControlObject*, bool, bool, bool, double) at /tmp/mixxx/src/control/control.cpp:144
DEBUG ASSERT: "m_pControl || flags.testFlag(ControlFlag::NoAssertIfMissing)" in function void ControlProxy::initialize(ControlFlags) at /tmp/mixxx/src/control/controlproxy.cpp:35
DEBUG ASSERT: "valid() || flags.testFlag(ControlFlag::NoAssertIfMissing)" in function void ControlProxy::initialize(ControlFlags) at /tmp/mixxx/src/control/controlproxy.cpp:36
a

Holzhaus · 2020-07-04T20:36:35Z

See #2910.

control: Use assertions in ControlDoublePrivate if warn is true

c127832

Holzhaus added the code quality label Jun 21, 2020

Holzhaus changed the base branch from master to 2.3 June 21, 2020 12:21

uklotzde suggested changes Jun 21, 2020

View reviewed changes

uklotzde added this to the 2.3.0 milestone Jun 21, 2020

uklotzde changed the title ~~[WIP] Improve safety of Control Objects with assertions~~ [WIP] Prevent wrong usage of Control Objects with assertions Jun 21, 2020

Holzhaus added 5 commits June 21, 2020 18:36

control: Refactor ControlProxy and require flag to allow empty keys

96558ba

control: Remove unneeded if statement around debug assertion

64b9883

skin/legacyskinparser: Don't warn when skins create controls

88082cf

widget/wmainmenubar: Do not attach to controls before skin is loaded

b2542b4

control: Print warning instead of debug message if CO exists

5c65773

Holzhaus requested a review from uklotzde June 21, 2020 18:09

Holzhaus changed the title ~~[WIP] Prevent wrong usage of Control Objects with assertions~~ Prevent wrong usage of Control Objects with assertions Jun 21, 2020

uklotzde reviewed Jun 21, 2020

View reviewed changes

Holzhaus added 4 commits June 22, 2020 00:40

control/controlproxy: Remove useless setFlag call

ac0724e

control: Fix some style issues in legacy code

0e9b7f1

test/beatstranslatetest: Remove unneeded control flags

aaa3ee5

control/controlproxy: Clean up some debug assertions

98bd364

daschuer reviewed Jun 22, 2020

View reviewed changes

Comment thread src/mixer/playermanager.cpp Outdated

daschuer requested changes Jun 22, 2020

View reviewed changes

Comment thread src/skin/legacyskinparser.cpp

Holzhaus added 3 commits June 23, 2020 16:31

controls/controlproxy: Don't call initialize if key is null

ec5100f

mixer/playermanager: Fix ControlFlag for [AutoDJ],enabled ControlProxy

8b5fbbe

widget/wwidgetstack: Add missing ControlProxy::AllowEmptyKey flag

9bf40c1

uklotzde approved these changes Jun 23, 2020

View reviewed changes

uklotzde self-requested a review June 23, 2020 22:22

uklotzde suggested changes Jun 23, 2020

View reviewed changes

uklotzde approved these changes Jun 29, 2020

View reviewed changes

daschuer requested changes Jun 30, 2020

View reviewed changes

Comment thread src/control/control.h Outdated

Comment thread src/mixer/playermanager.cpp Outdated

Comment thread src/mixer/playermanager.cpp Outdated

Holzhaus added 2 commits July 1, 2020 12:40

widget: Disable assertions for ControlProxy's created from skins

78a466f

control: Remove ControlFlag::InitializeLater enum value

d5eeb79

daschuer reviewed Jul 1, 2020

View reviewed changes

Comment thread src/control/control.h Outdated

daschuer approved these changes Jul 1, 2020

View reviewed changes

control/control: Renumber ControlFlag enum members

10dc4f5

daschuer merged commit 98e95a5 into mixxxdj:2.3 Jul 2, 2020

Uh oh!

Conversation

Holzhaus commented Jun 21, 2020

Uh oh!

Uh oh!

Uh oh!

uklotzde Jun 21, 2020

Choose a reason for hiding this comment

Uh oh!

Holzhaus Jun 21, 2020

Choose a reason for hiding this comment

Uh oh!

Holzhaus Jun 21, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

uklotzde commented Jun 21, 2020

Uh oh!

Holzhaus commented Jun 21, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

daschuer commented Jun 22, 2020

Uh oh!

daschuer commented Jun 22, 2020

Uh oh!

Holzhaus commented Jun 23, 2020

Uh oh!

uklotzde commented Jun 23, 2020

Uh oh!

uklotzde left a comment

Choose a reason for hiding this comment

Uh oh!

Holzhaus commented Jun 23, 2020

Uh oh!

uklotzde commented Jun 23, 2020

Uh oh!

Holzhaus commented Jun 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

uklotzde commented Jun 29, 2020

Uh oh!

uklotzde commented Jun 30, 2020

Uh oh!

Holzhaus commented Jun 30, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

daschuer commented Jul 1, 2020

Uh oh!

daschuer commented Jul 1, 2020

Uh oh!

Holzhaus commented Jul 1, 2020

Uh oh!

daschuer commented Jul 2, 2020

Uh oh!

daschuer commented Jul 2, 2020

Uh oh!

Holzhaus commented Jul 2, 2020

Uh oh!

daschuer commented Jul 2, 2020

Uh oh!

uklotzde commented Jul 3, 2020

Uh oh!

Holzhaus commented Jul 4, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Holzhaus commented Jun 23, 2020 •

edited

Loading