Components ES6 rewrite with improvements #4550

Swiftb0y · 2021-12-05T15:52:54Z

Reopen #4171 with Be-ing#56 on top.

In the old PR, there is still a review from @Holzhaus which has not been addressed, though I plan to take care when I find the time.

Make Button.types enum a static frozen/read-only property on the button object in an effort to reduce the exposure of the prototype

replace `function(){}.bind(this)` pattern with arrow functions where applicable.

ywwg · 2021-12-05T19:33:22Z

We should really be adding tests to our javascript corpus just like the rest of our codebase. Rather than commit this large PR all at once, I'd prefer to commit this a piece at a time, starting with just the base component class and accompanying tests that demonstrates its correctness, including communication with control objects and midi send/receive. As a side-benefit, the test would also help serve as a way for controller writers to see how the library is supposed to be used.

I had done some minor refactoring to make it easier to write controller tests here, but it was rejected because it was based on the old controller model: #3074. (But it also might be better to do the tests in native javascript, anyway)

Swiftb0y · 2021-12-05T20:34:08Z

IMO there are more pressing priorities for the controller right now. Before spending time adding the infrastructure to add tests, we should rather work on a newer mapping engine. Transitioning to QJSEngine was just the first part. Now we desperately need to get rid of the manual handler registration via XML and making ES6 Modules usable. From that point, adding tests is likely a matter of shipping a pre-made established library. However, before spending time on the controller engine, we should rather focus on Qt6 and thus the new QML UI...

So what I'm trying to say is that there are more pressing issues right now and thus I'd like to keep the scope of this PR still as small as possible. This is mostly intended as a follow up PR to the QJSEngine transition which gave us ES7 support, so our APIs should leverage that. The functional changes are very minimal. As a consequence, I'd really like to get this into 2.4 (if I manage to to finish it in time).

ywwg · 2021-12-05T20:41:18Z

Can you at least use git mv so it's clear what is the same as the old file and what's new? As it is now it looks like 1100 lines of new code and it's not really feasible to review that much at once. I can't just assume that it's a noop change without any sort of indication of testing or correctness.

ywwg · 2021-12-05T20:49:30Z

In general I'm not convinced by claims that we have "more important things to do than write tests." Code is code and if we want Mixxx to remain stable, all code needs to be tested. I spend a lot of time fixing regressions, and that all subtracts from whatever new feature work I might want to do. I am all for making ES6 classes usable, but if we're writing a library component that people are going to rely on, it needs some basic test coverage.

Be-ing · 2021-12-05T20:56:45Z

@ywwg Refusing to review code because it's above your arbitrary code size threshold while demanding to drastically expand the scope of PRs is toxic. Likewise, pointing fingers at people for introducing minor regressions while cleaning up old code is toxic and shows a disrespect for the hard work of others. If you think hacking a testing framework into a legacy architecture is more important than moving to Qt6, well, have fun taking care of a dying application that still depends on Qt5 when the world is moving onto Qt7.

Swiftb0y · 2021-12-05T20:58:29Z

Can you at least use git mv so it's clear what is the same as the old file and what's new?

We still have to ship the 0.0 version since many existing controller mappings depend on it.
The file here is a new version that got reiterated on (with some breaking changes since the new syntax is already a breaking change). using git mv does not make sense here. If you want to see the differences between the 0.0 and 0.1 version, I advise you to just diff the different files locally.

In general I'm not convinced by claims that we have "more important things to do than write tests." Code is code and if we want Mixxx to remain stable, all code needs to be tested.

I generally agree, please don't get me wrong, I was not arguing against testing. I was saying that introducing the ability to even write tests is not worth the effort right now. Especially considering that most of the code is already "battletested" from the previous iteration.

Swiftb0y · 2021-12-05T21:04:35Z

@Be-ing, while I appreciate you are voicing your opinion here, please be more respectful. Owen does not have any obligation to review this PR, if he doesn't want to spend the time on reviewing either someone else will review this or this PR will start rotting like the rest of the 120 PRs.
@ywwg I'd still very much appreciate your input on this though. This is the opportunity for breaking changes so this is where we could start integrating features from your own JS library.

Be-ing · 2021-12-05T21:06:37Z

