Scriptable moodlamps #362
Scriptable moodlamps #362
Conversation
|
hmmm.... Not a huge fan. Looks like bringing a gun to a knife fight to me (adding a js engine for something as small as custom moodlights, which one person asked for and didn't reply to). |
|
It's limited API wize and does not have everything you'd have in a browser or nodejs, if that's your concern. It's pretty much the bare language with basic stuff like Math, Regex... (you can expose your own things to it from cpp side though) When I added the lamps originally I knew it wasn't a permanent thing, to me it always made more sense to have them configurable from outside, I just did not know how yet. Then I found QJSEngine and it looked like the best of both worlds:
So while it wasn't specifically in response to that ticket, it (and couple of other people I saw on reddit) motivated me for sure. And I don't take the dud(ett)e's no answer as a lack of interest, just put yourself in that person's shoes: you have this program for your LEDs and you wish you could light them up with the colors of your favorite sports team or whatever and to do so all you need is learn basics of git, install the whole toolchain, learn how those work, learn some cpp, re-read what that guy on github told you about the files of interest, ask more questions because it was too vague, do your lamp, learn how to do a pull request, fuck up your branches, clean up, do a pull request, wait couple of weeks for a feedback, things would need to be changed because it's the first time you touched cpp, wait more for final approval, wait more til your lamp makes to a release, ???, profit! The next logical steps with this to me would be:
The complexity of the latter will go beyond small lamp math (should be easier to convince people to use a documented set of JS functions than to make them find a language that can talk to a socket and then find a way to run it, would solve all the tickets about timed profile switching etc), and even lamps and soundviz depend on user's imagination so... This is why I think it's an ok investment.
|
Software/res/moodlamps/static.mjs
Outdated
| @@ -0,0 +1,9 @@ | |||
| export const name = "Static (default)" | |||
|
|
|||
| // note: in static or very slow animations it's better to keep the refresh interval under 500ms to avoid bugs | |||
There was a problem hiding this comment.
250 is also under 500. Did you have some bugs because of this? At some point I added FAKE_GRAB_INTERVAL because some devices turn off after no input in a second.
There was a problem hiding this comment.
it's been a while, I think the issue was that putting a too high of a value was causing lag when changing modes, so the lower the better
and probably also with "dont send data unless changed"
I'll double check
There was a problem hiding this comment.
"putting a too high of a value was causing lag when changing modes" - switching the mode should reset the timer, so if it does cause a delay, it would be worth checking out why.
There was a problem hiding this comment.
I'm only 99% sure about this, but I found and fixed the lag that occurred on lamp change: there was no call to update so you had to wait for the timer to fire
Software/src/MoodLampManager.cpp
Outdated
| if (m_isMoodLampEnabled && m_lamp) | ||
| m_timer.start(m_lamp->interval()); | ||
| if (m_isMoodLampEnabled && !m_jsLamp.isUndefined()) | ||
| m_timer.start(std::max(1, m_jsLamp.property("interval").toInt())); |
There was a problem hiding this comment.
If values larger than X cause errors, it would be nice to either log an error if it is, or add some logic to handle the case that updates the color without running the JS. Not everyone reads comments of other scripts ;)
There was a problem hiding this comment.
It was to avoid 0 or negative times. I opted for checking on load. Not sure if there's a need for an explicit arbitrary upper limit though (and how much?.. the value will always be within start()s range as is)
Software/src/Settings.cpp
Outdated
| { | ||
| DEBUG_LOW_LEVEL << Q_FUNC_INFO; | ||
| return value(Profile::Key::MoodLamp::Lamp).toInt(); | ||
| const QString& val = value(Profile::Key::MoodLamp::Lamp).toString(); | ||
| return val.isEmpty() || val.contains(QRegularExpression("^\\d+$")) ? Profile::MoodLamp::LampDefault : val; |
There was a problem hiding this comment.
Please add a comment explaining why numbers are not interpreted as script names. Ideally you'd have a map from numbers to the scripts as a compatibility layer. Breaking things people have set up in the past is confusing and in-transparent.
|
Hm Okay, fine with me. What's the point of rgb cycle (vs the existing change color with rate + static)? |
I'd say it's the other way around: programability of the lamps makes the built-in cycling a bit redundant, but it's a (free) way to seed a semi-random base color if one is needed, so I left it in but maybe the built-in should be changed into rgb as it has "better" colors (soundviz would be affected too) |
|
I'm not sure if the built-in color list was curated to be "moodlight-ier" than the hue cycling, so my first reaction would be "keep the old one". I just found the user experience confusing because when the cycle is active, the "change color with rate" field does not affect the cycle speed. Not a big thing. |
|
installed qt declarative on the ci machine. Test this please |
|
Meh. 18.04 has only QT 5.9.5. |
As a test for #335 I swapped cpp lamps for js with QJSEngine and it works pretty well.
Here's the readme.
Also
qmlmodule needsqtdeclarative5-devpackage to build on ubuntu.What I'm not sure about is how to bundle and deploy the scripts to the user folder on all platforms, so if you have some ideas...Actually baking script into resources (and copying on launch) works well.
So it's now ok to merge for me...just kidding, this needs Qt 5.12 ;-;