Skip to content
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

[WIP] circlesclock: load circles from clkinfo #2251

Merged
merged 1 commit into from Nov 23, 2022

Conversation

glemco
Copy link
Contributor

@glemco glemco commented Nov 9, 2022

Simple view of the closest running timer in the clock.
I cannot figure out how to properly generate the icon though, I created a 16x16 image and converted in b/w but it only works if I don't set a colour, otherwise the picture is weirdly mirrored (I bet it has something to do with the size, any hint is appreciated).
TODO:

  • differentiate among alarms and timers
  • something else?

@thyttan
Copy link
Collaborator

thyttan commented Nov 9, 2022

Just thinking out loud: Could clock_info be good for this? It's used in BWClock and LinuxClock. And for example SmplTmr provides clock_info, so maybe scheduler could/should be made to do that as well.

Also there's this discussion #2226.

Sorry, don't have a solution to your problem with the icon.

@glemco
Copy link
Contributor Author

glemco commented Nov 9, 2022

Well, maybe. The thing is that I wrote this piece of code a while ago when clkinfo wasn't there yet.
The library looks promising but I am not familiar with it.

@thyttan
Copy link
Collaborator

thyttan commented Nov 9, 2022

Yeah, it's the same for me as well ;)

Good idea about adding timer! :)

@glemco
Copy link
Contributor Author

glemco commented Nov 10, 2022

Anyway, I am still struggling with the icon: here's an example (you can try in the IDE) with the timer image I created (16x16, 1bbp, image string, inverted) and with another icon from circlesclock. Without colour they both look fine, but if I change colour the timer looks bad and doesn't even take the right colour (the code reproduces how images are rendered in the clock).

const timerIcon = atob("EBAB/////+AH8A/zz/GP+B/8P/5//D/4H/AP8A/gB/////8=");
const pressureIcon = atob("EBCBAAAAAYADwAJAAkADwAPAA8ADwAfgB+AH4AfgA8ABgAAA");
function drawPics(icon, x) {
  g.drawImage(icon, 0+x,0);
  const colorTimerIcon = {
        width: 16,
        height: 16,
        bpp: 1,
        transparent: 0,
        buffer: E.toArrayBuffer(icon),
        palette: new Uint16Array(['#fff', g.toColor('#f0f')])
      };
  g.drawImage(colorTimerIcon, 16+x,0);
}
drawPics(timerIcon, 0);
drawPics(pressureIcon, 32);

@gfwilliams
Copy link
Member

Hi - I think there are two issues here...

  • The image shouldn't be inverted when you convert it - in the example you have, if your theme background is white, you want the image itself to be black (right now the image background is black and the image foreground is transparent)
  • To change the color of the 1bpp image you don't need to redefine the image (that's doesn't work for image strings - the string you need has to be created using the 'image object' export mode). All you need to do is use setColor:
const timerIcon = atob("EBAB/////+AH8A/zz/GP+B/8P/5//D/4H/AP8A/gB/////8=");
const pressureIcon = atob("EBCBAAAAAYADwAJAAkADwAPAA8ADwAfgB+AH4AfgA8ABgAAA");
function drawPics(icon, x) {
  g.reset().drawImage(icon, 0+x,0);
  g.setColor('#f0f').drawImage(colorTimerIcon, 16+x,0);
}
drawPics(timerIcon, 0);
drawPics(pressureIcon, 32);

So this would work great if the images were converted 'uninverted'. But if you post up the source images I could do it for you.

BUT

I'm really not a fan of shoving loads of code into clock faces for extras. That's why we created clock_info and have that issue (referenced above). It makes the clocks unmaintainable.

For example, you should probably use the sched library to access sched.json - that's what it's for. Right now you're checking the t field, but not anything else (whether the alarm has already fired, what date it's set for, whether it's enabled, etc) so in all but the most minimal cases this code won't display the correct value.

... and then someone else will copy it into a new clock face and so on, and before we know it we have this not-quite-working code all over the place and it's a nightmare to fix.

Also, as far as I can tell if you install https://banglejs.com/apps/?id=smpltmr then this functionality is already part of clock_info (and it deals correctly with the different states of alarms)... If you're dead set on not using clock_info, you should probably still copy the https://github.com/espruino/BangleApps/blob/master/apps/smpltmr/clkinfo.js code to get active timers