Of course, nobody has an obligation to review any particular PR. What I am taking issue with is this behavior of commenting on a PR to explicitly refuse to review it while simultaneously demanding to drastically expand its scope. If you don't want to review it, just don't review it and leave it to people who are interested in it.

Be-ing · 2021-12-05T21:22:10Z

The new commits look fine to me. I leave it to @Holzhaus to follow up on his prior review.

Be-ing · 2021-12-05T21:23:01Z

By the way, thank you for the small, easy to review, self-contained commits with clear commit messages.

ywwg · 2021-12-06T21:04:56Z

@Swiftb0y , my worry is that it never seems to be a good time to write tests. I honestly can't think of a better time to do so then when we're rewriting a library -- the workings of the code is in your mind and there is nothing depending on it yet (right?). I know there's a lot more exciting things to do but at some point we need to pay down this technical debt.

As for the library itself, the main reason I wanted to write mine was to separate the action of a button from the midi control it is connected to. A button is two parts: the physical hardware with input and output, and the action that it takes and status that it gives. With my little controller, buttons needed to have up to four different modes for a single piece of hardware. I don't believe the current components library supports that level of indirection. (An example from a non-custom hardware -- the Reloop MIXTOUR has three different shift buttons, and knobs and buttons take different actions depending which one is held).

So in my library, the physical button can have a multitude of modes, and depending which layer is selected, the different modes will be active or ignored. The actual implementation leaves something to be desired, but it's more flexible than the components library.

ywwg · 2021-12-06T21:08:16Z

as for "increasing scope", I'm asking for the scope to be reduced -- Ideally we'd commit this file just starting with the base class and the button class and a few simple tests. then we add a few more subclasses.... and it would get pretty mechanical and move quickly. There is the initial hurdle to write the first test, but that will pay off for all of our js files that need testing

ywwg · 2021-12-06T21:09:00Z

something like https://jestjs.io/

Swiftb0y · 2021-12-06T21:23:30Z

my worry is that it never seems to be a good time to write tests.

I can understand that, but now is not only not the time but the arguments for doing it now have decreased weight:

I honestly can't think of a better time to do so then when we're rewriting a library -- the workings of the code is in your mind and there is nothing depending on it yet (right?).

Well this is not a full rewrite (the commit message is misleading). The behavioral aspects are the same (compare existing version with the new one). The main reason for this PR is that we modernize the API to use the new features language available with QJSEngine (replacing prototypical inheritance with class syntax, using arrow functions where applicable, using for...of instead of raw loops, etc). At the moment, there are 0 functional changes compared to the old library.

My point is that I don't want to force creators of new mappings to use old JS patterns because our library mandates it. It raises the barrier to entry for ComponentJS users. That is why I really want this PR to go into 2.4 (so its released together with QJSEngine).

If we change existing code to add new features (rather than just adding a new component or fixing obvious bugs), then I'm also in favor of the gradual, test-centric approach, but making this a requirement for this PR does not make sense.

Swiftb0y · 2021-12-06T21:24:45Z

Thanks for your input in regards to what you'd like to see in a newer ComponentsJS version, we will consider them when doing an actual rewrite of the code.

ywwg · 2021-12-06T21:27:47Z

ok, and I appreciate the work. Hopefully there is someone already familiar with this code who can review it.

uklotzde · 2021-12-06T21:51:59Z

I am not a JavaScript developer and do not qualify for reviewing this PR.

If someone is willing to set up tests for the JavaScript parts please do so instead of requesting them from someone else. It is out of scope of this PR.

ywwg · 2021-12-15T22:44:57Z

Given the lack of anyone else being able to review this... I will give it a try. When it's ready for review please mark it Ready For Review and I'll do my best. thank you.

Be-ing · 2021-12-16T02:12:24Z

So I guess my review doesn't count? Nor does @Holzhaus' review? @ywwg you have a history of belittling others' reviews. It's not okay. Writing "thank you" immediately after pretending like others' work doesn't exist comes across really badly.

Holzhaus · 2021-12-16T08:14:03Z

So I guess my review doesn't count? Nor does @Holzhaus' review? @ywwg you have a history of belittling others' reviews. It's not okay. Writing "thank you" immediately after pretending like others' work doesn't exist comes across really badly.

