LCD Overlay use by multiple apps

[bobrippling]

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

What about something like this:

var overlay = require("overlay")

var priority = ??? // A number?
var o = overlay.claim(priority)
if (o) {
  // We can now draw to the overlay, move it around, etc
  o.setOverlay(...)

  // At some later point in time
  o.release()

  // Or we can:
  o.onRemove = () => {
    // A higher priority overlay event came in
  }

  o.onAttempt = () => {
    // A lower (or same) priority overlay event came in, but we still have the overlay
    // Perhaps we draw a little icon or alter a widget to indicate there's something else wanting to appear
    // (I feel like `onAttempt` isn't a great name here)
  }
} else {
  // Current overlay is notified via its `onAttempt`, we don't have the overlay
  overlay.onRelease(() => {
    // Add ourselves to the queue?
  })
}

Seems like that could work. An alternative would be extending the setLCDOverlay call to allow giving it a fourth argument containing options like setUI gets. That way all applications using it now could be treated as having some default priority without needing changes. There are currently only 6 apps using the overlay so it would not be that big of a deal to change all of them. Maybe discussion on this would be better visible in a dedicated issue or forum thread?

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

[gfwilliams]

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

[halemmerich]

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.

[bobrippling]

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

[gfwilliams]

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:

• it takes an id string, and a remove callback that is called when the overlay is removed by another call to Bangle.setLCDOverlay
• When you call Bangle.setLCDOverlay(id)/similar it removes the overlay only if it's got the same ID

Are there any cases where this would actually be a massive problem? It seems like:

• swipe down to show widgets, get a message -> widgets disappear, messages displayed, all good
• get a message, swipe down to show ctrlpad/widgets -> message disappears but handlers removed, also ok

If this works, adding it could hopefully be done without really adding any more bloat.

[miseler]

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.

[gfwilliams]

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.

[halemmerich]

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.

[bobrippling]

Yeah the separate library for more complicated use sounds good, same for an app for widget_utils too

[gfwilliams]

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?

[halemmerich]

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

[gfwilliams]

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?

[halemmerich]

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.

[gfwilliams]

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?

[gfwilliams]

Just done

[halemmerich]

Great, thanks, I'll adapt my code and have a try.

[halemmerich]

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

[gfwilliams]

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?

[halemmerich]

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.