So I guess is there a way we can modify clock_info to make it do what you need? For example as you're drawing these circles maybe we need to return the value as a number, and min/max values for each circle?

@gfwilliams gfwilliams marked this pull request as draft November 10, 2022 09:21
@glemco
Copy link
Contributor Author

glemco commented Nov 10, 2022

Well, as I'm not the developer of circlesclock, I tried to keep things as close how they are implemented already. I'd rather not change all the icons as I don't have the original pictures. pressureIcon is already there and works somehow fine both with and without colour, as I'm adding timerIcon I'd like it to behave just like the others (unless the solution is so bad and there's a valid reason to change them all).
Here's the timer icon I'm using, in case you want to try something with it.
small_timer

Regarding the data source, at the time I wrote that code (months ago), I thought it could be great to get all the circles in this clock in a standardised way (somehow like complications work in WearOS), I couldn't find anything implemented and had to do it manually.
I'm supposing clkinfo works like that so I'd definitely be up to use it, just I'm wondering why cannot the clkinfo for timers be embedded in the default sched library (or in the Alarms & Timers app) rather than in yet another app to install.

The idea I had (based on how this watchface works), is that the common interface should return a number (for the gauge) and a short text (to display inside). In this way any kind of progress-bar-like complication can be implemented easily on any watchface, I'm not sure clkinfo can do that already (is there a proper documentation for it?).

@gfwilliams
Copy link
Member

This works how I'd expect now?

const timerIcon = atob("EBABAAAAAB/4D/AMMA5wB+ADwAGAA8AH4A/wD/Af+AAAAAA=");
const pressureIcon = atob("EBCBAAAAAYADwAJAAkADwAPAA8ADwAfgB+AH4AfgA8ABgAAA");
function drawPics(icon, x) {
  g.reset().drawImage(icon, 0+x,0);
  g.setColor('#f0f').drawImage(icon, 16+x,0);
}
drawPics(timerIcon, 0);
drawPics(pressureIcon, 32);

image

I'm wondering why cannot the clkinfo for timers be embedded in the default sched library (or in the Alarms & Timers app)

Yes, I think it probably should be. I can't remember 100% but I think it's probably because it's @peerdavid who really started using clock-info, and since the simpletimer app is his I think he just added the code in there to avoid messing with a 'default' app - but maybe we could look at moving it...

the common interface should return a number (for the gauge) and a short text (to display inside). In this way any kind of progress-bar-like complication can be implemented easily on any watchface

The existing docs are at the top of https://github.com/espruino/BangleApps/blob/master/modules/clock_info.js

Right now we return an icon and text, but we could easily extend it without breaking anything to include a min/max and numerical value too... In fact I just pushed a change to add this.

@peerdavid
Copy link
Contributor

Hi,

can't remember 100% but I think it's probably because it's @peerdavid who really started using clock-info, and since the simpletimer app is his I think he just added the code in there

exactly I added this to the simpletimer app as I was not sure if we want this in the default sched library. But I totally agree with you that it would fit better in the sched library in case we want this :)

@glemco
Copy link
Contributor Author

glemco commented Nov 10, 2022

Well, I can have a check then, at least to implement it for the timer in this watchface. Thanks for fixing the icon, however if the clkinfo returns an icon too, that could even be easier (although this clock requires a 16x16, the 24x24 may be just too big).
Another nice thing of circlesclock is that some icons change based on the values (e.g. the weather condition), if that can be done too, then this watch can be redesigned to rely just on clkinfo for all data (not sure what the original developer @myxor thinks about it, I may give it a shot if I find some time).
@peerdavid if it's just a matter of moving the code from one app to the other and nothing breaks, then I guess it can help!

@peerdavid
Copy link
Contributor

peerdavid commented Nov 10, 2022

Yea I think moving the file from simpletimer to sched is enough :)

@glemco
Copy link
Contributor Author

glemco commented Nov 11, 2022

Yea I think moving the file from simpletimer to sched is enough :)

It's a little trickier as the simple timer has just one single timer (and gets it through its id), I'm playing around to see what can I do with it.
Ideally I guess it should show all active timers and alarms, but in cases like the circlesclock, we are just interested in the closest timer/alarm. The simplest trick would be to set this new field hasRange (or any other flag) only in the one we are interested in from the clkinfo itself.

