Open bobrippling opened 6 months ago
Thanks - overwriting Bangle.setLCDOverlay
in #3394 seems a bit hacky - but I'll post on there about it.
Rather than hacking around this, maybe it's best if we just make Bangle.setLCDOverlay
take another id
argument, and we allow more than one overlay at once?
Potentially it's not the end of the world to update the draw function to handle multiple overlays at once? It'll be a bit slower but it won't be a big deal: https://github.com/espruino/Espruino/blob/master/libs/graphics/lcd_memlcd.c#L292-L295
Allowing multiple overlays is not a complete solution. Apps that show an overlay and manipulate global state need to be informed if they are currently on top or not. Multiple overlays multiplies the problem of dealing with touchscreen event handlers that were present before opening the overlay. I think keeping it simple with one allowed overlay and just assuming that any overlay could be a "modal" one highjacking all handlers should cover most usecases.
There are apps that use overlays as part of their UI, so calling those back on removal of the Overlay would allow them to redraw the overlay contents directly on their graphics instance and keep the appearance consistent without having the overlay (probably slower because of more full redraws) as long as another overlay is open. openstmap
for example could do it that way.
I can see both use cases - say there's a modal overlay (ctrlpad
), then a notification (notify
) popping up at the top of the screen would be ok, but only by "coincidence" that we know they won't overlap.
I suppose we'd still need a priority / z-order so the firmware knows which to draw last, so perhaps fully modal would be good as an initial approach.
Looking at openstmap
, I suppose you'd want to allow (for example) ctrlpad
to be dragged down from the top, then when openstmap
tries to enable the overlay, it'll be prevented (the above overlay.claim()
call will return falsy) and can just skip overlay rendering while ctrlpad
is visible.
Whereas notify
could be a higher priority - a notification comes in, ctrlpad
gets its onRemove
called, clears its event handler hooks and then the notification displays. Then in the future we could look at how we can allow the notification to draw at the top of the screen while ctrlpad
or openstmap
's overlays are visible
Ok, well, it saves me work if you don't want multiple overlays :)
I guess I'm a bit concerned that Bangle.setLCDOverlay
was intended as a way for us to to quickly and temporarily display info over an existing app, it wasn't intended as a full-on window manager with priorities and input handling... I wonder whether trying to make it into that is really the right choice for Bangle.js?
In the openstmap case, using setLCDOverlay seems like the wrong solution. The contents of the screen that's overwritten could be copied back into a small buffer before drawing the icon and overlays could be avoided completely. That would IMO be preferable to having the overlay and then also a fallback - it's likely faster and more efficient for rendering too. I'll see about making that change.
My real concern here is we're trying to deal with an issue which occurs reasonably rarely even with non-standard apps installed, but we'll be introducing more code that may end up running and slowing down every user's Bangle all the time. So really the question is how we implement this in such a way that it's not going to slow everything down when it's not used.
widget_utils
especially is a bit of a pain as it's used in a bunch of apps and is already quite big/slow, and we don't want it to automatically load another library on startup if an app just wanted to use it to call .hide
. I guess if this handling were in a library that was actually a new app then it would only be called after the user swiped down on widgets, rather than at app load time.
But just for a second, can we assume that we extend Bangle.setLCDOverlay
so:
id
string, and a remove
callback that is called when the overlay is removed by another call to Bangle.setLCDOverlay
Bangle.setLCDOverlay(id)
/similar it removes the overlay only if it's got the same IDAre there any cases where this would actually be a massive problem? It seems like:
If this works, adding it could hopefully be done without really adding any more bloat.
Maybe off-topic, but: On Android devices there is a feature (hidden in the developer options) called "Show Taps" which displays a circle at the most recent touch position for a few 100 milliseconds. Since the Bangle2 touch screen occasionally feels a bit imprecise I would love to have this feature here. I'm just wondering, whether this is a use case for setLCDOverlay and whether it might be relevant to the discussion.
miseler yes, it's definitely something that'd be pretty easy to add with setLCDOverlay right now. It could be a pretty simple app to add.
But just for a second, can we assume that we extend
Bangle.setLCDOverlay
so
That is a very KISS solution and probably removes 95% of problematic cases. I think that would be enough for me. The remove callback is essential for apps that use overlays to clean up their stuff to prevent breakage in other apps. Maybe that even allows implementing more fancy handling of overlays on top of that if somebody really wants to do that in a library/boot code.
The whole input/handler/watches part should be done in a separate library anyway for apps that actually want that complexity.
Do files on storage have priority over modules in an app? If so we could reduce widget_utils
to the bare minimum (no autohide, no animation) and just have an app installing a library for the full version.
Yeah the separate library for more complicated use sounds good, same for an app for widget_utils
too
Ok, cool - before I do this can you think if there's anything in that 5% that's going to be a nightmare?
To try and make this backwards compatible, I could extend it to:
setLCDOverlay(img,x,y) -> setLCDOverlay(img,x,y,{ onRemove : fn, id : "str" })
setLCDOverlay() -> setLCDOverlay(undefined,{ id : "str" })
At least then it's a bit more extensible, and on older firmwares they'll just ignore the callback?
Do files on storage have priority over modules in an app? If so we could reduce widget_utils to the bare minimum
Sadly I think it's the other way around. I guess we would need to make a new app for 'swipe on widgets' and then have all apps/clocks that do it modified to use that. Then widget_utils could be reduced to just show/hide as it was before?
that's going to be a nightmare?
I can only think of situations where the UX is suboptimal in case multiple apps want to use the overlay. As long as the remove callback is correctly called it should at least all work in a defined way at all times with this solution.
What about introducing this as a polyfill to the bootloader app and using it a while before promoting it to firmware? That would allow to change the API without many concerns for backwards compatibility in firmware. I have played around with your proposal a bit but did not test on hardware yet, just the emulator: https://gist.github.com/halemmerich/631d226e393e1d10da5ac1b47e025cab
That sounds like a good plan, yes.
However I'm planning on releasing a 2v22 firmware pretty soon, so in a way if I'm going to do it I'd rather get something built in before that (or it'll be a few months until the next one).
I've just added some code to the cutting edge firmware, so this should hopefully all work now. Only thing I changed from above is null->undefined
as null
wouldn't have been easily compatible with the old firmware.
Please can you have a try with it and see if that's all working ok for you?
I have found a potential problem with how widget_utils
uses setLCDOverlay
. The animation calls it for every step and every widgets draw so the remove handler is called lots of times.
Do we want the app handling that or maybe just not call the remove handler if the id
is the same as the current one while setting a new overlay?
The app could differentiate because it knows if it called to remove or to set an overlay, but I have a tendency to solving that in setLCDOverlay
. I don't really see a usecase for calling remove on setting a new overlay for the same id, if an app actually wants that it could use two ids for that or clean up before setting the new overlay.
Another small nitpick, the option is called onRemove
, while other stuff (setUI
,showScroller
) calls it just remove
.
Thanks - good point. Maybe we just don't call it if we're updating the overlay - and I'll change it to remove
- I guess nobody has started using it yet?
Just done
Great, thanks, I'll adapt my code and have a try.
I have tested removal of the widget_utils
overlay by messagesoverlay
and changing between clock and launcher with fastload while widgets are swiped down. It seems to work well so far.
Changes are deployed here: https://halemmerich.github.io/BangleApps/ with the changes being here; https://github.com/espruino/BangleApps/compare/master...halemmerich:BangleApps:newLCDOverlay
Edit: A short test with 2v21 was successful as well: clock->swipe down widgets->show message->swipe down widgets again->fastload to launcher
Great, thanks! Just looking at these changes, are you sure we need the overlayRemoved logic?
It feels the only time it's needed is if something else displays another overlay during the short swipe animation, so that it doesn't get removed.
... but even then, it seems like cleanupOverlay gets called by the remove handler, and that clears animInterval which was the place where you check overlayRemoved - so it feels like the exports.overlayRemoved check in queueDraw will always return false?
No, we don't need that. Resetting the offset at all relevant places and depending on that to determine if the overlay should be enough.
Affected hardware version
Bangle 1, Bangle 2
Your firmware version
2v21
The bug
Continuing the conversation from https://github.com/espruino/BangleApps/pull/3394#issuecomment-2108832638
The default priority is a nice idea, perhaps we could manually trigger
Bangle.on("overlayReplaced", ...)
handlers handles too, to allow the above code to be a bit less disruptive.@gfwilliams any thoughts?
Installed apps
No response