E_showMenu: possibly refactor to avoid memory leak by not passing internal menu object `l` as argument to callback functions

[thyttan]

Background

why l is passed

Yeah, I think the idea was the calling function would get the scroll level out of the menu easily (but it could have done that anyway). One bad side-effect is if you do "menitem":showNewMenu you get a memory leak as the old menu is passed as an argument, but I think realistically we can't easily remove it now unless it breaks something.

... so thinking about the memory leak I guess my gut feeling is to not pass l.

Originally posted by @gfwilliams in #2565 (comment)

Commenting on this:

I think realistically we can't easily remove it now unless it breaks something.

I did some digging below.

Conclusions based on findings below

• There should be no problem to remove the scroller object from being sent as argument to the callback functions - as no app seem to use that option (Edit: also the case for Bangle.js 1, see comment).
• The software reference needs updating regarding the contents of the returned object - as it seems to correspond to an older implementation. Edit: I see, for Bangle.js 1 it's still draw, select and move (+ back and lastIdx).
• The custom implementations may need updates to their respective readme's to indicate the returned object does not match the current firmware implementation.

Method

1. Grep for E\.showMenu over all files in BangleApps.
   

2. Grep again over the files from (1.) with (\.scroller)|(\.draw\(\)).
   

3. Copy matches to quickfix list for inspection, resulting in findings below.

   • matches in the quickfix list that didn't have to do with E.showMenu were omitted from findings sections below.

apps/setting/settings.js|151 col 53| if ("qmsched" in WIDGETS) WIDGETS["qmsched"].draw();
apps/setting/settings.js|248 col 6| m.draw();
apps/wid_edit/settings.js|122 col 8| m.draw();
apps/spacew/app.js|9 col 2| m.draw(); // draw centered on the middle of the loaded map
apps/spacew/app.js|22 col 4| m.draw();
apps/spacew/app.js|257 col 11| if (0) m.draw();
apps/promenu/boot.js|151 col 10| l.draw();
apps/promenu/boot.js|170 col 4| l.draw();
apps/widbgjs/settings.js|22 col 27| WIDGETS['widbgjs'].draw();
apps/menusmall/boot.js|90 col 10| l.draw();
apps/menusmall/boot.js|108 col 4| l.draw();
apps/gpsrec/app.js|209 col 8| osm.draw();
apps/alarm/app.js|109 col 34| var scroller = E.showMenu(menu).scroller;
apps/gipy/app.js|1512 col 16| osm.draw();
apps/wrkmem/app.js|348 col 45| swipeControls.forEach(control => control.draw());
apps/gpspoilog/app.js|19 col 7| menu.draw();
apps/touchmenu/touchmenu.boot.js|70 col 10| m.draw();
apps/touchmenu/touchmenu.boot.js|75 col 12| m.draw();
apps/touchmenu/touchmenu.boot.js|89 col 12| m.draw();
apps/touchmenu/touchmenu.boot.js|101 col 8| m.draw();
apps/touchmenu/touchmenu.boot.js|122 col 4| m.draw();
apps/touchmenu/touchmenu.boot.js|137 col 8| m.draw();
apps/recorder/app.js|257 col 8| osm.draw();
apps/menuwheel/boot.js|165 col 10| l.draw();
apps/menuwheel/boot.js|182 col 8| l.draw();
apps/menuwheel/boot.js|185 col 4| l.draw();
apps/multitimer/app.js|180 col 10| s.draw();
apps/multitimer/app.js|476 col 10| s.draw();
apps/puzzle15/puzzle15.app.js|662 col 8| board.draw();
apps/promenu/bootb2.ts|168 col 10| l.draw();
apps/promenu/bootb2.ts|202 col 4| l.draw();
apps/activepedom/settings.js|34 col 31| //WIDGETS["activepedom"].draw();
apps/grocery/app.js|22 col 9| menu.draw();
apps/qmsched/app.js|95 col 4| m.draw();
apps/qmsched/app.js|134 col 31| if (m.lastIdx===undefined) m.draw(); // applyTheme didn't redraw menu, but we need to show updated mode
apps/widmp/settings.js|12 col 40| if (WIDGETS["widmp"]) WIDGETS["widmp"].draw();
apps/openstmap/app.js|38 col 18| const count = m.draw();
apps/openstmap/app.js|44 col 6| m.draw();
apps/promenu/bootb2.js|143 col 18| l.draw();
apps/promenu/bootb2.js|170 col 6| l.draw();
apps/hadash/hadash.app.js|33 col 29| const r = E.showMenu(menu).scroller;

Apps using the object from E.showMenu

Summary

No app seem to use the option to look at the scroller object as a parameter to any of the entry mapped callback functions.

Findings per app

// Template
// Uses: draw, scroller.
// Routed via: return statement, callback function parameter.

// setting
// Uses: draw.
// Routed via: return.
apps/setting/settings.js|248 col 6| m.draw();

// wid_edit
// Uses: draw.
// Routed via: return.
apps/wid_edit/settings.js|122 col 8| m.draw();