@gfwilliams
Copy link
Member

clock_info can return more than one item, so I guess it could sort them by the time until they'll fire, and then list them all, nearest first. So then you get one shown, but if you swipe you can cycle through others

@glemco glemco force-pushed the devel branch 2 times, most recently from 2c6745e to ae1bf1c Compare November 16, 2022 13:32
@myxor
Copy link
Contributor

myxor commented Nov 17, 2022

@glemco feel free to make your changes to circlesclock if you want to. So far your changes look fine for me.

@gfwilliams
Copy link
Member

How's this looking now? Is it still a work in progress?

@glemco
Copy link
Contributor Author

glemco commented Nov 18, 2022

Thanks @myxor , hope not to screw it up 😅
@gfwilliams I have a better version on my master branch (don't want to mess this up with tons of forced pushes), it kind of works but still has some bugs, as soon as it looks fine I'll let you know.

@glemco
Copy link
Contributor Author

glemco commented Nov 19, 2022

I am still struggling with the icons, right now I'm getting them from the clkinfo and it works fine for the bangle stuff, however the ones from the weather clkinfo don't really work.. they always get the background color and I cannot manage to change it
I'm not sure if the images themselves are not fine for this usage or I'm doing something wrong.. (in bwclock they all work though..).
Here's the usual minimal example with those, if I don't change the background they aren't even visible (always white).

const icon1 = atob("GBiBAf/D//+B//8Y//88//88//88//88//88//8k//8k//8k//8k//8k//8k//4kf/5mf/zDP/yBP/yBP/zDP/5mf/48f/8A///D/w==");
const icon2 = atob("GBiBAf/7///z///x///g///g///Af//Af/3Af/nA//jg//B/v/B/H+A/H8A+D8AeB8AcB4AYA8AYA8AYA+A4A/B4A//4A//8B///Dw==");
const icon3 = atob("GBiBAf4f//wP//nn//Pn//Pzg//nAf/meIAOfAAefP///P//+fAAAfAAB////////wAAP4AAH///z///z//nz//nz//zj//wH//8Pw==");
function drawPics(icon, y) {
  g.reset().drawImage(icon, 0,y*24);
  g.setColor('#f0f').drawImage(icon, 24,y*24);
}
g.setColor("#888").fillRect(0,0,512,512);
drawPics(icon1, 0);
drawPics(icon2, 1);
drawPics(icon3, 2);

@thyttan
Copy link
Collaborator

thyttan commented Nov 19, 2022

they always get the background color

Is that true whatever theme you set/whatever the background color is?

@glemco
Copy link
Contributor Author

glemco commented Nov 19, 2022

Well, they are invisible both on my bangle (dark theme) and on the emulator. I didn't try anything else but the clock is supposed to change their colour according to settings (e.g. each circle has its own colour) and I cannot manage that

gfwilliams added a commit that referenced this pull request Nov 21, 2022
@gfwilliams
Copy link
Member

Just a note that you can use g.imageMetrics(img) to get some info about an image.

But it looks like the issue is that:

  • Those images are inverted
  • The transparent color is the foreground!

So I don't see how this could ever have worked properly :( I've just pushed a fix for that though!

If it helps anyone else, you can fix it with this code.

img= E.toUint8Array(img)
img[3]=0;
for (var i=4;i<img.length;i++) img[i]^=255;
img=E.toString(img);
g.reset().drawImage(img);
print(btoa(img));

@thyttan
Copy link
Collaborator

thyttan commented Nov 21, 2022

So I don't see how this could ever have worked properly

BW Clock inverts the theme at some point. Maybe that's an explanation?

@gfwilliams
Copy link
Member

BW Clock inverts the theme at some point. Maybe that's an explanation?

Ahh, that might do it! Although I'd have thought that with clock_info either weather would work and all the others are broken, or the others would be work and weather would be broken.

But maybe the icons were copied out of BW clock and not tested?

@glemco
Copy link
Contributor Author

glemco commented Nov 21, 2022

Thank you both very much! Finally I'm starting to understand a little more about the image format.
So I guess I can carry on with the clock, ignoring the images representation.

@gfwilliams
Copy link
Member

So I guess I can carry on with the clock, ignoring the images representation.

Yes - if you update weather using the development app loader that should be fixed though

@glemco glemco changed the title [WIP] circlesclock: added timer [WIP] circlesclock: load circles from clkinfo Nov 22, 2022
@glemco glemco marked this pull request as ready for review November 22, 2022 13:20
@glemco
Copy link
Contributor Author

glemco commented Nov 22, 2022

I put in this pull request only the modifications on circlesclock, in the meanwhile I changed something in some clkinfos to display data as they were in this clock before the update. Some more pull requests will follow.
I made some assumptions:

  • the field text is short enough and works fine on the circles (not really bulletproof, would it make sense to have a short field for this case?)
  • if items don't have a name, I assume we are in the case in which they are going to change over time (e.g. timers or calendar events), then I read just the first.
  • Since this watch doesn't allow user interaction after set-up from settings, clkinfos with a name are assumed to always be there (e.g. Weather/temperature) while in other cases the first item should be the most meaningful to show (e.g. closest timer, if there).

@gfwilliams
Copy link
Member

This looks great, thanks! Sounds like a good plan about detecting items changing - but if it's a worry then we could add a dynamic:true field I guess?

would it make sense to have a short field

I think if this becomes a problem, maybe - however the idea of clock_info is to display the stuff in quite a small area (I'm using 50px wide in my tests at the moment) so hopefully all the infos will be designed not to be that wide.

@gfwilliams gfwilliams merged commit 6cae944 into espruino:master Nov 23, 2022
@glemco
Copy link
Contributor Author

glemco commented Nov 23, 2022

This looks great, thanks! Sounds like a good plan about detecting items changing - but if it's a worry then we could add a dynamic:true field I guess?

That would make sense too! Later on I should be checking those indicators more carefully.

I think if this becomes a problem, maybe - however the idea of clock_info is to display the stuff in quite a small area (I'm using 50px wide in my tests at the moment) so hopefully all the infos will be designed not to be that wide.

Well, inside circles the text is really small (ideally just a number + measure unit), take the simple timer for instance: the timer value is given in the format T-XYZ m, although not a big deal, the T- are taking space and I'd avoid them on the circlesclock.
I'm also thinking about a circle for the calendar events, the event title won't ever fit in it though (but the number of hours till the event would, for instance).

Thanks for the feedback 👍

@gfwilliams
Copy link
Member

the T- are taking space and I'd avoid them on the circlesclock.

I'd be very happy if you wanted to do a PR to add an optional small field? It wouldn't break anything and as you say it could be useful

@glemco
Copy link
Contributor Author

glemco commented Nov 23, 2022

I'd be very happy if you wanted to do a PR to add an optional small field? It wouldn't break anything and as you say it could be useful

Great! I'll add it to the pipeline.

@thyttan thyttan mentioned this pull request Dec 4, 2022
@gfwilliams
Copy link
Member

Just a note that something came up in the forum about this, and it seems that these changes broke the HRM and probably anything that would auto-update: https://forum.espruino.com/conversations/382172/#comment16789739

I've made some big changes to try and strip out all the old code now and to just use the addInteractive function. It's not ideal, but I think at least it's a better staring point for it now.

@glemco
Copy link
Contributor Author

glemco commented Dec 7, 2022

Well, it clearly did (and probably the same with other sensor-based data), but wouldn't it be cleaner if the clkinfo itself was managing the activation of the sensors?
Maybe I'm playing wrong with the show()/hide() methods, but I think it would be ideal if the two were enough to have the data ready. Does it make sense?

@gfwilliams
Copy link
Member

but wouldn't it be cleaner if the clkinfo itself was managing the activation of the sensors?

Well, it does if you use addInteractive, which we're now using.

The issue is that some sensors (like HRM) don't have the data ready instantly - they have to go away and come back some time later.

If you want to interface with the clock infos at a lower level than addInteractive, you have to tell them when they're visible, and when they're not, and respond to their requests to redraw. There's been a lot of work put in to ensure they handle everything else based on that.

The real issue I see with what's done now is the colors are still per-circle, but now you can cycle through what gets shown. So I guess really the settings for color need to be per type of circle (eg, HRM, etc).

@glemco
Copy link
Contributor Author

glemco commented Dec 7, 2022

Mmh I see, looks promising. I'd not mess up with it until the interface is stable then.
However, what I liked about circlesclock (over say bwclock, which is also very nice) is that it's static, what I set stays there and I can use gestures for something else.
Swiping doesn't seem to go well with QuickLaunch (both the action and the circle change are triggered), plus it adds a quite noticeable delay every time it loads (just tried it now without checking the code, maybe that can be fixed).
If that's the way circlesclock is going to look like in the future, it's definitely worth it defining colours per circle (and maybe use sensible defaults for what isn't overridden by users).
But do you have any idea how to prevent other undesired actions? Like temporarily disable other touch receivers when a circle is selected? Not sure what's possible there

@thyttan
Copy link
Collaborator

thyttan commented Dec 7, 2022

One way around the specific problem with Quick Launch could be to add actions on double swipes in Quick Launch. That way you can reserve single swipe actions to the clock, although the clock would act on the first of the two swipes in that case. I have though about adding this to Quick Launch but haven't got around to it as I currently use Slope Clock which doesn't do anything on swipes.

@glemco
Copy link
Contributor Author

glemco commented Dec 8, 2022

Yeah that could be an option too, but honestly I'd still prefer using swipes for Quick Launch.
What about using the same taps as in bwclock (e.g. instead of a swipe up, tapping on the upper part of the screen) also here in circlesclock? Maybe with a visual feedback like little arrows once a circle is selected and starting the selection with a long-press of a circle instead of single to avoid accidental taps?

@gfwilliams
Copy link
Member

plus it adds a quite noticeable delay every time it loads

That delay has always existed, but maybe it's more noticeable now the clock draws first - the 'ring draw' algorithm just draws a gazillion circles, and takes 0.25 sec PER ring. If there are 3x full circles that ends up being 6x0.25 sec just to draw the rings!

So really someone ought to change that - when this came up I'd actually posted a ring draw algorithm to the forum but if I change it and the rings a bit different someone will likely complain.

Swiping doesn't seem to go well with QuickLaunch

Normally, swiping has no effect on clock_info, it's only when you tap and focus one of the circles. Perhaps clock_info could set handled=true on each event and QuickLaunch could check that to ensure that it doesn't fire if the event has been 'used'.

Or I believe when @peerdavid gets time he's planning to fix some outstanding issues with the clock_info in BW clock and then maybe add that code to clock_info itself, so when that happens maybe the code could be used here instead.

I think clock_info is getting big enough that maybe it could be an item in the app loader in its own right, with a settings page. In that case, what appears in each element (and the mode of interaction) could be configured.

@glemco
Copy link
Contributor Author

glemco commented Dec 8, 2022

That delay has always existed, but maybe it's more noticeable now the clock draws first - the 'ring draw' algorithm just draws a gazillion circles, and takes 0.25 sec PER ring. If there are 3x full circles that ends up being 6x0.25 sec just to draw the rings!

So really someone ought to change that - when this came up I'd actually posted a ring draw algorithm to the forum but if I change it and the rings a bit different someone will likely complain.

Damn, never noticed!
Well, you can't make everyone happy, unfortunately. Would that new algorithms make the circles much uglier? IMO since the bangle already doesn't have a particularly sharp resolution, faster circles are better. (Would it make sense a poll? or a setting in the app for accurate vs fast circles?)

Or wouldn't it be better off implemented in the Graphics library itself, something like a drawArc? Or a slice of a circle given the angle?
I just checked the code and I see now how a complex algorithm that is, but I understand how easy it can be to get inefficient code without a trivial way to draw arcs (sure it won't be trivial for the Graphics library either, but can move the complexity away in many cases).

Normally, swiping has no effect on clock_info, it's only when you tap and focus one of the circles. Perhaps clock_info could set handled=true on each event and QuickLaunch could check that to ensure that it doesn't fire if the event has been 'used'.

That looks neat. Can it be done with the current events interface or does it need an extension under?

I think clock_info is getting big enough that maybe it could be an item in the app loader in its own right, with a settings page. In that case, what appears in each element (and the mode of interaction) could be configured.

Agree, also there's no way to update the clkinfo module (or any module) without updating the app itself using it, right?
I think it's much more maintainable as standalone library as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants