split Components JS library into its own file #1162
Conversation
and create lib directory in res/controllers
|
ping @ferranpujolcamins and @timrae |
to prepare for being able to register MIDI input handlers from JS
| @@ -20,6 +20,10 @@ print = function(string) { | |||
| engine.log(string); | |||
| } | |||
|
|
|||
| var debugObj = function (obj) { | |||
| print(JSON.stringify(obj, null, 2)); | |||
| }; | |||
There was a problem hiding this comment.
What about printObject to be parallel with print right above it?
There was a problem hiding this comment.
SGTM. I'll change it.
| undefined !== this.outCo && | ||
| undefined !== this.output && | ||
| typeof this.output === 'function') { | ||
| this.connections[0] = engine.connectControl(this.group, this.outCo, this.output); |
There was a problem hiding this comment.
OT: This is an annoying detail of our C++ code style -- 4 space indent plus hanging indent leads to these sorts of formattings where the block is at the same indent as the guards. I wonder if our Javascript style is still nascent enough that we could switch to 2-space indent? I'd like to do the same for C++ someday.
There was a problem hiding this comment.
I'm not too partial either way about 2 spaces or 4 spaces, but I don't see how switching to 2 spaces would solve this particular issue. Do you have a proposal for a consistent rule for indenting in this situation?
There was a problem hiding this comment.
Two space indent puts the guards that are hanging-indented from the if statement at a different indent level than the if statement's block.
if (foo &&
bar &&
baz) {
code();
}There was a problem hiding this comment.
Also, I don't like this clunky undocumented syntax for storing callback connection objects. I may change it in the future to use a more JavaScriptish convention (like new Connection(group, co)), but considering it's abstracted away by the library that's not very important.
There was a problem hiding this comment.
However, these connection objects should have a trigger() method. Currently the only way to trigger them is with engine.trigger(group, co), and I don't know how that behaves if multiple callbacks are connected to the same group & CO.
There was a problem hiding this comment.
IIRC you can't do that (you can't create a C++-backed object from Javascript) so you'll always need to ask the engine to do that for you. You can make a JS wrapper around it though (maybe that's what you meant).
There was a problem hiding this comment.
I don't know how that behaves if multiple callbacks are connected to the same group & CO.
It'll call them both. Would a trigger method on the connection object just trigger that handler or should it trigger the control (which will trigger all the connected handlers)?
|
I haven't found the time to look at this until now -- cool stuff! Some high levels comments:
var Controls = function() {
// define the whole library in here
var exports = {
Control: ...,
Button: ...,
...
}
return exports;
}(); The way we do "imports" in script is pretty janky right now, since we eval the file in the global scope, so it's possible the user could break something by redefining a variable that's internal to the library. Or two scripts that define the same symbol could conflict if they were both imported. A little while ago I started work on supporting a NodeJS-style require function (rryan@2458fde) for importing modules. This would also make it easy to switch to that once it's ready since instead of returning exports the code would set |
| }, | ||
| }; | ||
|
|
||
| script.samplerRegEx = /\[Sampler(\d+)\]/ ; |
There was a problem hiding this comment.
I don't think you should change other libraries from here :). Want to move these to the common scripts or make them a local variable?
There was a problem hiding this comment.
Yes, I meant to move those but forgot about it.
Oh yea, I totally forgot Deck and Control were already defined in common-controller-scripts.js :). |
|
Nice! I'll have a deeper look ASAP and finish the Xone K2 4-fx mapping with it. |
|
I agree that this should be wrapped up in a big object for the library as is conventional for JS libraries. However, that could get awkward with the The problem with semver naming with the present format is that every mapping's XML file would have to be updated to use the new filename to make use of updates until we have a better way to import libraries. Maybe we could use MAJOR.MINOR instead of MAJOR.MINOR.PATCH?
Yep, currently this works by overwriting those undocumented (and as far as I can tell, unused) objects on common-controller-scripts.js. |
Yea, the patch version may be overkill. major.minor probably gets most of the benefit
I believe they are e.g. |
(though it doesn't matter if they don't import both -- this is more about future conflicts) |
Lower-case speaks module name to me and CamelCase is for classes so I don't think I'd be confused by |
|
The glaring omission from the library is a Jogwheel object. I don't have any controllers with jog wheels, so if anyone wants to contribute that, please do. |
Be-ing
force-pushed
the
hercules_p32_mapping_for_2.1
branch
from
d4a6d84
to
4104794
Compare
|
Before we can make a generic jog wheel object we probably need to either expose the latency, or ideally fix the engine so jogging doesn't depend on it anymore. |
|
(Currently the jog sensitivity depends the latency setting, it's a bit awkward) |
That doesn't need to be fixed before we have a Jogwheel object in the library. When that is is fixed in the engine, it would be great if it could be done in a way that keeps the library's API backwards compatible. |
|
ping @arximboldi and @pwhelan |
|
Ack, Mixxx copies the library files from res/controllers/lib to ~/.mixxx/controllers. Not pretty. |
and get rid of the res/controllers/lib directory. Mixxx was copying files from res/controllers/lib to ~/.mixxx/controllers/ without creating a lib directory in ~/.mixxx/controllers, creating confusing issues
Hm -- maybe calling the 2nd part of the ConfigKey a control is itself confusing, since a control is identified by a (group, item) pair. (though the ConfigKey name of "item" for the 2nd part of the pair is a pretty terrible name). Doesn't the same confusion apply with inCo/outCo, except it's obscured by the indirection of having to know that 'co' refers to ControlObject in order to be confused? BTW some day we'll rename ControlObject to just Control in the C++ side of things (someone was very OOP happy in the early 2000s and wanted to stick Object on the end of everything just to confer extra "object"-ness upon them :) ). |
Yes, it does. Fresh ideas are welcome. |
|
How about Component instead of Control? |
|
Name? |
|
inKey/outKey seems good. It's a little confusing with the ConfigKey C++ objects that combine group and key, but I don't think that's a big deal. I'm leaning towards renaming the library and the Control object to Component unless anyone has a better suggestion |
I don't feel strongly -- Control seemed fine to me though since it helps you interact with Mixxx's control system. |
Yes, but it's confusing because there is not a 1:1 relationship between the JS objects and Mixxx's ControlObjects. |
👍 I updated the wiki page to have a little intro that uses (group, key): |
Be-ing
changed the title
Be-ing
force-pushed
the
hercules_p32_mapping_for_2.1
branch
from
1465148
to
274048d
Compare
|
I am happy with the names now. Unless anyone has any more name suggestions, I think this is ready for merge. I have been continuing to work on the documentation. Hopefully it is a more approachable for people who don't know JS well. |
|
ping @illuusio |
|
This looks very interesting @Be-ing, nice move! Before getting into the details: I see there are lots of moving parts still, would you like me to review the code or comment on the overall design? |
|
I'm more interested in comments about the design, but if you want to review the details of the code that would be appreciated too. |
Thanks for considering my naming nits. If anyone has comments to rename or otherwise change things we can continue to do so directly to v0.0 until 2.1 is released. |
|
Any plans for introducing support for button holding, double presses, and modifiers other than shift? |
|
By "button holding", do you mean doing an alternative action after holding a button down for a specified amount of time, or do you mean activating a ControlObject only while a button is held down? If you mean the latter, that's possible by setting the Button's onlyOnPress property to false. If you mean the former, I don't have any immediate plans for implementing that, but if you want to add that to the library, go for it. You may want to read through this wiki page for some tips on that. I don't have plans for implementing double presses either, but again, you're welcome to contribute that. The Reloop Beatpad mapping implemented both of those, but it was done in a way that I don't think fits very well with how Components is designed. For most use cases that just require changing the value of one inKey for a regular press and another inKey for a hold or double press, the API should be simple and not require writing any custom functions. Button.prototype.input could be modified to execute callback functions that are properties of the Button object. You could provide default functions for those that would toggle the alternate inKey similar to how Button.prototype.input works. Then, if someone wanted to implement a complex behavior as the long press or double press action, they'd just have to provide a custom callback function while the library would take care of the timer logic. Modifiers other than shift can already be implemented, I just haven't documented it very well yet. You can take a look at the source code for the default EffectUnit ComponentContainer for an example. That's a complex example that handles the interaction of two modes that each have alternate actions with shift. Basically the idea is to use ComponentContainer.forEachControl or ComponentContainer.reconnectControls to iterate over each Component in the ComponentContainer and change its properties however is necessary for the new layer. |
|
I meant the former: alternative action after holding a button down for a specified amount of time. I probably do not have time to implement these, but we'll see. Thanks for your work! |
|
Would there be any performance penalty for including more utilities into the lodash build? |
|
I doubt it would practically matter. What else do you want to include? |
|
Well... why not all of core for instance? |
|
Sure, why not? @rryan any opinion on that? |
Generally smaller -> better. But speed-wise I don't see why it would be slower unless you called the added functions (they're mostly adding helpers onto core type prototypes, right? not replacing builtins). It would use more memory -- but probably an unnoticeably larger amount (you could measure that pretty easily too). |
This will let others use the library now without having to copy and paste a bunch of code. The library is versioned, starting now with version 0. The version number can be incremented whenever a backwards incompatible API change is made. Controls as well as Lodash have been moved to a new
libdirectory inres/controllersto minimize clutter. The Controls library has been named to specify it is only for MIDI controllers for now. Hopefully in the future it will be ported to work with HID. The documentation has been moved to the wiki, allowing it to continue to be improved independently of the code.