Fix led_strip overlay #2362
Fix led_strip overlay #2362
Conversation
|
Can you please explain why all of this is required to fix what seems to be a relatively small bug? |
haslinghuis
force-pushed
the
fix-ledstrip
branch
3 times, most recently
from
9d503df to
d94f015
Compare
haslinghuis
force-pushed
the
fix-ledstrip
branch
2 times, most recently
from
668455b to
1b0197d
Compare
src/js/msp/MSPHelper.js
Outdated
| mask |= (led.x << 4); | ||
|
|
||
| for (const functionLetterIndex of led.functions) { | ||
| for (const functionLetterIndex in led.functions) { |
There was a problem hiding this comment.
Do we want to include all non-array properties of led.functions in this? If not then this is wrong, and the line below should be changed.
There was a problem hiding this comment.
Changed back to standard for loop is much more clear.
There was a problem hiding this comment.
Using for ( of ) would be perfectly fine in these cases:
for (const directionLetter of led.directions) {
 const bitIndex = ledDirectionLetters.indexOf(directionLetter);
 if (bitIndex >= 0) {
 directionMask = bit_set(directionMask, bitIndex);
}
}
However, for ( in ) is not ok, as it iterates over a different set.
There was a problem hiding this comment.
Somehow for ( of ) didn't work here, and for ( in ) did, but it's recommended not to use it as index while iterating over an array:
https://stackoverflow.com/questions/500504/why-is-using-for-in-for-array-iteration-a-bad-idea
src/js/msp/MSPHelper.js
Outdated
| } | ||
|
|
||
| for (const overlayLetterIndex of led.functions) { | ||
| for (const overlayLetterIndex in led.functions) { |
src/js/msp/MSPHelper.js
Outdated
| mask |= (led.color << 18); | ||
|
|
||
| for (const directionLetterIndex of led.directions) { | ||
| for (const directionLetterIndex in led.directions) { |
haslinghuis
force-pushed
the
fix-ledstrip
branch
from
1b0197d to
8e8a15c
Compare
|
Kudos, SonarCloud Quality Gate passed!
|
2 participants
Fixes: #2359