9 days ago, @ywwg said that he hopes someone finds the time to review this. The only reaction to that was @uklotzde who said he doesn't feel qualified to review it. Nobody reviewed it since then, and there is still 0 formal reviews on this PR.

So I agree with him that there currently is a "lack of people being able to review this", probably due to unfamiliarity with JS, time constraints, lack of motivation or other factors.

I appreciate that @ywwg wants to help to finish this PR and I don't feel belittled at all. His message sounded friendly to me.

Besides: 10 days ago you attacked @ywwg and said "Refusing to review code [...] is toxic" (which implies that you agree that another review makes sense btw). Now he agrees to review and you also attack him. I don't understand what you want.

Be-ing · 2021-12-16T09:26:47Z

I did review this yet @ywwg just posted "Given the lack of anyone else being able to review this". This is disrespectful.

I don't understand what you want.

I want @ywwg to stop insulting, gatekeeping, ignoring, and taking for granted others' work.

which implies that you agree that another review makes sense btw

Sure, another review could help if it is done respectfully, remains on topic/within scope, and makes an effort to understand context before passing judgement.

Swiftb0y · 2021-12-16T10:35:57Z

Since the majority of the code is from you @Be-ing, merging this PR on the basis that you reviewed it is almost equivalent to merging your own PR.
Thank you @ywwg for offering to review this. I guess its reviewable in its current state. Any change can be added in separate PRs before the 2.4 release.

Be-ing · 2021-12-16T10:57:08Z

Since the majority of the code is from you @Be-ing, merging this PR on the basis that you reviewed it is almost equivalent to merging your own PR.

Not really, since you've reviewed it too. Not every PR needs 3-5 people picking it apart.

Swiftb0y · 2021-12-16T11:08:02Z

I guess this is a gray area to be clarified in our contribution guidelines.

ywwg · 2021-12-16T22:08:18Z

ok I'll take a look!

ywwg

This is the type of review I would like to write... is this helpful? If this is a library we hope more people are going to use then I think we should work to make it friendlier. I find it hard to follow right now and I think some basic rearrangement, comment documentation, and renaming of some things will make it better.

res/controllers/midi-components-0.1.js

ywwg · 2021-12-16T22:29:23Z

res/controllers/midi-components-0.1.js

+    };
+    Component.prototype.max = 127;
+    Component.prototype.shiftOffset = 0;
+    Component.prototype.sendShifted = false;


I think this is where we should define this.midi so it's more clear how to use it, but perhaps this whole block should move to the top of the class. Again, my thinking is that we want to have the parts the programmer is most likely to change at the top of the file, and the internal guts at the bottom.

The issue is that there aren't really sensible default values for this.midi. Components might be used without outputting anything just to track some state or just for input handling (in the case for pots).
We could make this.midi explicit by Component.prototype.midi = [undefined, undefined] (essentially just declaring the property)?

Reorderings in general should be fine, sure.

this.midi can't be defined on the prototype. It is unique to each instance of Component.

It can be defined there, but each instance needs to overwrite it regardless, so defining it there does not make a whole lot of sense as already pointed out.

I once tried to move a property onto a prototype and have each instance override it, but that did not work because each instance was actually sharing the same data.

hm ok. This may just be a limitation of javascript then?

I once tried to move a property onto a prototype and have each instance override it, but that did not work because each instance was actually sharing the same data.

Thanks for the insight. This seems rather strange and conflicts with some of my JS understanding, so I'll try to recreate the issue and look into what exactly is wrong here.

I wouldn't say that's a limitation. That's just how prototypes work. If you don't want that, just set a normal property, not the prototype's property, which is what the code already does.

I would say only spend a little time trying to make it work. We can work on it more later if needed. My goal with this review is to make this library a little more organized and comprehensible to someone who is new to it (me), not make it perfect.

res/controllers/midi-components-0.1.js

ywwg

OK I've finished going through the file. The effect section is particularly complex and it's hard to know what's going on, so I'd like to flag that for eventual cleanup and testing. But mostly I think it shouldn't be too much work to make some noticeable improvements.

thanks again for working on this.

res/controllers/midi-components-0.1.js

ywwg · 2021-12-18T19:53:08Z

res/controllers/midi-components-0.1.js

