[fastload] Improve code readability + new option

[d3nd3]

keep widgets in RAM but hide or show using widget_utils. In case user doesn't care about extra memory used or interference.

[d3nd3]

Sorry, it currently breaks your code a bit. I mis-interpreted something, have to fix it :P

[d3nd3]

I have attempted to make the code less spaghetti and more manageable. I hope you are in favour of this new layout and agree that it will help anyone making future changes.

[gfwilliams]

If you're hiding widgets, surely you need to override Bangle.loadWidgets so you can show them again if the app requests it?

Also, it seems you've got some changes to lcars in this PR?

edit: Assuming I merge the lcars PR first I guess this won't be an issue

[thyttan]

I have attempted to make the code less spaghetti and more manageable. I hope you are in favour of this new layout and agree that it will help anyone making future changes.

Maybe @halemmerich wants to weigh in on this?

[d3nd3]

edit: Assuming I merge the lcars PR first I guess this won't be an issue

No this is unrelated to LCARS supporting fastload.

If you're hiding widgets, surely you need to override Bangle.loadWidgets so you can show them again if the app requests it?

This sounds correct, thanks.

@thyttan have a look at the new code and tell me if you feel its better to work in or not please.

[d3nd3]

If you're hiding widgets, surely you need to override Bangle.loadWidgets so you can show them again if the app requests it?

if (isValidApp) require("widget_utils").show();
else require("widget_utils").hide();

This line is already doing that I believe? both widget_utils and loadWidgets check global.WIDGETS before doing anything.

Ah. If the app requests it, so not at load time, ye, I see your point now. But that is another story too, its checking if app.includes("Bangle.loadWidgets"). So theoretically it can't loadWidgets, if it could, it would not hide it. Unless it loads the widgets from another source/library eg. then your case is valid

[thyttan]

@thyttan have a look at the new code and tell me if you feel its better to work in or not please.

Well, it looks like it's easier to follow, certainly more descriptive. I didn't really understand most of how the code worked before, to be honest :P

I don't know how the changes affect performance or function, probably yourself and @halemmerich are better judges of that. I'll throw it on my watch to try it out though.

As long as functionality doesn't break and it's not significantly slower (since that is counter to the main purpose of the app) I'm all in favor of code that is easier to understand.

[d3nd3]

I didn't really understand most of how the code worked before

I saw that you were implementing a history thing, so it makes sense that you need to fully understand if you are affecting Bangle.load vs load etc and if you are targeting uiRemove capable apps or every app eg. There was the thought before that if you don't target the non-fast load scenario, it just resets ram anyway. But your idea involves writing to storage with E.kill so it does matter to you if you target uiRemove capable app or non-uiRemove app.

I had a hard time figuring out how it worked, so I dont' blame you one bit.

[d3nd3]

@gfwilliams The lcars commit in here was not intended to be in here. Just force pushed it and its gone.

[thyttan]

I didn't really understand most of how the code worked before

I saw that you were implementing a history thing, so it makes sense that you need to fully understand if you are affecting Bangle.load vs load etc and if you are targeting uiRemove capable apps or every app

You underestimate the amount of trial-and-error my coding practice consists of ;) I just hooked into what was already there, let that logic do it's thing and add app history on top of it.

eg. There was the thought before that if you don't target the non-fast load scenario, it just resets ram anyway. But your idea involves writing to storage with E.kill so it does matter to you if you target uiRemove capable app or non-uiRemove app.

Yes, I wanted the history functionality to be agnostic to if we are fastloading or not. But it doesn't change how ram is reset on regular loads though - I just use storage to keep track of the appHistory array between ram wipes. So app history didn't add more fastloading than was already present before.

[d3nd3]

if (redraw) Bangle.drawWidgets();
This line from Bangl_setUI_Q3.js is problematic for hiding the widgets also.

What is the main reasoning why apps which do not load widgets cannot be fastload into? The method to detect if they load widgets will depend on setUI({mode:"custom",back:()=>{}); too, because back is a widget. Does that rule apply to the back widget too or just more memory hungry widgets in general?

Is it because you cannot know in advance whether they load widgets, thus you cannot remove them even if you wanted to? I think the solution would be to unload the widgets by default. What is the issue with that? That not every widget supports a functional remove callback?

[App1 loads widgets] -> [fastload into App2 (setUI() && eval(app2))] -> [App2 doesn't load widgets]

Also where is the code which clears the display on fastload? I cant' seem to find it.

[halemmerich]

I like the structural changes to the code, but did not yet try it out on my bangle.

With regard to "unloading" widgets, I have toyed around with that and could not get it to work properly. There are timeouts and intervals to take care of, those are handled relatively easily. But since widgets have no limits on what they can do (overriding functions, setting watches, changing global state in whatever way) it is not easy to correctly cleanup after them when unloading. I got the closest to a working solution by defining a new widget format with a remove method and handling them slightly different to the current implementation, but even that had a lot of edge cases.

[gfwilliams]

What is the main reasoning why apps which do not load widgets cannot be fastload into?

Because you can't unload widgets at the moment.

The method to detect if they load widgets will depend on setUI({mode:"custom",back:()=>{}); too, because back is a widget

No - back should not be a problem because as soon as an app is unloaded back is removed by setUI().

I think the solution would be to unload the widgets by default. What is the issue with that? That not every widget supports a functional remove callback?

Yes! Pretty much all of them don't support removal. You can't just get rid of the widget from the array, there's listeners/timeouts/watches/etc.

Why don't we "just add a remove function to all 70 widgets"? Because honestly, code quality is not always great and the chances of all widgets actually unloading themselves without leaving memory allocated or handlers attached is basically zero.

There have been a bunch of issues already reported on GitHub, the forum and to me personally caused by 'fast load' capable apps/clocks that didn't unload themselves properly already, and if you start to extend that further to widgets it'll create a raft of new bugs.

It might save you 0.1s per app load for your small selection of apps/widgets, but thousands of users will suffer instability and I'll be having to deal with it and maintain it for years to come.

Why don't you just add widgets (and hide them) in whatever app you have a problem with? Some things like recorder won't work without widgets anyway.

Also where is the code which clears the display on fastload? I cant' seem to find it.

There isn't one. To make it seem fast we just wait until the app clears the display when it boots (because all apps should redraw everything when they first start);

The last PR you did had 24 separate comments/questions from you on it. Please can you try and search the forum and GitHub before asking questions? These questions about widgets/fast load have been discussed at length several times already when the fast load stuff was put in.

I'm afraid I just don't have time to answer loads of questions every PR - I wouldn't get any work done on anything else :)