Skip to content

Conversation

@thyttan
Copy link
Collaborator

@thyttan thyttan commented Oct 7, 2024

This will let the user toggle existing alarms, timers and events from the main menu of the app. Either by long touch of the menu entries or by touch on the icon on the right.

This change requires to send the touch event used in showScroller through to alarm app. Both showScroller and showMenu needs changes to make it work. So in this draft state of this PR I add two boot files for modifying those two functions. Have a look at the individual commits to see the diff from firmware version, which I copied over and committed unchanged before tweaking them.

Test it via: https://thyttan.github.io/BangleApps/?id=alarm

The tests fail for the showMenu boot file, but it works on the watch. Enough for testing purpouses.

@thyttan thyttan requested a review from bobrippling October 7, 2024 09:51
@thyttan thyttan force-pushed the alarm branch 3 times, most recently from bb70fdf to 823eda8 Compare October 7, 2024 09:59
@bobrippling
Copy link
Collaborator

Alarm app diff looks good to me, and so do the showMenu & showScroller ones. Given it a trial run and it works a treat

@thyttan
Copy link
Collaborator Author

thyttan commented Oct 7, 2024

Alarm app diff looks good to me, and so do the showMenu & showScroller ones. Given it a trial run and it works a treat

Thanks!

@thyttan
Copy link
Collaborator Author

thyttan commented Oct 7, 2024

@gfwilliams what would you say about tweaking showMenu_Q3 and showScroller_Q3 in firmware like I propose on this PR and then merge this without the modifications at boot?

When I tested just now, the alarm code seems to work as before if using the standard fw versions of showScroller/Menu. I don't think this should break anything - but maybe I'm overlooking something.

@thyttan
Copy link
Collaborator Author

thyttan commented Oct 7, 2024

Just tried on emulated Bangle.js 1 and got:

Uncaught Error: Can't read property 'scroll' of undefined
 at line 20 col 1070 in alarm.app.js
...mMenu,10,e,index,undefined,scroller.scroll,group)
                                      ^
in function "onchange" called from line 3 col 106
...onchange)a.onchange(a.value);Bangle.touchHandler==u&&d.draw(...
                              ^
in function "select" called from line 1 col 22
a?d.move(a):d.select()
                     ^
in function "d" called from line 1 col 3
d()
  ^
in function called from system
> 

Looking at the git blame it seems to have been around since 2023-11-14.

Will open a separate PR with a fix.

@gfwilliams
Copy link
Member

Personally, I don't think we should be including different versions of showMenu/etc in apps as it's a maintenance nightmare, but I guess you did this just so we could see what it was like?

I'm up for modifying the built-in ones to pass the extra values through - those changes you made are pretty tidy, although some docs on that would be good, so it's not yet another hidden feature that gets used in only one app ;)

Same for the Alarm app really - it's a nice addition, but it'd also need to be documented in the README as it's not obvious it's something that can be done, especially as different to the way the menu works in all other apps

@thyttan
Copy link
Collaborator Author

thyttan commented Oct 8, 2024

Thanks!

Personally, I don't think we should be including different versions of showMenu/etc in apps as it's a maintenance nightmare, but I guess you did this just so we could see what it was like?

Yes - exactly!

I'm up for modifying the built-in ones to pass the extra values through - those changes you made are pretty tidy, although some docs on that would be good, so it's not yet another hidden feature that gets used in only one app ;)

Ok - will look at that 👍

Same for the Alarm app really - it's a nice addition, but it'd also need to be documented in the README as it's not obvious it's something that can be done, especially as different to the way the menu works in all other apps

... and this to!

... either by long touch on an alarm entry or touching its icon on the
right.
@thyttan thyttan marked this pull request as ready for review October 11, 2024 10:34
@thyttan
Copy link
Collaborator Author

thyttan commented Oct 11, 2024

I think this is ready for merge together with espruino/Espruino#2565.

@gfwilliams
Copy link
Member

Looks good, thanks! Should be compatible with older firmwares too - it just won't work?

@gfwilliams gfwilliams merged commit 3d7c6db into espruino:master Oct 11, 2024
1 check passed
@thyttan
Copy link
Collaborator Author

thyttan commented Oct 11, 2024

Yes, on older firmwares touch parameter will become undefined and so always cause the else statement to run.

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.

3 participants