+    };
+    Component.prototype.max = 127;
+    Component.prototype.shiftOffset = 0;
+    Component.prototype.sendShifted = false;


I would say only spend a little time trying to make it work. We can work on it more later if needed. My goal with this review is to make this library a little more organized and comprehensible to someone who is new to it (me), not make it perfect.

ywwg · 2021-12-18T19:53:59Z

res/controllers/midi-components-0.1.js

+            return value > 0;
+        }
+        input(channel, control, value, status, _group) {
+            if (this.type === undefined || this.type === Button.types.push) {


can this be done with a switch statement? I think that would be more readable.

res/controllers/midi-components-0.1.js

ywwg · 2021-12-18T20:19:25Z

res/controllers/midi-components-0.1.js

+            this.toggle = function() {
+                // cycle through unitNumbers array
+                let index = this.unitNumbers.indexOf(this.currentUnitNumber);
+                if (index === (this.unitNumbers.length - 1)) {


mod trick again

res/controllers/midi-components-0.1.js

ywwg · 2021-12-18T20:21:15Z

res/controllers/midi-components-0.1.js

+                        if (colors !== undefined) {
+                            this.color = colors.unfocused;
+                        }
+                        this.group = "[EffectRack1_EffectUnit" +


we see this building of the group name a few times -- can we factor that out into a function effectGroupName() or some sort?

I think this is the last bit of low-hanging fruit we can do now. Does that seem reasonable?

ywwg · 2021-12-18T20:22:52Z

res/controllers/midi-components-0.1.js

+                                }
+                                this.pressedWhenParametersHidden = true;
+                            }
+                        } else {


the nesting gets very deep here. Where possible, can you do

if { ... return; } ...

instead of:

if { ... } else { ... }

Sure, I'm also favoring early returns, this was just not my code to begin with, but I can change it now surely.

…dRGB

…ible

It was brittle to begin with and should rather be handled by writing a custom `inValueScale`.

ywwg

Thanks for discussing these issues with me. I resolved most of the issues and there's just a couple more things we talked about that should be easy to do. It's good to have a more solid idea of where we need to take this library to make it better!

ywwg · 2021-12-26T19:48:49Z

res/controllers/midi-components-0.1.js

+            super.connect();
+            if (this.group !== undefined && this.colorKey !== undefined) {
+                // TODO (Swiftb0y): replace with arrow function once https://bugreports.qt.io/browse/QTBUG-95677 got fixed
+                this.connections[1] = engine.makeConnection(this.group, this.colorKey, function(color) {


I am ok with making this a TODO as well -- we should come up with some way of handling connections that doesn't rely so much on callers doing the right thing.

ywwg · 2021-12-26T19:50:18Z

res/controllers/midi-components-0.1.js

+                        if (colors !== undefined) {
+                            this.color = colors.unfocused;
+                        }
+                        this.group = "[EffectRack1_EffectUnit" +


I think this is the last bit of low-hanging fruit we can do now. Does that seem reasonable?

github-actions · 2022-03-27T00:06:30Z

This PR is marked as stale because it has been open 90 days with no activity.

christophehenry · 2024-06-30T18:43:44Z

This looks abandonned. Is there a reason?

Swiftb0y · 2024-06-30T19:42:19Z

no time / interested from me atm. you're welcome to pick this up if you'd like.

github-actions bot added the controller mappings label Dec 5, 2021

Be-ing and others added 6 commits December 5, 2021 18:08

Components: rewrite with ES6 classes in version 0.1

0504c79

midi-components-0.1: "use-strict" for library

6550016

midi-components-0.1: improve Button.types API (breaking)

7204728

Make Button.types enum a static frozen/read-only property on the button object in an effort to reduce the exposure of the prototype

midi-components-0.1: use arrow func where possible

fa00f31

replace `function(){}.bind(this)` pattern with arrow functions where applicable.

midi-components-0.1: simplify Component.trigger() & .disconnect()

762775b

midi-components-0.1: reorder comparision with constants to be consistent

bb47432

Swiftb0y force-pushed the components_es6 branch from 2c74d29 to bb47432 Compare December 5, 2021 17:13

Swiftb0y marked this pull request as draft December 5, 2021 20:35

Swiftb0y marked this pull request as ready for review December 16, 2021 10:36

ywwg reviewed Dec 16, 2021

View reviewed changes

res/controllers/midi-components-0.1.js Outdated Show resolved Hide resolved

midi-components-0.1: update link to docs

ba58ef6

ywwg requested changes Dec 18, 2021

View reviewed changes

Swiftb0y added 8 commits December 22, 2021 20:05

midi-components-0.1: improve docs with additional comments

8d5e2f7

midi-components-0.1: replace else if with switch case

dce20b0

midi-components-0.1: improve error thrown in default HotcueButton#sen…

92576cb

…dRGB

midi-components-0.1: relax TODO ownership

d28de84

midi-components-0.1: improve array wrap-around logic

9913322

midi-components-0.1: replace nested ifs with early returns where poss…

1d7e97d

…ible

midi-components-0.1: add TODO comment about separating the effects

a1024c0

midi-components-0.1: remove Pot invert feature

f593a5d

It was brittle to begin with and should rather be handled by writing a custom `inValueScale`.

Swiftb0y requested a review from ywwg December 22, 2021 20:54

ywwg reviewed Dec 26, 2021

View reviewed changes

github-actions bot added the stale Stale issues that haven't been updated for a long time. label Mar 27, 2022

mixxxbot mentioned this pull request Aug 24, 2022

Get Components JS docs up-to-date #10618

Closed

github-actions bot removed the stale Stale issues that haven't been updated for a long time. label Sep 1, 2023

mxmilkiib mentioned this pull request Feb 17, 2024

Up to ES7? and related JS/mapping thoughts dszakallas/mixxx-launchpad#86

Open

ronso0 added the controller backend label Feb 20, 2024

christophehenry mentioned this pull request Jul 3, 2024

ES6+ ComponentsJS #13437

Closed

Swiftb0y mentioned this pull request Jul 7, 2024

Create a reactive programming API for controllers #13440

Open

daschuer marked this pull request as draft October 9, 2024 19:59

Components ES6 rewrite with improvements #4550

Are you sure you want to change the base?

Components ES6 rewrite with improvements #4550

Conversation

Swiftb0y commented Dec 5, 2021

ywwg commented Dec 5, 2021 • edited Loading

Swiftb0y commented Dec 5, 2021 • edited Loading

ywwg commented Dec 5, 2021

ywwg commented Dec 5, 2021

Be-ing commented Dec 5, 2021 • edited Loading

Swiftb0y commented Dec 5, 2021

Swiftb0y commented Dec 5, 2021

Be-ing commented Dec 5, 2021 • edited Loading

Be-ing commented Dec 5, 2021

Be-ing commented Dec 5, 2021

ywwg commented Dec 6, 2021 • edited Loading

ywwg commented Dec 6, 2021

ywwg commented Dec 6, 2021

Swiftb0y commented Dec 6, 2021

Swiftb0y commented Dec 6, 2021

ywwg commented Dec 6, 2021

uklotzde commented Dec 6, 2021 • edited Loading

ywwg commented Dec 15, 2021

Be-ing commented Dec 16, 2021 • edited Loading

Holzhaus commented Dec 16, 2021

Be-ing commented Dec 16, 2021 • edited Loading

Swiftb0y commented Dec 16, 2021

Be-ing commented Dec 16, 2021

Swiftb0y commented Dec 16, 2021

ywwg commented Dec 16, 2021

ywwg 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

Swiftb0y Dec 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ywwg 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

ywwg Dec 18, 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

ywwg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Mar 27, 2022

christophehenry commented Jun 30, 2024

Swiftb0y commented Jun 30, 2024

ywwg commented Dec 5, 2021 •

edited

Loading

Swiftb0y commented Dec 5, 2021 •

edited

Loading

Be-ing commented Dec 5, 2021 •

edited

Loading

Be-ing commented Dec 5, 2021 •

edited

Loading

ywwg commented Dec 6, 2021 •

edited

Loading

uklotzde commented Dec 6, 2021 •

edited

Loading

Be-ing commented Dec 16, 2021 •

edited

Loading

Be-ing commented Dec 16, 2021 •

edited

Loading

Swiftb0y Dec 17, 2021 •

edited

Loading

ywwg Dec 18, 2021 •

edited

Loading