Components ES6 rewrite #4171

Be-ing · 2021-08-01T19:35:16Z

This is a stopgap measure to improve the developer experience until we rewrite the controller system. The rewrite is in a separate file to avoid breaking old mappings.

coveralls · 2021-08-01T20:08:13Z

Pull Request Test Coverage Report for Build 1088081279

0 of 0 changed or added relevant lines in 0 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.003%) to 26.014%

Totals
Change from base Build 1087078133:	0.003%
Covered Lines:	20005
Relevant Lines:	76902

💛 - Coveralls

Be-ing · 2021-08-01T20:33:53Z

res/controllers/Allen-and-Heath-Xone-K2-scripts.js

 };
-XoneK2.EffectUnit.prototype = new components.ComponentContainer();


no more of this gooblygook :)

Swiftb0y

Didn't take look at the changes for the K2 as well the EffectUnit yet. This is already plenty to discuss for now.

res/controllers/midi-components-0.1.js

res/controllers/Allen and Heath Xone K2.midi.xml

res/controllers/midi-components-0.1.js

Swiftb0y · 2021-08-02T15:32:02Z

res/controllers/midi-components-0.1.js

+            this.input = function(channel, control, value, status, _group) {
+                if (this.isPress(channel, control, value, status)) {
+                    if (engine.getValue(this.group, "track_loaded") === 0) {
+                        engine.setValue(this.group, "LoadSelectedTrack", 1);
+                    } else {
+                        if (this.volumeByVelocity) {
+                            engine.setValue(this.group, "volume", this.inValueScale(value));
+                        }
+                        engine.setValue(this.group, "cue_gotoandplay", 1);
+                    }
+                }
+            };


I know this is from components-0.0 but I'm confused why this even worked back then. If I understand JS correctly, the this of the new handler does not refer to the SamplerButton instance but the function object instance being created?

res/controllers/midi-components-0.1.js

Swiftb0y · 2021-08-02T18:23:02Z

res/controllers/midi-components-0.1.js

+        forEachComponent(operation, recursive) {
+            if (typeof operation !== "function") {
+                throw "ComponentContainer.forEachComponent requires a function argument";
+            }
+            if (recursive === undefined) { recursive = true; }
+
+            var that = this;
+            var applyOperationTo = function(obj) {
+                if (obj instanceof Component) {
+                    operation.call(that, obj);
+                } else if (recursive && obj instanceof ComponentContainer) {
+                    obj.forEachComponent(operation);
+                } else if (Array.isArray(obj)) {
+                    obj.forEach(function(element) {
+                        applyOperationTo(element);
+                    });
+                }
+            };
+
+            for (var memberName in this) {
+                if (ComponentContainer.prototype.hasOwnProperty.call(this, memberName)) {
+                    applyOperationTo(this[memberName]);
+                }
+            }
+        }


What do you think about using a generator? This way the consuming code could look a little more easy to understand for beginners. Eg:

