widget_utils - Remove the overlay on cleanup

[halemmerich]

Currently a swiped down widget bar stays on display when fastloading from clock to launcher. This cleans up the overlay if nothing else called setLCDOverlay in the meantime.

This will fail if something else replaces setLCDOverlay. That might not be problematic since there are limited use cases for doing that?

[bobrippling]

This will fail if something else replaces setLCDOverlay. That might not be problematic since there are limited use cases for doing that?

Yeah I think this is safe, I do have a few ideas about being able to share setLCDOverlay, now that ctrlpad is merged

[halemmerich]

Yeah I think this is safe, I do have a few ideas about being able to share setLCDOverlay, now that ctrlpad is merged

I have thought about extracting the code for handling "background" watches and screen event handlers into a library. Maybe it would be useful in a library for shared overlay access.

[bobrippling]

I have thought about extracting the code for handling "background" watches and screen event handlers into a library. Maybe it would be useful in a library for shared overlay access.

While we're on the topic, I suppose we need to figure out what should happen if two apps want to use the overlay at once. Do we raise an error? Block on a promise?
I'm thinking the latter, since the overlay is temporary it'll eventually be freed up. But maybe apps like ctrlpad have some way of hooking notifications so they show if ctrlpad is currently using the overlay

[halemmerich]

I think there should be some kind of priority based decision. Higher priority overlays replace other overlays and the removed overlay is informed via callback that it has been removed. An example would be swiped down widgets on a clock and a message incoming. The message overlay would be displayed directly and the lower priority overlay is just removed.
Blocking on a promise is not optimal if the other overlay is long living but not important.
If a lower priority overlay asks to be drawn the call could just return false or directly call the removal callback.

[bobrippling]

I like the sound of that, I suppose the promise blocking could be surprising to a user - for example, they've just hidden one overlay, then another pops up immediately as if it just arrived, when it's actually been waiting all along.

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?
  })
}

[halemmerich]

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?

[bobrippling]

Maybe discussion on this would be better visible in a dedicated issue or forum thread?

Good idea - #3417

[bobrippling]

Change looks good btw, thanks!

[gfwilliams]

Hi - Sorry, I just saw this one. I guess from my point of view, this looks like quite a lot of code when just Bangle.setLCDOverlay() would have worked apart from in the rare case something else was displayed.

Was there a specific app/thing where this really caused a problem?

I just mentioned in #3417 - maybe I change the firmware to allow >1 overlay? I guess priority could still be an issue but I wonder how much we care for things that are only supposed to display for a few seconds if the most recent thing displays on top

[halemmerich]

My usecase is messagesoverlay displaying (and having lots of background stuff hiding/tracking the original event handlers and watches) and widget_utils then removing the overlay but it leaves all the other stuff in place without visible indication.

Example: Be in clock with swiped down widgets, message is drawn as overlay (replacing widgets), timeout of widget_utils tries to automatically hide widget overlay, message overlay is removed by widget_utils, then the only active touch handler left is the one for closing the message overlay and nothing else but long press reload works to regain control. In case this happens without me noticing I have a clock completely unresponsive to anything but unlocking (and a single touch handler for the 0,0,176,40 area to close the no longer visible message).

Bangle.setLCDOverlay is only overridden as long the widgets are actually visible. When there is consensus on what to do in #3417 this would be changed again anyhow because there is a clean solution to this problem then. I'm also fine with removing the override while keeping the cleanup change until then, this happening is not that probable and even users not knowing the inner workings of messagesoverlay can get out of that with long press.

[gfwilliams]

Ok, thanks for the explanation.

As it's in we might as well make any new changes after #3417 - my concern is that this code pattern might be copied (eg for messagesoverlay) and then all kinds of problems get introduced with overwritten Bangle.setLCDOverlay functions which are far worse than the initial disappearing overlay one.