espruino / BangleApps

Bangle.js App Loader (and Apps)
https://banglejs.com/apps
MIT License
496 stars 1.17k forks source link

settings: restore previous menu's scroll position #3630

Closed bobrippling closed 6 days ago

bobrippling commented 3 weeks ago

To improve settings usability

thyttan commented 3 weeks ago

Tried it - works good on my watch!

I'll leave the merging to @gfwilliams.

gfwilliams commented 3 weeks ago

This is a pretty big change - how sure are you it's not going to break something, and that there aren't memory leaks with the push/pop menus?

And do you know if it noticeably increases memory usage? It looks like maybe it wouldn't by anything noticeable but it'd be really bad if it stopped working on Bangle.js 1.

It might be nice to have some kind of docs on when push/pop/restoreMenu should be used so that if someone works on this in the future we don't end up with it getting broken?

bobrippling commented 3 weeks ago

This is a pretty big change - how sure are you it's not going to break something, and that there aren't memory leaks with the push/pop menus?

I've been through most menus, particularly the ones that don't fit the typical pattern, but I'll do an exhaustive check for memory leaks and thus confirm there's no hidden bugs

And do you know if it noticeably increases memory usage? It looks like maybe it wouldn't by anything noticeable but it'd be really bad if it stopped working on Bangle.js 1.

Will add my findings from the above check

It might be nice to have some kind of docs on when push/pop/restoreMenu should be used so that if someone works on this in the future we don't end up with it getting broken?

Sounds good, yes I'll add that

gfwilliams commented 3 weeks ago

That's great - thanks for your work on this!

bobrippling commented 6 days ago

Exhaustively checked all the menu items, memory usage stabilises on returning out of the menus (max was about 5.5k) and I've noted down the usage in the readme. Give me a shout if there's anything I've missed

gfwilliams commented 6 days ago

That's awesome - thanks! Let's merge then!

bobrippling commented 6 days ago

Thanks!