-
-
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 64 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) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,14 +1,7 @@ | ||
/** | ||
* @file bulkcontroller.h | ||
* @author Neale Picket [email protected] | ||
* @date Thu Jun 28 2012 | ||
* @brief USB Bulk controller backend | ||
*/ | ||
|
||
#ifndef BULKCONTROLLER_H | ||
#define BULKCONTROLLER_H | ||
#pragma once | ||
|
||
#include <QAtomicInt> | ||
#include <QThread> | ||
|
||
#include "controllers/controller.h" | ||
#include "controllers/hid/hidcontrollerpreset.h" | ||
|
@@ -28,7 +21,7 @@ class BulkReader : public QThread { | |
void stop(); | ||
|
||
signals: | ||
void incomingData(QByteArray data, mixxx::Duration timestamp); | ||
void incomingData(const QByteArray& data, mixxx::Duration timestamp); | ||
|
||
protected: | ||
void run(); | ||
|
@@ -107,5 +100,3 @@ class BulkController : public Controller { | |
BulkReader* m_pReader; | ||
HidControllerPreset m_preset; | ||
}; | ||
|
||
#endif |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,27 +1,21 @@ | ||
/** | ||
* @file controller.h | ||
* @author Sean Pappalardo [email protected] | ||
* @date Sat Apr 30 2011 | ||
* @brief Base class representing a physical (or software) controller. | ||
* | ||
* This is a base class representing a physical (or software) controller. It | ||
* must be inherited by a class that implements it on some API. Note that the | ||
* subclass' destructor should call close() at a minimum. | ||
*/ | ||
|
||
#ifndef CONTROLLER_H | ||
#define CONTROLLER_H | ||
#pragma once | ||
|
||
#include <QElapsedTimer> | ||
#include <QTimerEvent> | ||
|
||
#include "controllers/controllerpreset.h" | ||
#include "controllers/controllerpresetfilehandler.h" | ||
#include "controllers/controllerpresetinfo.h" | ||
#include "controllers/controllerpresetvisitor.h" | ||
#include "controllers/controllervisitor.h" | ||
#include "controllers/engine/controllerengine.h" | ||
#include "controllers/scripting/legacy/controllerscriptenginelegacy.h" | ||
#include "util/duration.h" | ||
|
||
class ControllerJSProxy; | ||
|
||
/// This is a base class representing a physical (or software) controller. It | ||
/// must be inherited by a class that implements it on some API. Note that the | ||
/// subclass' destructor should call close() at a minimum. | ||
class Controller : public QObject, ConstControllerPresetVisitor { | ||
Q_OBJECT | ||
public: | ||
|
@@ -88,15 +82,9 @@ class Controller : public QObject, ConstControllerPresetVisitor { | |
// Handles packets of raw bytes and passes them to an ".incomingData" script | ||
// function that is assumed to exist. (Sub-classes may want to reimplement | ||
// this if they have an alternate way of handling such data.) | ||
virtual void receive(const QByteArray data, mixxx::Duration timestamp); | ||
virtual void receive(const QByteArray& data, mixxx::Duration timestamp); | ||
|
||
/// Apply the preset to the controller. | ||
/// @brief Initializes both controller engine and static output mappings. | ||
/// | ||
/// @param initializeScripts Can be set to false to skip script | ||
/// initialization for unit tests. | ||
/// @return Returns whether it was successful. | ||
virtual bool applyPreset(bool initializeScripts = true); | ||
virtual bool applyPreset(); | ||
|
||
// Puts the controller in and out of learning mode. | ||
void startLearning(); | ||
|
@@ -113,17 +101,17 @@ class Controller : public QObject, ConstControllerPresetVisitor { | |
|
||
// To be called in sub-class' open() functions after opening the device but | ||
// before starting any input polling/processing. | ||
void startEngine(); | ||
virtual void startEngine(); | ||
|
||
// To be called in sub-class' close() functions after stopping any input | ||
// polling/processing but before closing the device. | ||
void stopEngine(); | ||
virtual void stopEngine(); | ||
|
||
// To be called when receiving events | ||
void triggerActivity(); | ||
|
||
inline ControllerEngine* getEngine() const { | ||
return m_pEngine; | ||
inline ControllerScriptEngineLegacy* getScriptEngine() const { | ||
return m_pScriptEngineLegacy; | ||
} | ||
inline void setDeviceName(QString deviceName) { | ||
m_sDeviceName = deviceName; | ||
|
@@ -160,7 +148,7 @@ class Controller : public QObject, ConstControllerPresetVisitor { | |
// Returns a pointer to the currently loaded controller preset. For internal | ||
// use only. | ||
virtual ControllerPreset* preset() = 0; | ||
ControllerEngine* m_pEngine; | ||
ControllerScriptEngineLegacy* m_pScriptEngineLegacy; | ||
|
||
// Verbose and unique device name suitable for display. | ||
QString m_sDeviceName; | ||
|
@@ -203,5 +191,3 @@ class ControllerJSProxy : public QObject { | |
private: | ||
Controller* const m_pController; | ||
}; | ||
|
||
#endif |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,20 +1,14 @@ | ||
/** | ||
* @file controllermanager.cpp | ||
* @author Sean Pappalardo [email protected] | ||
* @date Sat Apr 30 2011 | ||
* @brief Manages creation/enumeration/deletion of hardware controllers. | ||
*/ | ||
#include "controllers/controllermanager.h" | ||
|
||
#include <QSet> | ||
#include <QThread> | ||
|
||
#include "util/trace.h" | ||
#include "controllers/controllermanager.h" | ||
#include "controllers/defs_controllers.h" | ||
#include "controllers/controllerlearningeventfilter.h" | ||
#include "controllers/defs_controllers.h" | ||
#include "controllers/midi/portmidienumerator.h" | ||
#include "util/cmdlineargs.h" | ||
#include "util/time.h" | ||
|
||
#include "controllers/midi/portmidienumerator.h" | ||
#include "util/trace.h" | ||
#ifdef __HSS1394__ | ||
#include "controllers/midi/hss1394enumerator.h" | ||
#endif | ||
|
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