From bb96714ab65e0c3353fa922cf51b342678279c9b Mon Sep 17 00:00:00 2001
From: Gordon Williams <gw@pur3.co.uk>
Date: Mon, 20 May 2024 10:27:43 +0100
Subject: [PATCH] Update setLCDOverlay as per
 https://github.com/espruino/BangleApps/issues/3417#issuecomment-2118191597

---
 ChangeLog                     |  2 +-
 libs/banglejs/jswrap_bangle.c | 32 +++++++++++++++++++-------------
 2 files changed, 20 insertions(+), 14 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 0008766b17..80a96d42fc 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -50,7 +50,7 @@
               - also speed up ObjectGetInteger by not requiring a new var to be allocated to unpack a NAME_INT
             jsvSkipName speed improvements by avoiding repeated masking of flags
             No longer call jsvKill/Init when resetting, but just jsvReset (this avoids free+malloc on ESP32)
-            Bangle.js2: Bangle.setLCDOverlay can now take an object with {id:str, onRemove:fn} to deal with multiple users of overlays better
+            Bangle.js2: Bangle.setLCDOverlay can now take an object with {id:str, remove:fn} to deal with multiple users of overlays better
 
      2v21 : nRF52: free up 800b more flash by removing vector table padding
             Throw Exception when a Promise tries to resolve with another Promise (#2450)
diff --git a/libs/banglejs/jswrap_bangle.c b/libs/banglejs/jswrap_bangle.c
index aacca5d17b..019d236cae 100644
--- a/libs/banglejs/jswrap_bangle.c
+++ b/libs/banglejs/jswrap_bangle.c
@@ -2405,13 +2405,13 @@ void jswrap_banglejs_setLCDOffset(int y) {
       ["img","JsVar","An image, or undefined to clear"],
       ["x","JsVar","The X offset the graphics instance should be overlaid on the screen with"],
       ["y","int","The Y offset the graphics instance should be overlaid on the screen with"],
-      ["options","JsVar","[Optional] object `{onRemove:fn, id:\"str\"}`"]
+      ["options","JsVar","[Optional] object `{remove:fn, id:\"str\"}`"]
     ],
     "#if" : "defined(BANGLEJS_Q3) || defined(DICKENS)",
     "typescript" : [
       "setLCDOverlay(img: any, x: number, y: number): void;",
       "setLCDOverlay(): void;",
-      "setLCDOverlay(img: any, x: number, y: number, options: { id : string, onRemove: () => void }): void;",
+      "setLCDOverlay(img: any, x: number, y: number, options: { id : string, remove: () => void }): void;",
       "setLCDOverlay(img: any, options: { id : string }}): void;"
     ]
 }
@@ -2426,7 +2426,7 @@ var img = require("heatshrink").decompress(atob(`lss4UBvvv///ovBlMyqoADv/VAwlV//
 GwIKCngWC14sB7QKCh4CBCwN/64KDgfACwWn6vWGwYsBCwOputWJgYsCgGqytVBQYsCLYOlqtqwAsFEINVrR4BFgghBBQosDEINWIQ
 YsDEIQ3DFgYhCG4msSYeVFgnrFhMvOAgsEkE/FhEggYWCFgIhDkEACwQKBEIYKBCwSGFBQJxCQwYhBBQTKDqohCBQhCCEIJlDXwrKE
 BQoWHBQdaCwuqJoI4CCwgKECwJ9CJgIKDq+qBYUq1WtBQf+BYIAC3/VBQX/tQKDz/9BQY5BAAVV/4WCBQJcBKwVf+oHBv4wCAAYhB`));
-Bangle.setLCDOverlay(img,66,66, {id: "myOverlay", onRemove: () => print("Removed")});
+Bangle.setLCDOverlay(img,66,66, {id: "myOverlay", remove: () => print("Removed")});
 ```
 
 Or use a `Graphics` instance:
@@ -2438,7 +2438,7 @@ ovr.palette = new Uint16Array([0,0,g.toColor("#F00"),g.toColor("#FFF")]); // (op
 ovr.setColor(1).fillRect({x:0,y:0,w:99,h:99,r:8});
 ovr.setColor(3).fillRect({x:2,y:2,w:95,h:95,r:7});
 ovr.setColor(2).setFont("Vector:30").setFontAlign(0,0).drawString("Hi",50,50);
-Bangle.setLCDOverlay(ovr,38,38, {id: "myOverlay", onRemove: () => print("Removed")});
+Bangle.setLCDOverlay(ovr,38,38, {id: "myOverlay", remove: () => print("Removed")});
 ```
 
 To remove an overlay, simply call:
@@ -2450,6 +2450,9 @@ Bangle.setLCDOverlay(undefined, {id: "myOverlay"});
 Before 2v22 the `options` object isn't parsed, and as a result
 the remove callback won't be called, and `Bangle.setLCDOverlay(undefined)` will
 remove *any* active overlay.
+
+The `remove` callback is called when the current overlay is removed or replaced with
+another, but *not* if setLCDOverlay is called again with an image and the same ID.
 */
 void jswrap_banglejs_setLCDOverlay(JsVar *imgVar, JsVar *xv, int y, JsVar *options) {
   bool removingOverlay = jsvIsUndefined(imgVar);
@@ -2461,22 +2464,25 @@ void jswrap_banglejs_setLCDOverlay(JsVar *imgVar, JsVar *xv, int y, JsVar *optio
     id = jsvObjectGetChildIfExists(options, "id");
   }
   JsVar *ovrId = jsvObjectGetChildIfExists(execInfo.hiddenRoot, "lcdOvrId");
+  bool idIsDifferent = !jsvIsEqual(id, ovrId);
+  jsvUnLock(ovrId);
   // if we are removing overlay, and supplied an ID, and it's different to the current one, don't do anything
-  if (removingOverlay && id && !jsvIsEqual(id, ovrId)) {
-    jsvUnLock2(ovrId, id);
+  if (removingOverlay && id && idIsDifferent) {
+    jsvUnLock(id);
     return;
   }
-  jsvUnLock(ovrId);
-  // We're definitely changing overlay now... run the remove callback if it exists
-  JsVar *removeCb = jsvObjectGetChildIfExists(execInfo.hiddenRoot, "lcdOvrCb");
-  if (removeCb) {
-    jsiQueueEvents(0, removeCb, NULL, 0);
-    jsvUnLock(removeCb);
+  // We're definitely changing overlay now... run the remove callback if it exists and the ID is different
+  if (idIsDifferent) {
+    JsVar *removeCb = jsvObjectGetChildIfExists(execInfo.hiddenRoot, "lcdOvrCb");
+    if (removeCb) {
+      jsiQueueEvents(0, removeCb, NULL, 0);
+      jsvUnLock(removeCb);
+    }
   }
   // update the fields in the Bangle object
   if (imgVar) {
     jsvObjectSetOrRemoveChild(execInfo.hiddenRoot, "lcdOvrId", id);
-    removeCb = jsvIsObject(options) ? jsvObjectGetChildIfExists(options, "onRemove") : NULL;
+    JsVar *removeCb = jsvIsObject(options) ? jsvObjectGetChildIfExists(options, "remove") : NULL;
     jsvObjectSetOrRemoveChild(execInfo.hiddenRoot, "lcdOvrCb", removeCb);
     jsvUnLock(removeCb);
   } else { // we're removing the overlay, remove all callbacks