-
-
Notifications
You must be signed in to change notification settings - Fork 948
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
Terminal Watchface Improvements and added Weather. #2001
base: main
Are you sure you want to change the base?
Conversation
Build size and comparison to main:
|
This is really cool, I like the graying out of n/a data! How does it truncate if you have longer weather condition names, like Scattered Clouds or something? Also, there seems to be a One suggestion though: the PS1 could be, say, lightGray so that it doesn't overpower the rest of the data, maybe then the time can be white so that it's easier to see at a quick glance? I haven't really used this face a lot, but this PR might tip the scale for me. |
|
||
using namespace Pinetime::Applications::Screens; | ||
|
||
namespace { | ||
lv_color_t temperatureColor(int16_t temperature) { |
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.
This makes me think that we can make this part of the InfiniTimeTheme...
@vkareh I'm confused about the #'s at the end of the lines as well, they were already there so i didn't want to remove them incase there was a reason. As far as the trunacation, I modified your |
// We lock satuation and brightness at 100% and traverse the cilinder | ||
// between red and green, thus avoiding the darker RGB on medium battery | ||
// charges and giving us a much nicer color range. | ||
uint8_t hue = batteryPercentRemaining.Get() * 120 / 100; |
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.
Any chance this can live elsewhere and we can reuse it here? Not super important, since my color PR may not ever be merged though 😛
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.
Maybe in the theme too?
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.
Should I add it to InfiniTimeTheme.h
in this PR, or should that be a seperate PR?
~WatchFaceTerminal() override; | ||
|
||
void Refresh() override; | ||
|
||
private: | ||
Utility::DirtyValue<int> batteryPercentRemaining {}; | ||
Utility::DirtyValue<uint8_t> hue {}; |
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.
I think this can be a local variable inside the function, no need to track its state at the class level
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.
Fixed.
case Pinetime::Controllers::SimpleWeatherService::Icons::Clouds: | ||
return "Cloudy"; | ||
case Pinetime::Controllers::SimpleWeatherService::Icons::BrokenClouds: | ||
return "Cloudy"; |
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.
I see what you did here. Clever.
It will conflict with the weather app PR, so we'll need to find a good middle ground if both make it into main
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.
Okay, I see this is a different, simplified, function. Makes sense now! 👍
66b10c5
to
44d3f52
Compare
In fully agree. Light gray looks better |
I agree with Albey above that I prefer the labels being white. The # symbols come from the way that the text was being coloured. It was done with I definitely prefer lightGray to gray, but perhaps if the labels are the same colour white works too? Is there a reason that the same function for the weather description from the forecast app can't be used here? Are some words too long to fit? I quite liked the colour variety of the current watch face. Maybe we can experiment with the colours a bit? |
@FintasticMan Ah that's where the # came from, and yes more than a one word weather description overflows. Here's a version of the face with the labels white (#ffffff), the time and date using their original color value, and the prompts still using the lightGray value. |
The extra line is nice and obvious. Only changing colour wouldn't communicate to new users of the face why it has done so. Moreso, communicating any information through colour alone makes it inaccessible to colourblind users. That said, I'm not opposed to adding that colour to the mail text. That could make it stand out even better than it does now, I think. Was there another reason you suggested changing the colour instead of having the line? |
@AlbeyAmakiir I just thought it was a bit out of place for a terminal watch face to have random text above the first prompt appear. But you're right, just changing the color wouldn't work, I hadn't taken into account color blind users. I think having a more realistic terminal message could fit the lore a little better, I made an example of a background 'notify' job completing in the picture below, but if anyone has a better one let me know. (I'm also completely fine with leaving it as it was if that's how people prefer it). |
Hmm, I'm not sure. Having it at the top does have the advantage that it's right next to the direction in which you need to swipe for notifications, which I only noticed because your alternative is at the other extreme. But you're right it's a little off-lore. I also considered a So it might be best to leave it as is, but I'm chill if others feel differently. I'm not sure how to weigh up the pros and cons of all these options. |
char tempUnit = 'C'; | ||
lv_obj_set_style_local_text_color(weather, LV_LABEL_PART_MAIN, LV_STATE_DEFAULT, temperatureColor(temp)); | ||
if (settingsController.GetWeatherFormat() == Controllers::Settings::WeatherFormat::Imperial) { | ||
condition = Symbols::GetSimpleCondition(optCurrentWeather->iconId); |
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.
This is in the wrong scope. It also doesn't need DirtyValue tracking.
I suggest just calling it in the lv_label_set_text_fmt
:
lv_label_set_text_fmt(weather, "#ffffff [WTHR]# %i°%c %s ", temp / 100, tempUnit, Symbols::GetSimpleCondition(optCurrentWeather->iconId));
Then get rid of the condition
variable.
case Pinetime::Controllers::SimpleWeatherService::Icons::Snow: | ||
return "Snowy"; | ||
case Pinetime::Controllers::SimpleWeatherService::Icons::Smog: | ||
return "Misty"; |
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.
This line is misaligned
case Pinetime::Controllers::SimpleWeatherService::Icons::Sun: | ||
return "Clear"; | ||
case Pinetime::Controllers::SimpleWeatherService::Icons::CloudsSun: | ||
return "Cloudy"; | ||
case Pinetime::Controllers::SimpleWeatherService::Icons::Clouds: | ||
return "Cloudy"; | ||
case Pinetime::Controllers::SimpleWeatherService::Icons::BrokenClouds: | ||
return "Cloudy"; | ||
case Pinetime::Controllers::SimpleWeatherService::Icons::CloudShowerHeavy: | ||
return "Rainy"; | ||
case Pinetime::Controllers::SimpleWeatherService::Icons::CloudSunRain: | ||
return "Rainy"; | ||
case Pinetime::Controllers::SimpleWeatherService::Icons::Thunderstorm: | ||
return "Stormy"; | ||
case Pinetime::Controllers::SimpleWeatherService::Icons::Snow: | ||
return "Snowy"; | ||
case Pinetime::Controllers::SimpleWeatherService::Icons::Smog: | ||
return "Misty"; |
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.
I think these should follow the OWM main group names more closely:
- Clear
- Clouds
- Clouds
- Clouds
- Drizzle
- Rain
- Thunder(storm) <--- Thunderstorm doesn't fit, but Thunder does
- Snow
- Mist <--- This one doesn't have a single name for the main group, but I've seen several apps defaulting to Mist, since it's the first one in the atmosphere condition group.
uint8_t hue = batteryPercentRemaining.Get() * 120 / 100; | ||
lv_obj_set_style_local_text_color(batteryValue, LV_LABEL_PART_MAIN, LV_STATE_DEFAULT, lv_color_hsv_to_rgb(hue, 100, 100)); | ||
lv_label_set_text_fmt(batteryValue, "#ffffff [BATT]# %d%%", batteryPercentRemaining.Get()); | ||
if (batteryController.IsPowerPresent()) { |
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.
I think this should call IsCharging()
instead, as once it reaches 100% the controller will stop charging once the battery is full. Also, displaying 100% Charging
looks odd.
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.
I agree.
I agree with @AlbeyAmakiir with regards to accessibility. Color-coding on a notification is not great UX. The Playing with a terminal, I recreated what the flow would look like this (and yes, I modified my
Based on this, the signal message can be either before the top prompt (in the case of using Translating this to the watch face, I think we have two options that could work and satisfy the lore: I personally prefer the first one. |
1c7fcb9
to
dbce980
Compare
I won't know for sure until I see it on the actual watch screen, but on my computer the 3-bit colours look a bit too harsh. Maybe it'll be fine irl, though? I'm not sure. I definitely prefer the notify text being at the top over the bottom. (Also, I really appreciate that you actually tested that on a terminal. Fantastic. XD ) |
@AlbeyAmakiir I agree - the 3-bit colours are not great, to be honest. |
I would say it would be best to break the weather display out of this PR into its own, so that we can discuss that separately from changing the UI. |
dbce980
to
9fa41c4
Compare
I much prefer the white prompts. It fits the terminal theme better (at least in my experience) and using less colours makes the face as a whole less busy. |
9fa41c4
to
241e4ef
Compare
+1 having weather on terminal watchface would be great, as it is more precise for the number of steps than pinetimestyle |
Use @vkareh's new weather forecast app as a template to implement the weather into the terminal watch face. Use InfintimeTheme Colors instead of hardcoded hex values. Reorder code to match the order of the UI. Implement @vkareh's variable battery icon color to the battery percentage text. Added a new IninftimeTheme color, gray, and turn certain values gray when they contain no data, like weather.
Labels are now a static white #ffffff, while the values can still dynamically change color.
defining it first. changed the 'you have mail' to '[1]+ Notify, to better align with terminal lore. Use `IsCharging` instead of `IsPowerPresent` to stop displaying the 'Charging' text once the battery's full. Update the return values for weather condition in `GetSimpleCondition` to match the standard names used in the rest of the project.
241e4ef
to
3341247
Compare
I created a new PR without the weather changes: #2135, I also created a PR for the simpler weather condition change: #2134 Now I'm wondering if it would be better to create a new PR for just the weather changes on the current main branch, and close this PR, or to reset the code in this PRs branch to the main branch, and then add the changes. In my opinion it would be more clean to just create a new PR and close this one.. but I'm often wrong lol. |
I don't personally use the terminal watch face, so I'm looking for suggestions on what colors you guys want, and if I should change certain text like the
steps#
in the step counter.