ES6+ ComponentsJS

[christophehenry]

Superseeds #4550

This is an attempt at rewriting midi-components-0.0.js with modern JS and correct type hinting fr IDEs.

I'm starting small. Component by component in order to get regular feedbacks, starting with the general Component.

Some rationals guiding the development of the new API and design decisions (will be updated as development goes):

1. The API is developped for eas of use first. It priorizes readability and documentation with JSDoc to ease IDEs type hinting.
2. The Component's constructor doesn't not merge all of its parameters into this. This is to avoid multiple ways of overriding Component's behavior and easier documenting constructor parameters with JSDoc. If you need a more specific behavior, you should create a new class extending the original component.

[Swiftb0y]

Can you also add a dummy mapping that makes use of the components so the QJSEngine actually runs the code? I don't think it would work in the current form you're using features (static, private properties, spread syntax) not available in QJSEngine atm (~ES7 is the maximum IIRC). I filed QTBUG-97724 ages ago, but it doesn't seem like there is much interest in improving here.

[Swiftb0y]

Thank you for picking this up btw.

[Swiftb0y]

Can you explain a little more on why you think the promise model is suited well for timers here? From what I can tell, they would only work for one-shot timers. I know engine.beginTimer is clunky to use, but so is its direct foundation (window.setTimeout/window.setInterval) and I don't think JS overs anything better. Do you have a better idea of what constitutes a good timer API? I'm also not sure, but wasn't there a previous discussion on this already?

[christophehenry]

I am not aware of a previous discussion on this matter.

So the idea here is to offer a more modern and ES6+ way of using timers. Promises a particularly suited for this kind of task.

Of course, since Promises only resolve once, it makes more sense for one-shot timers. But the cancel() method I added also handle the case where the timer was already cancelled which has the advantage of avoiding the warning when trying to engine.stopTimer() a one-shot that was already executed. This warning message really bothers me.

Of course, if this is too controversial, I'll remove this. But I still think it's a nice addition.

[Swiftb0y]

We prefer the _ prefix tactic instead. Or do you have some rationale why the eslint-disable is better here?

Suggested change

	// eslint-disable-next-line no-unused-vars
	isPress(channel, control, value, status) {
	return value > 0;
	}
	isPress(_channel, _control, value, _status) {
	return value > 0;
	}

[christophehenry]

The problem with the _ prefix strategy is that it propagates when you override the method using IDEs code gen like Webstorm's. IMO the one _ strategy is suited for end users of an API but not when designing the API. That a parameter maybe unused in the base class is not relevant since these methods are designed to be overriden.

[Swiftb0y]

I know this is a nitpick, but a noop is not the same as a mathematical identity function. The latter is useful in mathematical contexts, the former is code smell (using a ternary merely for its sideffects is one as well). KISS.

Suggested change

	this.outTrigger ? this.trigger() : this.noop();
	}
	}
	noop() { /* Does nothing */ }
	if (this.outTrigger) {
	this.trigger();
	}
	}
	}

[christophehenry]

I don't agree but I won't fight over personal taste just to gain 2 SLOC. I'm ok with changing this 😉

[Swiftb0y]

I may be misinterpreting the code, but from what I could tell this abstraction would only call the callback after it has collected all the events for this.longPressTimeout milliseconds. That means that even simple button presses where you just want to run code once the button has been pressed (not even waiting for the release) would introduce this huge artificial latency. This is not an option, input latency has to be reduced to a sensible minimum (essentially as much as possible, though I wouldn't sacrifice other aspects such as code quality for it).

[christophehenry]

Yes, I agree. Which is why this code won't execute if the onLongPress and onDoublePress methods are not overrided. Plus, the new onEvent() is always called instantly.

Sadly, as of now, it's not possible to bind several scripts to the same (midiStatus, midiNo) tuple. Which means you must use the same component if you want to use several interaction to the same button. I think it's an acceptable tradeoff.

Or we could modify the engine to execute multiple callbacks for the same midi signal.

[christophehenry]

Ok, reflecting about this, I think I may have a more reactive algorithm for this feature. It goes like this:

• the event bus recieves a push event:
  • if the event is the first detected, wait 50ms
  • after 50 ms,
    • if the bus received a release event, we're either on a single or double push, wait another 50ms,
      • if the bus didn't receive a second push event, we're on a single push, call the dispatch callback signaling a single push, reset the bus, end of cycle, max reaction time: 100ms.
      • if the bus received a second push event, we're on a double push, call the dispatch callback signaling a double-push, reset the bus, end of cycle,max reaction time: 100ms.
    • if the bus didn't receive a release event, wait until the end of longPressTimeout, call the dispatch callback signaling a long push, reset the bus.
    • if a release event is received in between, call the dispatch callback signaling a long push, reset the bus.

Would that algorithm be more acceptable?

Maybe the 50ms window should be parametrable somewhere for accessibility, IDK. BTW, the longPressTimeout could also be Mixxx-wide parametrable instead of component-level. Is it feasible?

[Swiftb0y]

This strictly typed approach looses the feature where the user can add extra properties to the type when instantiating it. I think that is one of the strengths in QML and components-0.0. I fear the usage in mappings will get too clunky if we remove that feature. See my reply on zulip where I listed a bunch of usage examples where I used that pattern to avoid more broadly scoped (global) variables.

[christophehenry]

IMO, using the constructor parameter to override any component method defeats the purpose of using a class system to easier IDEs code completion and type hinting. If a user wants to override a component's methods without explicitly declaring a class, they can write the following:

const comp = new class extends components.Component {
    constructor(opts) {
        super(opts);
        this.isSomething = false;
    }
    inValueScale(value) {
        return value === 0x40 ? true : false;
    }
}({midiStatus: 0x70, midiNo: 0x40}

Personally, I'm in favor of this more explicit approach.

[christophehenry]

Closing in favor of #13459.