for (const comp of container.iter()) { // insert function body here }

I'd be willing to submit a PR implementing it if you don't want to bother researching generators first.

Swiftb0y · 2021-08-02T18:30:00Z

res/controllers/midi-components-0.1.js

+                    if (component instanceof Button
+                        && (component.type === Button.prototype.types.push
+                            || component.type === undefined)
+                        && component.input === Button.prototype.input
+                        && typeof component.inKey === "string"
+                        && typeof component.group === "string") {
+                        if (engine.getValue(component.group, component.inKey) !== 0) {
+                            engine.setValue(component.group, component.inKey, 0);
+                        }


shouldn't the button component be responsible for handling this special case?

Swiftb0y · 2021-08-02T19:01:05Z

res/controllers/midi-components-0.1.js

+    var EffectUnit = class extends ComponentContainer {
+        constructor(unitNumbers, allowFocusWhenParametersHidden, colors) {


This is very convoluted. I'll be looking at the entire EffectUnit code some other time.

There is no other time. Once we merge this, we can't change it. Or maybe we can, but when beta period starts we will have to stick with this API forever.

FWIW it sucks that JS has no keyword args like Python. We sometimes take an options object, sometimes individual parameters. Can we use one style consistently?

There is no other time. Once we merge this, we can't change it.

I meant some other time in the future before merging. I just wanted to submit the comments I had so far so Be does not have to wait 3 weeks for a review.

Swiftb0y · 2021-08-02T19:06:14Z

Also, we should introduce documentation using JSDoc comments now.

Holzhaus

Thanks. Some initial comments.

Holzhaus · 2021-08-02T20:18:01Z

res/controllers/midi-components-0.1.js

+            by calling this.disconnect(). This can be helpful for multicolor LEDs that show a
+            different color depending on the state of different Mixxx COs. Refer to
+            SamplerButton.connect() and SamplerButton.output() for an example.
+            **/


Can we use the same commenting style as we do for CPP code, i.e. move the comment out of the body and put it above the function. And maybe use ///. Not sure if that works with doxygen.

What do you think about using JSDoc in JS sources instead?

Holzhaus · 2021-08-02T20:19:10Z

res/controllers/midi-components-0.1.js

+            if (this.midi === undefined || this.midi[0] === undefined || this.midi[1] === undefined) {
+                return;
+            }


Is this expected or an error? If the latter we should throw an exception.

Holzhaus · 2021-08-02T20:21:03Z

res/controllers/midi-components-0.1.js

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


type can never be undefined right? Please use a switch case and throw an error in the default case.

This is JavaScript. Everything and anything can be undefined.

It is defined in the constructor though

res/controllers/midi-components-0.1.js

Holzhaus · 2021-08-02T20:25:25Z

res/controllers/midi-components-0.1.js

+            // Sends the color from the colorCode to the controller. This
+            // method will not be called if no colorKey has been specified.
+            if (colorCode === undefined || colorCode < 0 || colorCode > 0xFFFFFF) {
+                print("Ignoring invalid color code '" + colorCode + "' in outputColor()");


This should thow an exception IMHO. Because it cannot happen unless the user makes a mistake in the JS code.

Holzhaus · 2021-08-02T20:44:18Z

res/controllers/midi-components-0.1.js

+    var EffectUnit = class extends ComponentContainer {
+        constructor(unitNumbers, allowFocusWhenParametersHidden, colors) {


There is no other time. Once we merge this, we can't change it. Or maybe we can, but when beta period starts we will have to stick with this API forever.

Holzhaus · 2021-08-02T20:45:58Z

res/controllers/midi-components-0.1.js

+    var EffectUnit = class extends ComponentContainer {
+        constructor(unitNumbers, allowFocusWhenParametersHidden, colors) {


FWIW it sucks that JS has no keyword args like Python. We sometimes take an options object, sometimes individual parameters. Can we use one style consistently?

Holzhaus · 2021-08-02T20:48:30Z

res/controllers/midi-components-0.1.js

+            this.focusChooseModeActive = false;
+
+            // This is only connected if allowFocusWhenParametersHidden is false.
+            this.onShowParametersChange = function(value) {


Shouldn't this be a method instead of function expression property? (also applies to the rest of the code)

Holzhaus · 2021-08-02T20:49:41Z

res/controllers/midi-components-0.1.js

+
+                // Do not enable soft takeover upon EffectUnit construction
+                // so initial values can be loaded from knobs.
+                if (this.hasInitialized === true) {


isInitialized?

Holzhaus · 2021-08-02T20:50:17Z

res/controllers/midi-components-0.1.js

+            });
+            this.effectFocusButton.setColor();
+
+            this.init = function() {


where is this called? why is is this not part of the constructor?

Be-ing · 2021-08-03T07:50:45Z

To clarify my intention with this PR, I only intend to refactor the existing Components library so it can be used with ES6 classes without copying and pasting arcane JS magic from ~~Stack Overflow~~ the wiki. I am open to making some minor API changes and code cleanup as well but I am not looking to do a major overhaul of the library right now.

Swiftb0y · 2021-08-03T08:00:16Z

It seems you have forgotten to run pre-commit.

Be-ing · 2021-08-03T08:01:35Z

I have not forgotten. That was intentional, otherwise the Xone K2 commit would be cluttered with a bunch of unrelated automated code cleanup.

Be-ing · 2021-08-03T08:04:11Z

I included the Xone K2 commit in this PR as an example of how to use the modified library. It is not the focus here. If you want, I can split it to another PR.

Swiftb0y · 2021-08-03T08:31:04Z

I included the Xone K2 commit in this PR as an example of how to use the modified library. It is not the focus here. If you want, I can split it to another PR.

I guess its useful to develop alongside of... though I'd prefer to merge it separate. I don't know how that would be achieved best with Git though...

github-actions · 2021-11-03T00:31:39Z

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

Components: rewrite with ES6 classes in version 0.1

04df2b3

github-actions bot added the controller mappings label Aug 1, 2021

Be-ing requested review from Holzhaus and Swiftb0y August 1, 2021 19:35

A&H Xone K2: rewrite using ES6 classes with Components 0.1

3e8c5b4

Be-ing force-pushed the components_es6 branch from 9c71b87 to 3e8c5b4 Compare August 1, 2021 20:32

Be-ing commented Aug 1, 2021

View reviewed changes

Swiftb0y requested changes Aug 2, 2021

View reviewed changes

Holzhaus reviewed Aug 2, 2021

View reviewed changes

Swiftb0y mentioned this pull request Aug 4, 2021

ControllerScriptEngineBase: install console extension #4174

Merged

github-actions bot added the stale Stale issues that haven't been updated for a long time. label Nov 3, 2021

Be-ing mentioned this pull request Nov 20, 2021

Minimixxx controller config #4350

Merged

Be-ing closed this Nov 30, 2021

Swiftb0y mentioned this pull request Dec 5, 2021

Components ES6 rewrite with improvements #4550

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Components ES6 rewrite #4171

Components ES6 rewrite #4171

Be-ing commented Aug 1, 2021

coveralls commented Aug 1, 2021 •

edited

Loading

Be-ing Aug 1, 2021

Swiftb0y left a comment

Swiftb0y Aug 2, 2021

Swiftb0y Aug 2, 2021

Swiftb0y Aug 2, 2021

Be-ing Aug 2, 2021

Swiftb0y Aug 2, 2021

Swiftb0y Aug 2, 2021

Holzhaus Aug 2, 2021

Holzhaus Aug 2, 2021

Swiftb0y Aug 3, 2021

Swiftb0y commented Aug 2, 2021

Holzhaus left a comment

Holzhaus Aug 2, 2021

Swiftb0y Aug 3, 2021

Holzhaus Aug 2, 2021

Holzhaus Aug 2, 2021

Be-ing Aug 2, 2021

Holzhaus Aug 2, 2021

Holzhaus Aug 2, 2021

Holzhaus Aug 2, 2021

Holzhaus Aug 2, 2021

Holzhaus Aug 2, 2021

Holzhaus Aug 2, 2021

Holzhaus Aug 2, 2021

Be-ing commented Aug 3, 2021

Swiftb0y commented Aug 3, 2021

Be-ing commented Aug 3, 2021

Be-ing commented Aug 3, 2021

Swiftb0y commented Aug 3, 2021

github-actions bot commented Nov 3, 2021

		};
		XoneK2.EffectUnit.prototype = new components.ComponentContainer();

		var EffectUnit = class extends ComponentContainer {
		constructor(unitNumbers, allowFocusWhenParametersHidden, colors) {

Components ES6 rewrite #4171

Components ES6 rewrite #4171

Conversation

Be-ing commented Aug 1, 2021

coveralls commented Aug 1, 2021 • edited Loading

Pull Request Test Coverage Report for Build 1088081279

💛 - Coveralls

Choose a reason for hiding this comment

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 commented Aug 2, 2021

Holzhaus 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Be-ing commented Aug 3, 2021

Swiftb0y commented Aug 3, 2021

Be-ing commented Aug 3, 2021

Be-ing commented Aug 3, 2021

Swiftb0y commented Aug 3, 2021

github-actions bot commented Nov 3, 2021

coveralls commented Aug 1, 2021 •

edited

Loading