-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
split up ControllerEngine #2920
Changes from 30 commits
fed2e1b
9c04291
24408ab
979a243
b3d6ff8
72e849f
0968a9d
77ca43f
8153b3f
9363b49
e25bf12
e8e2487
26b51ec
e99cef3
f6f7050
b4d2d21
197c0d0
62fc40c
2046add
a691d32
487f8f9
2b6eb9f
fa16b57
427ba5e
caf3d68
6c1a6b6
df4f377
8ec7ed8
0d39501
13f86a5
e028e3e
5c1ace9
bdf497b
3383aef
19b9aaf
4505c86
95b83ea
e4bdfdd
269de0d
d72618f
d677f4b
5b02715
424c94b
d50b710
fa8771f
c96a2c2
cb41ffd
a80ca2c
f7b0ddc
0112554
65de8c5
94d9423
388b4e3
57c0219
e504678
e35e90f
5b81d28
98d9e37
4e744e0
6a78b67
0e6119c
145cf8e
7f2187d
30692a1
86bebb6
4227e22
b8e2430
20fbcb0
b5cf59d
1911580
b5fafcc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,9 +1,11 @@ | ||
#include "control/controlobjectscript.h" | ||
|
||
#include <QtDebug> | ||
|
||
#include "control/controlobjectscript.h" | ||
#include "controllers/controllerdebug.h" | ||
|
||
ControlObjectScript::ControlObjectScript(const ConfigKey& key, QObject* pParent) | ||
: ControlProxy(key, pParent, ControlFlag::NoAssertIfMissing) { | ||
: ControlProxy(key, pParent, ControllerDebug::shouldAssertForInvalidControlObjects()) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please explain under which conditions we crash. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This is trivially discoverable by reading the implementation of this simple function. If you can suggest a better name for the function, then do so, but again, comments that simply repeat what the code already says are noise. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can't follow your explaination. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Usually I'd agree, but here this behavior was requested explicitly by passing the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You responds is to an outdated message. Here it is again:
|
||
} | ||
|
||
bool ControlObjectScript::addScriptConnection(const ScriptConnection& conn) { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This means we crash Mixxx because of scripting issues during testing, but mute all warning during nomal use.
Both feels odd. It is annoying to crash Mixxx because of a typo in the script and silence script issues that can evolve at any time is also not good.
Instead we should verify to if the case is handled with reasonable infos in both cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does not assert in tests 8ec7ed8
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am still convinced that this should not crash and we need a change here.
But maybe I have missed something. Please describe the current situation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no crash. I don't know what you're talking about.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you're questioning the changes merged in #3056, this is not an appropriate place to do so. This PR is not changing behavior; only refactoring. I will not discuss any proposals to change behavior here. This PR has been waiting long enough. There is lots more work ahead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this will always assert not. Mapping Typo = crash ...
This is a change in this PR and that must not happen. Instead a error dialog is appropriated like we did for other scripting issues.
See related discussion.
https://mixxx.zulipchat.com/#narrow/stream/247620-development-help/topic/Crash.20starting.202.2E3.20if.20controller.20script.20has.20a.20specific.20kind
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not a behavior change. It was just implemented a little differently compared to main when resolving merge conflicts. Here is the debug assertion in main.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This only asserts if mixxx was started with the
--controllerDebug
flag and debug assertions are compiled in. And it will only crash if debug assertions are additionally set to fatal. So I don't see a problem with this.Also, that behavior was not introduced by this branch, so it shouldn't block merge.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sound reasonable.
Unfortunately I had hard times to understand that from the code.
I think this can be solved by a comment or a better function name.
From a distance I think it is that the root cause is that "shouldAssertForInvalidControlObjects = ControlFlag::None" read "Like Don't Assert". So I get hooked to the inverted logic.
Instead the function should become a bool.
And than we can do here more visible.
shouldAssertForInvalidControlObjects() ? ControlFlag::None : ControlFlag::AllowMissingOrInvalid
In addition ControlFlag::None can become ControlFlag::AssertValid or something