espruino / BangleApps

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

[mtnclock] Fix widgets disappearing on weather update #3586

Closed gloriousDan closed 2 months ago

gloriousDan commented 2 months ago

Fixes #3441

I found the issue for why the widget bar keeps disappearing in the mountain clock app. It's related to the weather update but the bug is within mtnclock's app.js file.

The widget setting is written within the mtnclock.json file. The weather data is written to the same file. When a weather update comes in, the file is overwritten and the showWidgets setting consequently deleted from the file which leads to the widget bar disappearing.

This should fix it. We could also read the current state of data before writing it but in my testing it worked fine like this.

gloriousDan commented 2 months ago

You can try it out on my app loader: https://gloriousdan.github.io/BangleApps/?q=mtnclock

bobrippling commented 2 months ago

Sounds good, thanks for the fix - looks like there's two lint errors if you're ok to sort those out? What do you think to merging the whole file each time, like a sort of:

writeJson(
  "mtnclock.json",
  Object.assign(
    readJson("mtnclock.json"),
    newSettings,
  )
);
gloriousDan commented 2 months ago

Good point, I thought about that too but at first implemented the simpler fix. But now I implemented your recommendations.

The linter issues should also be fixed now. Unfortunately I didn't get to set up the tests locally, so let's see if the workflows succeed this time :)

Edit: I now checked it in the espruino IDE and there were no errors anymore

gloriousDan commented 2 months ago

Can I somehow get the CI to run within in my fork?

bobrippling commented 2 months ago

Sure, I've kicked off CI here if that helps, it was because the PR was in draft. Those changes look great btw, I'll merge if you're happy with everything?

gloriousDan commented 2 months ago

Thanks, I marked it as draft because I thought of a potential error I still wanted to check/ fix. Maybe you could also help me determine wether it's a problem or not.

Since I load set the updated weather data not to the data object anymore, but to the newSettings and then write it to the mtnclock.json, I suspect that the clock only shows the updated data when the app is reloaded again. In my code the data object is only ever read from mtnclock.json at the start of the app and not updated later.

Is my understanding correct, that line 1 is only run once at the start?

I probably get around to fixing this soon, and afterwards we can merge it.

bobrippling commented 2 months ago

Yeah that's right, line 1 only runs at the start so the settings are read once - we can reassign to data instead so it remains the most up-to-date settings. Hope you don't mind - I've popped this in, let me know what you think (09bea87)

gloriousDan commented 2 months ago

That looks good, that's pretty much how I would have done it as well.

I just have one more Question concerning how the app's code is run.

When I exit the clock while opening the launcher, is the app paused in the background and resumes running or does it exit and start again from scratch after exiting the launcher?

If the former is the case, the current code might not pick up a change of the mtnclock showWidgets setting and then overwrite it with the outdated value again.

bobrippling commented 2 months ago

When I exit the clock while opening the launcher, is the app paused in the background and resumes running or does it exit and start again from scratch after exiting the launcher?

Sweet, and no you're safe - the app is restarted if you go from the launcher back to the clock, so that's all good. I'll merge - thanks for the fix!

gloriousDan commented 2 months ago

Great! Thank you for merging and your help 👍

bobrippling commented 2 months ago

Thanks for the time to put together the fix!