search

yastefan / grandMA3-Chataigne-Module

A Chataigne module to control grandMA3 via OSC
GNU General Public License v3.0
23 stars 7 forks source link

Update grandMA3.js #10

Closed 7heMech closed 1 year ago

7heMech commented 1 year ago

Removed a lot of repetition, and used some modern JS and hopefully cleaned up the code. Check it out and tell me what ya think.

yastefan commented 1 year ago

Hey 7heMech,

thanks for your contribution, I also like the modern ES6+ features. I chose oldschool JS at that time because Chataigne has only a simplitized JS interpreter [1]. Inspired by benkuper's XTouchMini plugin [2] I decided to go for the more secure variant. I tested the plugin in Windows 10, macOS and on a PI and had no problems so far. On which platform did you test your code? I can imagine switching to modern JS, but we should check if the JUCE JS interpreter can interpret modern JS on all platforms

[1] https://bkuperberg.gitbook.io/chataigne-docs/scripting/scripting-reference [2] https://github.com/benkuper/XTouch-Mini-Chataigne-module/blob/main/XTouchMini.js

7heMech commented 1 year ago

Hey, @yastefan It seems that you are indeed right, I hadn't checked compatibility with other platforms than my own, I checked out the source code of the JUCE interpreter which Chataigne uses, and they don't have full support for ES6+. Fortunately, most of the improvements in this pr aren't related to it. I'll add a new commit which makes it compatible for all platforms.

7heMech commented 1 year ago

@yastefan can you test if it works now, I think I removed the es6+ stuff

yastefan commented 1 year ago

Sorry for my late reaction. I have reviewed your code and don't want to merge it like this. The avoidance of code dublication by a set function I think is good and we should incorporate that into the code. Another function that builds the string prefix I can also imagine to avoid the repeated comparison statements for page 0. I can understand this one-liner var prefix = page === 0 ? '' :/Page${page};, but I think it makes the code less readable.

Changes to function definitions like this function sendControlMessage(page, type, executor, offset, value) should also be avoided, because in Chataigne as a target you can directly trigger functions as callback and we might break existing setups of users.

Template literals are a very nice feature, but I would also count them to ES6+ features and therefore rather avoid them.

I hope it's ok for you if I reject the merge request and we make a lean one by avoiding only the code publication. Currently I can't see if we create any edge cases

7heMech commented 1 year ago

Yeah, that's fine.