nexus-js/ui

Sequencer's 'change' event triggered even if value does not change

tbazin opened this issue · 6 comments

tbazin commented

ui/lib/interfaces/sequencer.js

Lines 313 to 326 in 3546e19

keyChange(note, on) {
// emit data for any key turning on/off
// i is the note index
// v is whether it is on or off
let cell = this.matrix.locate(note);
// this.matrix.set.cell(cell.column,cell.row,on);
this.matrix.pattern[cell.row][cell.column] = on;
var data = {
row: cell.row,
column: cell.column,
state: on
};
this.emit('change', data);
}

This does not check if the new value in on is actually identical to the previously stored value (e.g. when passing multiple times over a given cell during a single Touch action), in which case the semantics of the 'change' event is a bit broken since nothing actually changes on the interface.
Is this desired behaviour? Or should this be changed? (In which case the fix is trivial.)

Cheers!

mscottnelson commented

I think one could argue that it should only emit on actual changes, and that there would be a separate “data” event for capturing raw unfiltered data. That’s an active maintainer decision, but I think this one is unlikely to change without a major version update since it’s such fundamental behavior and folks are likely to depend on the current implementation.

However, the documentation seems to imply the scenario you expect. So it is either a bug or the documentation needs to be updated. Without knowing how many projects depend on the current behavior, I think I would vote for updating the documentation and making a “only when values actually change event” a feature request under some other name, like toggle.

In the meantime, you will need to filter duplicates yourself if this is an issue for you.

tbazin commented

Yep, I expected something of the sort given the fact that this could introduce breaking behavior changes.
I created a tiny subclass to suit my needs for now.

As far as the nexus/ui codebase is concerned, editing the docs to better reflect the actual behavior indeed seems like a reasonable compromise.

tatecarson commented

I am a little bit confused about how to update the documentation to make it more clear. It fires when clicking on the cell to turn it on or off. What language would make this more clear?

tbazin commented
mscottnelson commented

Within the context of the function, this will basically fire with any interaction, regardless of whether there is a value change or not. It applies in the mouse case mentioned, but could be relevant for any number of things including programmatic control of an interface.

I think the doc change would be something like “fires whenever a value is received, for example when clicking a cell from off to on.”

tatecarson commented

Ok, thanks for the help clearing that up. I have updated the docs to reflect what was discussed.

  • 👍1