// alarm
// Uses: scroller.
// Routed via: return.
apps/alarm/app.js|109 col 34| var scroller = E.showMenu(menu).scroller;

// gpspoilog
// Uses: draw.
// Routed via: return.
apps/gpspoilog/app.js|19 col 7| menu.draw();

// grocery
// Uses: draw.
// Routed via: return.
apps/grocery/app.js|22 col 9| menu.draw();

// qmsched
// Uses: draw.
// Routed via: return.
apps/qmsched/app.js|95 col 4| m.draw();
apps/qmsched/app.js|134 col 31| if (m.lastIdx===undefined) m.draw(); // applyTheme didn't redraw menu, but we need to show updated mode

// hadash
// Uses: scroller.
// Routed via: return.
apps/hadash/hadash.app.js|33 col 29| const r = E.showMenu(menu).scroller;

// wrkmem
// ?
// This is a hard one to wrap my head around. But I don't think it uses the scroller object at all.
apps/wrkmem/app.js|348 col 45| swipeControls.forEach(control => control.draw());

Apps that replace E.showMenu with their own imlementation.

Summary

There are inconsistencies between the returned objects constitution of the standard and custom implementations (Edit: It's that they correspond more to the Bangle.js 1 standard implementations).

The custom implementations mostly contain draw, select and move entries. But also e.g. info, scroll, selected, lastIdx. They most often return the object as well as send it as an argument to the callback functions.

The standard implementation contains draw and scroller (Edit: For Bangle.js 2):

{
  draw: function () { ... },
  scroller: { scroll: -24,
    draw: function () { ... },
    drawItem: function (a) { ... },
    isActive: function () { ... }
   }
 }

Findings per app

// Template
// Object contents: draw, scroller, ... .
// Routes via: return, callback.

// promenu creates a custom object that it returns/uses in callback functions. Interestingly this custom object corresponds better to the software reference than the standard implementation.
// Object contents: draw, select, move.
// Routes via: return, callback.
apps/promenu/boot.js|151 col 10| l.draw();
apps/promenu/boot.js|170 col 4| l.draw();
apps/promenu/bootb2.js|143 col 18| l.draw();
apps/promenu/bootb2.js|170 col 6| l.draw();
apps/promenu/bootb2.ts|168 col 10| l.draw();
apps/promenu/bootb2.ts|202 col 4| l.draw();

// menuwheel creates a custom object that it returns/uses in callback functions. Interestingly this custom object corresponds better to the software reference than the standard implementation.
// Object contents: lastIdx, draw, select, move.
// Routed via: return, callback.
apps/menuwheel/boot.js|165 col 10| l.draw();
apps/menuwheel/boot.js|182 col 8| l.draw();
apps/menuwheel/boot.js|185 col 4| l.draw();

// menusmall creates a custom object that it returns/uses in callback functions. Interestingly this custom object corresponds better to the software reference than the standard implementation.
// Object contents: draw, select, move.
// Routes via: return, callback.
apps/menusmall/boot.js|90 col 10| l.draw();
apps/menusmall/boot.js|108 col 4| l.draw();

// touchmenu creates a custom object that it returns.
// Object contents: info, scroll, selected, draw, select, move.
// Routed via: return.
apps/touchmenu/touchmenu.boot.js|70 col 10| m.draw();
apps/touchmenu/touchmenu.boot.js|75 col 12| m.draw();
apps/touchmenu/touchmenu.boot.js|89 col 12| m.draw();
apps/touchmenu/touchmenu.boot.js|101 col 8| m.draw();
apps/touchmenu/touchmenu.boot.js|122 col 4| m.draw();
apps/touchmenu/touchmenu.boot.js|137 col 8| m.draw();

[thyttan]

Checking for use of the passed argument l for Bangle.js 1 apps

1. Grepped for E\.showMenu\( over all files in BangleApps
2. Among the files resulting from (1.), Grepped for (\.select\()|(\.move\()|(\.lastIdx)|(\.back\()
3. Added the matches to quickfix list for inspection. See below.

apps/sokoban/app.js|401 col 12| map.move(max_direction);
apps/fileman/fileman.app.js|87 col 6| m.move(-1);
apps/acmaze/app.js|194 col 18| return this.move(-1, 0);
apps/acmaze/app.js|196 col 18| return this.move(1, 0);
apps/acmaze/app.js|202 col 18| return this.move(0,1);
apps/acmaze/app.js|204 col 18| return this.move(0,-1);
apps/touchmenu/touchmenu.boot.js|129 col 10| m.back();
apps/touchmenu/touchmenu.boot.js|132 col 8| m.select(d.x, d.y);
apps/qmsched/app.js|94 col 11| delete m.lastIdx; // force redraw
apps/qmsched/app.js|132 col 11| delete m.lastIdx; // force redraw
apps/qmsched/app.js|134 col 8| if (m.lastIdx===undefined) m.draw(); // applyTheme didn't redraw menu, but we need to show updated mode
apps/chess/app.js|161 col 20| const res = state.move(from, to);

4. After inspection, none of the matches corresponded to a use of the object through parameter in callback function. But only to the object returned from calling E.showMenu.