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

Add shorter/simpler weather condition options. #2134

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

JustScott
Copy link
Contributor

@JustScott JustScott commented Oct 3, 2024

These changes were originally created in PR #2001, but I'm spliiting them into their own PR as I believe they may be useful in other places and could be merged before the terminal watch face changes.

This change simply adds a GetSimpleCondition option as an alternative to GetCondition to return a shorter condition description. For example: returning 'Thunder' instead of 'Thunderstorm' as a way to save space in constrained areas without losing the original descriptions meaning.

There's an example GIF here of where this would be useful.

Copy link

github-actions bot commented Oct 3, 2024

Build size and comparison to main:

Section Size Difference
text 373040B 32B
data 948B 0B
bss 22536B 0B

Run in InfiniEmu

Copy link
Member

@FintasticMan FintasticMan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this!

@JustScott JustScott force-pushed the add_shorter_weather_condition_options branch from 28be2dc to 94748eb Compare November 20, 2024 14:00
case Pinetime::Controllers::SimpleWeatherService::Icons::CloudShowerHeavy:
return "Rain";
case Pinetime::Controllers::SimpleWeatherService::Icons::CloudSunRain:
return "Drizzle";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To me drizzle is quite different from rain showers, it implies fine droplets at a low intensity whereas showers can be heavy but brief (more importantly one justifies an umbrella and the other might not!). I think "Showers" might fit better or just "Rain" even

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, "Drizzle" should be changed to "Rain" in my opinion. I'm not a huge fan of "Shower" for the heavier rain case as I feel like that could be misinterpreted as a light shower as well as a heavy shower... perhaps we could change the GetCondition return to "Downpour" for CloudsShowerHeavy, and make it "Pour" for GetSimpleCondition? Just a suggestion, not sure how everyone would interpret "Pour" though.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These conditions are determined directly from the OpenWeather condition codes (which is also what Gadgetbridge uses). The actual API has a higher level of precision (light drizzle vs heavy shower and so on), but the weather apps compatible with Gadgetbridge for the most part only support a handful of the condition codes in the spec.

I would not recommend deviating from this spec, as it would compound the delta between the weather app and gadgetbridge...

@mark9064 mark9064 added the maintenance Background work label Nov 20, 2024
@mark9064 mark9064 added this to the 1.16.0 milestone Nov 20, 2024
@JustScott JustScott force-pushed the add_shorter_weather_condition_options branch from 94748eb to bcc7ca7 Compare December 15, 2024 19:47
case Pinetime::Controllers::SimpleWeatherService::Icons::Snow:
return "Snow";
case Pinetime::Controllers::SimpleWeatherService::Icons::Smog:
return "Mist";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This ("Mist") is the only questionable one. Based on the spec it seems like it should apply to anything that opacifies the atmosphere. However, most apps (and the spec icon itself) just rely on "mist" as a catch-all, so I guess it's fine.

Copy link
Contributor

@vkareh vkareh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR looks good to me and I like the simplified descriptions (plus they match the spec), but where are we expecting to call GetSimpleCondition? Are there existing or planned use cases for it? In my mind if you want a shorter condition, you can just get the icon.

@JustScott JustScott force-pushed the add_shorter_weather_condition_options branch from bcc7ca7 to 1b4c981 Compare January 30, 2025 21:05
@NeroBurner
Copy link
Contributor

@vkareh yes the short names are intended to be used for the weather info in the Terminal Watchface. See #2204

(I believe you already learned of it since your comment, but just for future readers to make this fact explicit)

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

Successfully merging this pull request may close these issues.

5 participants