Skip to content
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

Bangle_setUI_Q3/F18: tweaks (read below) #2571

Merged
merged 3 commits into from
Nov 4, 2024

Conversation

thyttan
Copy link
Contributor

@thyttan thyttan commented Oct 15, 2024

  • Add custom handlers on top of the standard modes as well. Previously this was only possible for mode == "custom".
    • The goal here is to make it possible to move all input handling inside setUI where today some apps add on extra handlers outside of setUI calls.
  • Change the default behaviour of the hardware button to act immediately on press down. Previously it has been acting on button release.
    • This makes the interaction slightly snappier.
    • In addition to the existing btn key a new btnRelease key can now be specified. btnRelease will let you listen to the rising edge of the hardware button.

These are the same changes introduced as customized setui via espruino/BangleApps#3487, espruino/BangleApps#3491 and espruino/BangleApps#3563.

Forum thread: https://forum.espruino.com/conversations/397606/

Before merge:

  • update documentation
  • update the Bangle.js 1 implementation?

- Add custom handlers on top of the standard modes as well. Previously this was only possible for mode == "custom".
  - The goal here is to make it possible to move all input handling inside `setUI` where today some apps add on extra handlers outside of `setUI` calls.
- Change the default behaviour of the hardware button to act immediately on press down. Previously it has been acting on button release.
  - This makes the interaction slightly snappier.
  - In addition to the existing `btn` key a new `btnRelease` key can now be specified. `btnRelease` will let you listen to the rising edge of the hardware button.
@thyttan thyttan marked this pull request as draft October 15, 2024 21:00
@gfwilliams
Copy link
Member

Thanks! Yes, this looks good.

Do you have a Bangle.js 1 to test with? I'd be tempted not to mess with Bangle.js 1 now, but I guess if some apps are using setUI and expecting things to work a certain way then we may have to...

@thyttan
Copy link
Contributor Author

thyttan commented Oct 16, 2024

Do you have a Bangle.js 1 to test with? I'd be tempted not to mess with Bangle.js 1 now, but I guess if some apps are using setUI and expecting things to work a certain way then we may have to...

I don't have a Bangle.js 1 no.

Can we just refrain from modifying apps that support Bangle.js 1 (I haven't checked how many are relevant here) and change those over only when/if we get to tweaking the setUI_F18 implementation?

And add a note to the reference that the behaviour is slightly different across Bangle.js 1 & 2?

@thyttan
Copy link
Contributor Author

thyttan commented Oct 21, 2024

@gfwilliams if my last comment sounds like it works I can modify this PR to accommodate 🙂

@gfwilliams
Copy link
Member

Can we just refrain from modifying apps that support Bangle.js 1 (I haven't checked how many are relevant here) and change those over only when/if we get to tweaking the setUI_F18 implementation?

I'm not quite sure what you mean here. I feel like a lot of the 'core' apps target both, so we can't just update for one and not the other. So I guess something like Run or Messages might be an example here - it's designed for both, but it uses Layout which uses falling edge at the moment? So I guess there's a question of how they'll interact - like what if you bring up E.showMenu from Layout, then go back with the button (which happens on rising edge?). Now the Layout is showing but Layout is listening for the falling edge and gets called on the same button press?

So I think some of those apps will need changes, and then they will at least need testing on a Bangle.js 1

@thyttan
Copy link
Contributor Author

thyttan commented Oct 21, 2024

Can we just refrain from modifying apps that support Bangle.js 1 (I haven't checked how many are relevant here) and change those over only when/if we get to tweaking the setUI_F18 implementation?

I'm not quite sure what you mean here. I feel like a lot of the 'core' apps target both, so we can't just update for one and not the other.

Yes, so those would fall under "apps that support Bangle.js 1" in my thinking, and should not be modified yet.

... So I guess something like Run or Messages might be an example here - it's designed for both, but it uses Layout which uses falling edge at the moment?

EDIT: With setuichange messagegui seems to always react on rising edge whether in menu, message, message list, etc. For run the button still acts on falling edge.

... So I guess there's a question of how they'll interact - like what if you bring up E.showMenu from Layout, then go back with the button (which happens on rising edge?). Now the Layout is showing but Layout is listening for the falling edge and gets called on the same button press?

Hm - I seem to not have stumbled upon that specific combo in my daily driving of setuichange. It sounds like a valid point though (without actually inspecting the code), and I could see that happening for the messagegui app if setUI wasn't updated (as would be the case for Bangle.js 1).

As for the run app, I actually think we should not adapt it and just leave it to react on EDIT: FALLING edge. That makes sure a run is not accidentally started when the user just wanted to exit the app by reloading on long press. For that app I don't think the refactoring makes too much sense.

So I think some of those apps will need changes, and then they will at least need testing on a Bangle.js 1

Yes, lets hold off for a while then!

I'll comment here again if I reach the conclusion that the problems would actually not occur.

EDITS: per above

@thyttan
Copy link
Contributor Author

thyttan commented Oct 21, 2024

Edited to correct my writing above....

@gfwilliams
Copy link
Member

... and should not be modified yet.

If they don't need modifying to work with this then great - but I think obviously if they end up breaking then I don't think we have a choice.

Great news on messagesgui - I guess that uses back in Layout which calls straight to setUI

About the Run app, I think maybe that actually is a good reason why we should modify the Bangle.js1 setUI, if only to add btnRelease as an option (even if right now it does the exact same thing as btn). For run it's not needed as we use Layout and that uses setWatch directly, but it may be we hit other apps that rely on setUI(...btn) and we want to be able to change those to btnRelease to be explicit.

Explicitly state button should fire on falling edge.

Does not change `btn` mapped handlers to act on rising edge just jet.

As per suggestion in
espruino#2571 (comment).
@thyttan thyttan changed the title Bangle_setUI_Q3: tweaks (read below) Bangle_setUI_Q3/F18: tweaks (read below) Nov 1, 2024
thyttan pushed a commit to thyttan/BangleApps that referenced this pull request Nov 2, 2024
thyttan pushed a commit to thyttan/BangleApps that referenced this pull request Nov 2, 2024
In step with espruino/Espruino@c5fce53, on PR espruino/Espruino#2571

This is currently untested since I don't have a Bangle.js 1 myself.
thyttan pushed a commit to thyttan/BangleApps that referenced this pull request Nov 2, 2024
In step with espruino/Espruino@6ac038d on PR espruino/Espruino#2571

This is currently untested since I don't have a Bangle.js 1 myself.
@thyttan
Copy link
Contributor Author

thyttan commented Nov 2, 2024

Ok - I refactored the Bangle.js 1 setui now as well. Have a look when you got the time. Maybe we can announce on the forum thread and ask for testers for that as well, if you don't spot something right away.

thyttan pushed a commit to thyttan/BangleApps that referenced this pull request Nov 2, 2024
In step with espruino/Espruino@12dacea on PR espruino/Espruino#2571

This is currently untested since I don't have a Bangle.js 1 myself.
@gfwilliams
Copy link
Member

That looks great, thanks! I think we've come far enough with this the thing to do is maybe just to merge in and see if we get any complaints. I can always pull it out before a full release if there are problems

@gfwilliams gfwilliams marked this pull request as ready for review November 4, 2024 11:35
@gfwilliams gfwilliams merged commit 12dacea into espruino:master Nov 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants