-
Notifications
You must be signed in to change notification settings - Fork 1.3k
clkinfo: add filtering #4104
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
clkinfo: add filtering #4104
Conversation
|
Hey - quick question. If the user excludes a specific clock info, will that mean that apps specifically trying to load that clockInfo won't be able to? Eg. An app only uses one clock info like 'sunrise clockinfo' but everywhere else it's omitted? |
It depends how the clkinfo is loaded. If BangleApps/apps/clock_info/lib.js Lines 174 to 190 in 638d5cb
How does the app in question use (for example) sunrise clkinfo? |
|
@gfwilliams how does this look? |
| .keys(settings.exclude) | ||
| .filter(k => !(k.replace(re, "") in filterMenu)) | ||
| .forEach(k => { | ||
| delete settings.exclude[k]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One very tiny thing: suppose someone installs a clockinfo, excludes it, and then deletes it. You handle that here, but only save it if the user actually changes something in the menu. Maybe just add a changed=true; ... if (changed) save() so at least just looking at the menu would be enough to remove the entry?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good, done!
|
That looks perfect to me - really nice and tidy. I wouldn't worry about the 'what if an app used a clockinfo' thing - I'm not aware of any apps doing that, and even then someone would have to explicitly disable the clockinfo.
The easiest way to do that would be to just rewrite I wouldn't add a check for the contents of the clockinfo JSON file as that's just another read and possible delay. |
Thanks, sorted! |
|
Looks perfect - thank you! Shall we merge? |
|
Sounds good to me - thanks again! |
|
Not sure it's from this. But clock infos misbehave on my watch, fw 2v28.70 iflash. Have you noticed anything? I'll try debugging some tomorrow. Haven't used clock infos in a while so not sure when it would have started. |
|
How so? For me on regular cutting-edge fw, nothing seems out of the ordinary using this version |
|
OK - thanks! Interesting and a little reassuring :) |
|
I've not noticed anything, but haven't changed the setup after I filtered down the clkinfos. What's the misbehaviour? |
|
@thyttan What's the problem you're having? Recently, my ClockInfos started bugging out too, and wouldn't update the screen to show the new clockInfo, saying : Uncaught Error: Function code is undefined, not a string
at menuHideItem (clock_info:293:23)
itm.hide(options);
^
at clock_info:327:31
menuHideItem(oldMenuItem);Even though it was never a problem before, which now crashes my clock face each time. I had updated to this version prior, but the issue wasn't there, and now it just randomly is showing up... |
|
Yes, I think we're seing the same thing now then 🙃 Would you have the time to file a bug report @RKBoss6? |
|
Yeah, I can get on that in a bit! |
|
Ah, interesting. Can you file it on the Espruino project? The idea was to detect broken functions but I guess the interpreter decides not even to try allocating a code variable if the function is empty |
|
Yeah, I can copy the issue over to the espruino repo |
|
Just fixed that issue: espruino/Espruino#2681 |
This allows users to hide clkinfos they aren't using. No extra cost since we use the clkinfo cache
Closes espruino/EspruinoAppLoaderCore#83
clock_info.json, need to force the boot code regeneration/bootupdate.jsto run