-
-
Notifications
You must be signed in to change notification settings - Fork 754
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
discussion: SetUI Q3 changes #2526
Conversation
TODO: Not thoroughly tested yet. First discussed here around here: espruino/BangleApps#3435 (comment)
This is maybe a quick fix. There could maybe be a bigger refactor to handle btn watches in the same way as other handlers are added, where they aren't added until we know which on we want to set (By writing/overwriting e.g. `Bangle.touchHandler` and only actually set it with `Bangle.on` at the end of the script).
`btn` can be set like before, so backwards compatible I think. But it can now also be an object on form {fn: ()={}, edge: "rising/falling/both"}. @gfwilliams has suggested another solution using callbacks in espruino/BangleApps#3452 (comment) The solution I opted for here for now follows the same coding style of how the `type`/`mode` parameter of setUI can be either a string or an object.
To make the ui feel snappier. First discussed around here: espruino/BangleApps#3435 (comment)
if (options.btn) { | ||
if (Bangle.btnWatches) Bangle.btnWatches.forEach(clearWatch); | ||
var e = "rising"; | ||
if ("object"==typeof options.btn) { | ||
e = options.btn.edge; | ||
options.btn = options.btn.fn; | ||
} | ||
Bangle.btnWatches = [ | ||
setWatch(function() { options.btn(1); }, BTN1, {repeat:1,edge:e}) | ||
]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gfwilliams I'm particularly interested in what you think about this solution to letting the user choose button edge (between rising/falling/both). Since it's different from what you proposed with callbacks.
The solution I opted for here for now follows the same coding style of how the type
/mode
parameter of setUI can be either a string or an object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, but honestly, I think I prefer the callback solution mentioned before - the main reason is that this was isn't backwards compatible. At least the old one would silently continue but not add the watch - if someone ran spotrem on an old firmware it'd just fail with an error and not do anything
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... also I feel like the implementation is actually marginally tidier:
if (options.btn) Bangle.btnWatches.push(setWatch(options.btn.bind(options), BTN1, {repeat:1,edge:"rising"}))
if (options.btnRelease) Bangle.btnWatches.push(setWatch(options.btnRelease.bind(options), BTN1, {repeat:1,edge:"falling"}))
Just to add I think this still needs a lot of testing. We've gone from in espruino/BangleApps#3435 having an app that enables this for users that want it, to enabling it for everyone. Personally I'd quite like to see this as an app that overrides |
@gfwilliams thanks! I'll refactor to the callback solution and package it as an app. |
@gfwilliams
Related issues:
espruino/BangleApps#3435
espruino/BangleApps#3452
I've tweaked setUI for Bangle.js 2 to try and achieve goals discussed previously in the issues linked above, regarding the opinion that hw button edge should be rising - and that setting custom handlers on any mode of setui should be possible.
Here I tweaked
spotrem
to take advantage of these setUI changes to move all input logic inside setUI: espruino/BangleApps#3485NOT THOROUGHLY TESTED!