QML: Add support for selecting/loading effects #4000

Holzhaus · 2021-06-15T15:45:54Z

Based on #3992.

Swiftb0y · 2021-06-15T16:34:41Z

Please Rebase.

Also PR nr 4000. Yeah 🎉

This allows accessing the visible effect list from QML.

Holzhaus · 2021-06-15T17:00:40Z

Caveat: The comboboxes currently don't have any signal connections to the actual effect slot. This means changing the effects from the controller or via COs won't update the combobox atm.

Swiftb0y

I really can't comment on any C++ changes. Though the current approach seems a bit clunky to me.

res/skins/QMLDemo/EffectUnit.qml

Swiftb0y · 2021-06-15T16:43:58Z

src/skin/qml/qmlvisibleeffectsmodel.h

+class QmlVisibleEffectsModel : public QAbstractListModel {
+    Q_OBJECT
+  public:
+    enum Roles {


enum class? not sure though if that works with Q_ENUM

It works, but it's kind of clunky because of all the casting between int and the enum value. Since the enum part of the class body and each member has a "Role" suffix, I think it's okay like this.

@uklotzde what do you think?

Swiftb0y · 2021-06-15T19:32:20Z

res/skins/QMLDemo/EffectUnit.qml

+                    model: Mixxx.EffectsManager.visibleEffectsModel
+                    onActivated: {
+                        console.warn(1, root.unitNumber, effect.effectNumber, model.get(index).effectId);
+                        Mixxx.EffectsManager.loadEffect(1, root.unitNumber, effect.effectNumber, model.get(index).effectId);


I'm not sure I like this... Though I don't really have anything better to propose...

The question is how do we receive changes. I checked the controls and didn't see any COs that indicate the currently loaded effect. Therefore I think we need to add a QmlEffectChainSlotProxy that can emit signals when the effect is changed. If we do that, we can move the load method there.

Binding to (EffectRack1_EffectUnitN_EffectM,loaded) might work. It should emit a "trigger" signal even if the value didn't change. But in the long term, we do need QmlEffectChainSlotProxy anyways so we can configure the parameters and so forth. We might even consider trying to remove the GUI changes from #2618 and redo that part based on QML.

I don't think we actually need it for parameter configuration except the names.

Well we don't really need it, but I find the current way its implemented with COs very hacky and would like to get rid of it in the longterm actually..

Swiftb0y · 2021-06-15T19:46:08Z

Clicking outside the Combobox popup doesn't close it for some reason. I need to click on another interactive element such as a Button or Knob. It would also be great if the combobox was editable and you could just search for the effect you wanted in the model, but thats polish to be done later.

Swiftb0y · 2021-06-16T07:55:35Z

I'm questioning whether this is a good approach. The overengineering of the effect system makes it really hard to judge properly.
I personally don't like the amount of "getters on globals" (such as Mixxx.EffectsManager.getEffectSlot) for example.

I'm ok with merging it as it for the sake of progress, but I don't want to have to commit to this API in the longterm.

Holzhaus · 2021-06-16T09:09:18Z

I agree, but on the other hand I don't know if it's possible to expose stuff like that with making EffectsManager a singleton which I dislike even more.

Swiftb0y

Not super happy with the QML but its fine as a short term solution. I'd prefer if someone with more C++ experience would take a look at that though before merging.

Holzhaus · 2021-06-21T00:32:16Z

I also added initial support for expandable effect units. Don't pay too much attention to that, it's not that useful yet because there is no support for effect parameter names, linking or other fancy stuff yet.

I'll probably have to refactor it later on by implementing a QmlEffectParametersModel that is used by a ListView. It'll take care of that in a separate PR.

Holzhaus · 2021-06-21T15:21:10Z

Nevermind, switched to the ListView approach in this PR.

Swiftb0y

The size of the line indicator seems too big.

res/skins/QMLDemo/EffectRow.qml

res/skins/QMLDemo/EffectSlot.qml

Swiftb0y · 2021-06-21T17:25:43Z

res/skins/QMLDemo/EffectSlot.qml

+    property int unitNumber // required
+    property int effectNumber // required
+    property bool expanded: false
+    readonly property string group: slot.group


Suggested change

readonly property string group: slot.group

property alias group: slot.group

I assume this doesn't work?

Didn't try, maybe the readonly flag from the slot property is also applying to the alias. I can check.

Nope, doesn't work. Maybe because the order of property initializiation is not defined or something:

warning [Main] file:///home/jan/Projects/mixxx/res/skins/QMLDemo/main.qml:120:9: Type EffectRow unavailable EffectRow { ^ warning [Main] file:///home/jan/Projects/mixxx/res/skins/QMLDemo/EffectRow.qml:14:9: Type Skin.EffectUnit unavailable Skin.EffectUnit { ^ warning [Main] file:///home/jan/Projects/mixxx/res/skins/QMLDemo/EffectUnit.qml:24:9: Type EffectSlot unavailable EffectSlot { ^ warning [Main] file:///home/jan/Projects/mixxx/res/skins/QMLDemo/EffectSlot.qml:13:27: Invalid alias reference. Unable to find id "slot" property alias group: slot.group ^ warning [Main] Skin "QMLDemo" failed to load!

Mhmm thats strange. I guess I'm misinterpreting the Qt docs section https://doc.qt.io/qt-5/qtqml-syntax-objectattributes.html#considerations-for-property-aliases

res/skins/QMLDemo/EffectSlot.qml

Swiftb0y · 2021-06-21T18:10:47Z

res/skins/QMLDemo/EffectUnit.qml

@@ -1,75 +1,109 @@
 import "." as Skin
+import Mixxx 0.1 as Mixxx
 import QtQuick 2.12
 import QtQuick.Controls 2.12
 import QtQuick.Layouts 1.12
 import "Theme"

 Item {


not sure if you think the effort is worth it, but you could add the expanded controls from latenight as well:

This PR is already quite big. Someone can take care of that separately I think. There's also other stuff missing, like the link type, or configuring the knob arc start from the parameter, etc.

src/skin/qml/qmleffectmanifestparametersmodel.cpp

Swiftb0y · 2021-06-21T18:43:00Z

src/skin/qml/qmleffectmanifestparametersmodel.cpp

+    if (parent.isValid()) {
+        return 0;
+    }


Suggested change

if (parent.isValid()) {

return 0;

}

if (!parent.isValid()) {

return 0;

}

not sure how the QModelIndex API is used, but returning early when the parent is valid seems strange.

This model is a list. This means it has N rows. If parent is valid, this means that we're not requesting the rowCount for the whole list, but for an individual item. If this was a tree model (like the library sidebar), then each item might also have children. This is not the case here. Therefore, we always return zero, because no item has children.

I understand now. Thanks for the explanation. I guess I should read the Qt docs first without asking questions in code review.

Swiftb0y

Thanks. Unfortunately there are still two blocking issues IMO.

The effectUnit desperately needs a DryWet knob, otherwise the unit stays inaudible when mixxx started completely dry.
The buttons for the parameters of a effect don't actually work atm. You can most easily test this using the Tremolo Effect and playing with the Quantize and Triplet buttons.

res/skins/QMLDemo/EffectSlot.qml

Holzhaus · 2021-06-22T11:09:02Z

Thanks. Unfortunately there are still two blocking issues IMO.

The effectUnit desperately needs a DryWet knob, otherwise the unit stays inaudible when mixxx started completely dry.

I don't consider this a blocker, but I added it. Only visible when expanded though.

The buttons for the parameters of a effect don't actually work atm. You can most easily test this using the Tremolo Effect and playing with the Quantize and Triplet buttons.

I checked, and some buttons did work, some did not, because our effect CO infrastructure is badly designed IMHO. I tried to mitigate this by adding a way to retrieve the parameter control key from the model.

Swiftb0y

I checked, and some buttons did work, some did not, because our effect CO infrastructure is badly designed IMHO. I tried to mitigate this by adding a way to retrieve the parameter control key from the model.

Thats very strange I'll guess we should take care of it in conjunction with #2618.

LGTM. I guess no one else is interested in reviewing this so I'm just going to merge now.

ywwg · 2021-06-28T03:14:10Z

This merge seems to have made mixxx unbuildable at HEAD for me:

/usr/bin/ld: libmixxx-lib.a(mocs_compilation.cpp.o): in function `mixxx::skin::qml::QmlEffectsManagerProxy::qt_static_metacall(QObject*, QMetaObject::Call, int, void**)':
mocs_compilation.cpp:(.text+0x9a4): undefined reference to `mixxx::skin::qml::QmlEffectsManagerProxy::getEffectSlot(int, int, int) const'
/usr/bin/ld: libmixxx-lib.a(mocs_compilation.cpp.o): in function `mixxx::skin::qml::QmlVisibleEffectsModel::qt_static_metacall(QObject*, QMetaObject::Call, int, void**)':
mocs_compilation.cpp:(.text+0xa8d): undefined reference to `mixxx::skin::qml::QmlVisibleEffectsModel::get(int) const'
/usr/bin/ld: libmixxx-lib.a(mocs_compilation.cpp.o): in function `mixxx::skin::qml::QmlEffectManifestParametersModel::qt_static_metacall(QObject*, QMetaObject::Call, int, void**)':
mocs_compilation.cpp:(.text+0xb6f): undefined reference to `mixxx::skin::qml::QmlEffectManifestParametersModel::get(int) const'
/usr/bin/ld: libmixxx-lib.a(mocs_compilation.cpp.o): in function `mixxx::skin::qml::QmlEffectSlotProxy::qt_static_metacall(QObject*, QMetaObject::Call, int, void**)':
mocs_compilation.cpp:(.text+0xe7a): undefined reference to `mixxx::skin::qml::QmlEffectSlotProxy::getEffectId() const'
/usr/bin/ld: mocs_compilation.cpp:(.text+0xeca): undefined reference to `mixxx::skin::qml::QmlEffectSlotProxy::getGroup() const'
/usr/bin/ld: mocs_compilation.cpp:(.text+0xed1): undefined reference to `mixxx::skin::qml::QmlEffectSlotProxy::getNumber() const'
/usr/bin/ld: mocs_compilation.cpp:(.text+0xeea): undefined reference to `mixxx::skin::qml::QmlEffectSlotProxy::getChainSlotGroup() const'
/usr/bin/ld: mocs_compilation.cpp:(.text+0xef1): undefined reference to `mixxx::skin::qml::QmlEffectSlotProxy::getChainSlotNumber() const'
/usr/bin/ld: mocs_compilation.cpp:(.text+0xf0a): undefined reference to `mixxx::skin::qml::QmlEffectSlotProxy::getRackGroup() const'
/usr/bin/ld: mocs_compilation.cpp:(.text+0xf19): undefined reference to `mixxx::skin::qml::QmlEffectSlotProxy::getRackNumber() const'
/usr/bin/ld: mocs_compilation.cpp:(.text+0xf29): undefined reference to `mixxx::skin::qml::QmlEffectSlotProxy::getParametersModel() const'

Bisecting identifies the first bad commit as: 8cab623

At first glance I have no idea why.

ywwg · 2021-06-28T03:40:07Z

Cleaning out cbuild seems to fix it

github-actions bot added build skins labels Jun 15, 2021

Holzhaus added 4 commits June 15, 2021 18:56

QML: Add EffectManager proxy and visibleEffectModel

8cab623

This allows accessing the visible effect list from QML.

skins/QMLDemo: Use actual visibleEffectList from EffectManager

9a797ee

QML: Add a way to load effects via effect manager

2b9f3e2

skins/QMLDemo: Enable switching effects via comboboxes

dea4f22

Holzhaus force-pushed the qml-effects branch from 2bce7d2 to dea4f22 Compare June 15, 2021 16:59

Swiftb0y reviewed Jun 15, 2021

View reviewed changes

Holzhaus force-pushed the qml-effects branch from 3bcd539 to 1be5671 Compare June 16, 2021 09:51

Holzhaus added 2 commits June 16, 2021 12:27

QML: Add QmlEffectSlotProxy

5855308

skins/QMLDemo: Listen to effect changes via COs and update ComboBox

58ed35d

Holzhaus force-pushed the qml-effects branch from 1be5671 to 58ed35d Compare June 16, 2021 10:29

Swiftb0y approved these changes Jun 16, 2021

View reviewed changes

Holzhaus added 3 commits June 20, 2021 23:02

skins/QMLDemo: Move EffectSlot to separate file

79e555b

skins/QMLDemo: Ensure that the correct effect is selected after start

2d0234e

skins/QMLDemo: Make EffectUnit expandable

9e7756e

Holzhaus added 2 commits June 21, 2021 17:15

QML: Expose Effect slot parameters as model

8775193

skin/QMLDemo: Display EffectSlot parameters using ListView

66e33f2

Holzhaus added 2 commits June 21, 2021 17:54

Merge branch 'main' of github.com:mixxxdj/mixxx into qml-effects

7098d11

skins/QMLDemo: Fix broken EffectUnit bg when single unit is expanded

7486a67

Holzhaus requested a review from Swiftb0y June 21, 2021 16:14

Swiftb0y requested changes Jun 21, 2021

View reviewed changes

Holzhaus added 6 commits June 21, 2021 21:23

skins/QMLDemo: Use actual type instead var for property declaration

534df61

skins/QMLDemo: Remove obsolete TODO comment

8b5eeef

QML: Add TODO for enum usage to QmlEffectManifestParametersModel

aa271d8

skins/QMLDemo: Use JS template strings instead of concatenation

93dec8e

skins/QMLDemo: Fix MiniKnob indicator and reduce duplication

c8cb59a

skins/QMLDemo: Use fixed width for knob indicator

f58eabc

Holzhaus requested a review from Swiftb0y June 21, 2021 19:47

Holzhaus added 2 commits June 21, 2021 21:51

skins/QMLDemo: Fix label for FX parameters where shortName is not set

ae7408f

skins/QMLDemo: Increase effect parameter width to prevent elided text

c20b1e2

Swiftb0y requested changes Jun 21, 2021

View reviewed changes

res/skins/QMLDemo/EffectSlot.qml Show resolved Hide resolved

Holzhaus added 5 commits June 22, 2021 11:17

skins/QMLDemo: Add TODO regarding null coalescing

fdb46c0

skins/QMLDemo: Make effect param buttons toggleable and add label

77650d1

skins/QMLDemo: Show dryWet/superKnob only when EffectUnit is expanded

3265842

QML: Add a way to retrieve the Effect parameter control key from model

a99bd50

skins/QMLDemo: Fix wrong effect parameter control keys

c8cfefa

Holzhaus requested a review from Swiftb0y June 22, 2021 11:09

Swiftb0y approved these changes Jun 22, 2021

View reviewed changes

Swiftb0y merged commit 5c20663 into mixxxdj:main Jun 22, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

QML: Add support for selecting/loading effects #4000

QML: Add support for selecting/loading effects #4000

Holzhaus commented Jun 15, 2021

Swiftb0y commented Jun 15, 2021

Holzhaus commented Jun 15, 2021 •

edited

Loading

Swiftb0y left a comment

Swiftb0y Jun 15, 2021

Holzhaus Jun 15, 2021 •

edited

Loading

Swiftb0y Jun 15, 2021

Holzhaus Jun 15, 2021

Swiftb0y Jun 15, 2021

Holzhaus Jun 15, 2021

Swiftb0y Jun 15, 2021

Swiftb0y commented Jun 15, 2021

Swiftb0y commented Jun 16, 2021

Holzhaus commented Jun 16, 2021

Swiftb0y left a comment •

edited

Loading

Holzhaus commented Jun 21, 2021

Holzhaus commented Jun 21, 2021 •

edited

Loading

Swiftb0y left a comment

Swiftb0y Jun 21, 2021

Holzhaus Jun 21, 2021

Holzhaus Jun 21, 2021

Swiftb0y Jun 21, 2021

Swiftb0y Jun 21, 2021

Holzhaus Jun 21, 2021

Swiftb0y Jun 21, 2021

Holzhaus Jun 21, 2021

Swiftb0y Jun 21, 2021

Swiftb0y left a comment

Holzhaus commented Jun 22, 2021

Swiftb0y left a comment

ywwg commented Jun 28, 2021 •

edited

Loading

ywwg commented Jun 28, 2021

	readonly property string group: slot.group
	property alias group: slot.group

QML: Add support for selecting/loading effects #4000

QML: Add support for selecting/loading effects #4000

Conversation

Holzhaus commented Jun 15, 2021

Swiftb0y commented Jun 15, 2021

Holzhaus commented Jun 15, 2021 • edited Loading

Swiftb0y left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Holzhaus Jun 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Swiftb0y commented Jun 15, 2021

Swiftb0y commented Jun 16, 2021

Holzhaus commented Jun 16, 2021

Swiftb0y left a comment • edited Loading

Choose a reason for hiding this comment

Holzhaus commented Jun 21, 2021

Holzhaus commented Jun 21, 2021 • edited Loading

Swiftb0y left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Swiftb0y left a comment

Choose a reason for hiding this comment

Holzhaus commented Jun 22, 2021

Swiftb0y left a comment

Choose a reason for hiding this comment

ywwg commented Jun 28, 2021 • edited Loading

ywwg commented Jun 28, 2021

Holzhaus commented Jun 15, 2021 •

edited

Loading

Holzhaus Jun 15, 2021 •

edited

Loading

Swiftb0y left a comment •

edited

Loading

Holzhaus commented Jun 21, 2021 •

edited

Loading

ywwg commented Jun 28, 2021 •

edited

Loading