GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — weather: Add new app with forecast by vkareh · Pull Request #1995 · InfiniTimeOrg/InfiniTime

Conversation

@vkareh

@vkareh vkareh commented Jan 31, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

Added a new weather app that shows the current weather and forecast. If forecast is not available, it just shows the current weather.

InfiniSim_2024-02-02_145908

Building the sim requires InfiniTimeOrg/InfiniSim#138

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

minacode, yusufmte, and niklassc-xyz reacted with thumbs up emoji
All reactions
  • 👍 3 reactions
@github-actions

github-actions Bot commented Jan 31, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

Build size and comparison to main:

Section Size Difference
text 377080B 3840B
data 940B 0B
bss 63516B 0B
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@FintasticMan

FintasticMan commented Jan 31, 2024

Copy link
Copy Markdown
Member

Looks good! Is there enough space to add the ° symbol to the forecast? It looks a bit off without it IMO. It might also be nice to know the current min and max temp?

About the forecast not showing up with Breezy Weather + Gadgetbridge, could you check if it works with Tiny Weather Forecast Germany? The Gadgetbridge side should be working, so I suspect that Breezy Weather just doesn't provide that information.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

FintasticMan
FintasticMan requested changes Jan 31, 2024
View reviewed changes

@FintasticMan FintasticMan left a comment

Copy link
Copy Markdown
Member

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 small thing, for the rest this looks good!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread src/displayapp/screens/Weather.h Resolved

Uh oh!

There was an error while loading. Please reload this page.

@vkareh

vkareh commented Feb 1, 2024

Copy link
Copy Markdown
Contributor Author

Ha! Found the issue. The wrong variable was being referenced in Gadgetbridge and so was not sending forecast data. I've submitted a patch to fix it: https://codeberg.org/Freeyourgadget/Gadgetbridge/pulls/3551

@FintasticMan Thanks for reviewing, I'll start addressing these issues sometime today.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@FintasticMan

FintasticMan commented Feb 1, 2024

Copy link
Copy Markdown
Member

Oh oopsie, that was my code in GB 😅

Thanks for finding and submitting a fix.

vkareh reacted with laugh emoji
All reactions
  • 😄 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vkareh

vkareh commented Feb 1, 2024

Copy link
Copy Markdown
Contributor Author

@FintasticMan

Looks good! Is there enough space to add the ° symbol to the forecast? It looks a bit off without it IMO.

Unfortunately no. With the ° symbol things start wrapping when you get to double-digit negative temperatures. This is also already the smallest font available.

It might also be nice to know the current min and max temp?

Added that now. Also added the location. Maybe it's too busy?

I also made the temperature yellow so that it stands out a bit more from the rest.

InfiniSim_2024-02-01_102010

Thoughts?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vkareh
vkareh force-pushed the weather-app branch from f09242e to cb810c7 Compare February 1, 2024 15:27
@FintasticMan

FintasticMan commented Feb 1, 2024

Copy link
Copy Markdown
Member

I think that looks good, but I'm not exactly the best at UI design. If some more people chime in, I think that this is good! (You do still need to fix some formatting FYI. You can use the git pre-commit hook to do this automatically.)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vkareh
vkareh force-pushed the weather-app branch from cb810c7 to f5ba5dd Compare February 1, 2024 15:52
@vkareh

vkareh commented Feb 1, 2024

Copy link
Copy Markdown
Contributor Author

@FintasticMan thanks - formatting is fixed now. The sim build breaks due to a new operator override in the Forecast struct. Should be fixed by InfiniTimeOrg/InfiniSim#138

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vkareh
vkareh force-pushed the weather-app branch from f5ba5dd to 7d63158 Compare February 1, 2024 16:18
@vkareh

vkareh commented Feb 1, 2024

Copy link
Copy Markdown
Contributor Author

I removed the location display. It was causing my watch to crash and reboot. I might debug it later, but easier to remove it for now...

InfiniSim_2024-02-01_112143

FintasticMan, KaffeinatedKat, and tituscmd reacted with thumbs up emoji
All reactions
  • 👍 3 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vkareh
vkareh force-pushed the weather-app branch from 7d63158 to f6cf49a Compare February 1, 2024 16:25
@FintasticMan

FintasticMan commented Feb 1, 2024

Copy link
Copy Markdown
Member

Do positive numbers above 100 display fine? That is possible with Fahrenheit I think...

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vkareh

vkareh commented Feb 1, 2024

Copy link
Copy Markdown
Contributor Author

Yep, they look fine. The problem would be if it gets to 100 below zero (e.g. -100 has 4 characters), but we have far bigger problems if that's the case 😆

Acceptable display range would then be -99 to 999, from a purely technical point of view.

InfiniSim_2024-02-01_113440

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@minacode

minacode commented Feb 1, 2024

Copy link
Copy Markdown
Contributor

This looks very good! I am looking forward to use this app once the support in InfiniLink is ready.

One small thing: you could use InfiniTimes theme colors.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vkareh

vkareh commented Feb 1, 2024

Copy link
Copy Markdown
Contributor Author

@minacode

One small thing: you could use InfiniTimes theme colors.

I'd be more than happy to take suggestions on how to improve the thing. Here are two options using the InfiniTimeTheme colors:

InfiniSim_2024-02-01_130349 InfiniSim_2024-02-01_130434

Any preference? Other color combinations? I haven't been able to figure out how to change the color of the text in the forecast table...

KaffeinatedKat reacted with heart emoji
All reactions
  • ❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@KaffeinatedKat

KaffeinatedKat commented Feb 1, 2024

Copy link
Copy Markdown
Contributor

that looks incredible, great work. Would it be possible to have the color change based on the temp? (eg. blue for cold and yellow/red for hot). That would really help bring it together imo

JustScott reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@JustScott

JustScott commented Feb 1, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

Wow this looks great! Do you think it would be possible to fit the date and/or week day above or below each forecast. Example: 'thu', 'fri', '2/1', '2/17'?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vkareh

vkareh commented Feb 1, 2024

Copy link
Copy Markdown
Contributor Author

that looks incredible, great work. Would it be possible to have the color change based on the temp? (eg. blue for cold and yellow/red for hot). That would really help bring it together imo

Thanks! I love this idea.

Wow this looks great! Do you think it would be possible to fit the date and/or week day above or below each forecast. Example: 'thu', 'fri', '2/1', '2/17'?

I'll try, but I suspect there won't be much room for this. Also, this app is already annoyingly heavy (3388B as of last build), and calculating the days based on timestamps and then displaying that on a table will eat up a lot more, I think.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vkareh

vkareh commented Feb 1, 2024

Copy link
Copy Markdown
Contributor Author

What are your thoughts on this? Anything 0°C or below is blue, 40°C or above is red, anything else is yellowish:
InfiniSim_2024-02-01_132859 InfiniSim_2024-02-01_132904 InfiniSim_2024-02-01_133258

KaffeinatedKat, minacode, and tituscmd reacted with thumbs up emoji JustScott, minacode, and tituscmd reacted with heart emoji
All reactions
  • 👍 3 reactions
  • ❤️ 3 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@JustScott

JustScott commented Feb 1, 2024

Copy link
Copy Markdown
Contributor

Wow this looks great! Do you think it would be possible to fit the date and/or week day above or below each forecast. Example: 'thu', 'fri', '2/1', '2/17'?

I'll try, but I suspect there won't be much room for this. Also, this app is already annoyingly heavy (3388B as of last build), and calculating the days based on timestamps and then displaying that on a table will eat up a lot more, I think.

Perhaps just adding the weekday and date for today somewhere could work, like is done on the digital watch face where it's a less visible grey.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@minacode

minacode commented Feb 1, 2024

Copy link
Copy Markdown
Contributor

Maybe increase the space between the big temperature and the description above?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vkareh

vkareh commented Feb 1, 2024

Copy link
Copy Markdown
Contributor Author

How about now?
InfiniSim_2024-02-01_154859 InfiniSim_2024-02-01_155008

I tried putting the current day/time (last updated time, really) but it was hard to align everything. It was just too cluttered...

minacode reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@KaffeinatedKat

KaffeinatedKat commented Feb 1, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

that looks really awesome, we don't really need to show the current day/time because that's easily accessible elsewhere. Having the forecasted days shown is more important. Great work

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@ghost

ghost commented Feb 1, 2024 •
edited by ghost
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

What are your thoughts on this? Anything 0°C or below is blue, 40°C or above is red, anything else is yellowish:

I think blue for ~10°C and lower, and red for ~30°C and higher would be more useful.

Personally, i would prefer it to be separated somewhat like this:

  • <= 4°C: blue – water can freeze at those temperatures
  • 5-15°C: bright blue – a cool temperature-range
  • 15-22°C: white – medium-range
  • 22-28°C: yellow – like a warm spring or mild summer
  • >= 29°C: red – hot

2 more suggestions:

  • Assuming the top forecast-values are minimum and the bottom ones maximum:
    The maximum should be at the top, because it is higher.

  • Maybe also colour the forecast-values, so that one can see the temperature-ranges easily at a glance?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vkareh

vkareh commented Feb 1, 2024

Copy link
Copy Markdown
Contributor Author

@MoonlightWave-12

The maximum should be at the top, because it is higher.

Good idea. Done:
InfiniSim_2024-02-01_161858

With regards to temperature colors, anything between 0°C and 40°C starts to become really subjective. Where I live currently 5°C is a very warm winter day, whereas 30°C is a hot summer day. However, I grew up in a place where 90°F is a mild winter day.

I'm okay with your ranges, but would like some consensus before comitting to those numbers. I also propose these:

  • 0 or lower: blue (freezing)
  • 0 - 4: cyan (ice warning)
  • 4 - 32: yellow (nothing special, but color white doesn't look great here)
  • 32 or higher: red (hot)

The 32°C number is from wikipedia but we can also use 27°C

minacode and keithzg reacted with thumbs up emoji
All reactions
  • 👍 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vkareh

vkareh commented Feb 1, 2024

Copy link
Copy Markdown
Contributor Author

@MoonlightWave-12

Maybe also colour the forecast-values, so that one can see the temperature-ranges easily at a glance?

That'd be nice, but I haven't figured out how to apply color to the table cells, only the table border (which I made black to blend in with the background)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vkareh
vkareh force-pushed the weather-app branch 2 times, most recently from 698ae9a to ce5f0d9 Compare February 1, 2024 21:45
@minacode

minacode commented Feb 8, 2024

Copy link
Copy Markdown
Contributor

We could center the cells and pad them with spaces if necessary.

vkareh and tituscmd reacted with thumbs up emoji
All reactions
  • 👍 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@FintasticMan

FintasticMan commented Feb 8, 2024

Copy link
Copy Markdown
Member

Good thinking!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vkareh

vkareh commented Feb 8, 2024

Copy link
Copy Markdown
Contributor Author

Sounds like a good idea. I can try, but each cell will need to know how many digits are in the temperature above or below itself and then add padding based on that. Maybe just centering all of them is not that bad after all?

Anyway, I'll see what I can pull off next time I'm playing with this.

FintasticMan, minacode, and tituscmd reacted with thumbs up emoji
All reactions
  • 👍 3 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vkareh
vkareh force-pushed the weather-app branch from 489ade4 to 2d18237 Compare February 10, 2024 15:50
@NeroBurner NeroBurner added this to the 1.15.0 milestone Feb 10, 2024
@vkareh
vkareh force-pushed the weather-app branch from 6e541c5 to 13f46d6 Compare February 10, 2024 18:28
@vkareh

vkareh commented Feb 10, 2024

Copy link
Copy Markdown
Contributor Author

@FintasticMan @minacode - success! I managed to use character padding to align temperatures:

  • With table borders (to visualize the alignment):
    InfiniSim_2024-02-10_131558
  • Without table borders:
    InfiniSim_2024-02-10_132039
  • With more realistic data:
    InfiniSim_2024-02-10_132314 InfiniSim_2024-02-10_132253 InfiniSim_2024-02-10_132229
FintasticMan, tituscmd, and niklassc-xyz reacted with thumbs up emoji FintasticMan, minacode, escoand, tituscmd, and niklassc-xyz reacted with heart emoji
All reactions
  • 👍 3 reactions
  • ❤️ 5 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

FintasticMan
FintasticMan requested changes Feb 10, 2024
View reviewed changes
Comment thread src/displayapp/screens/Weather.cpp Resolved

Uh oh!

There was an error while loading. Please reload this page.

minacode
minacode reviewed Feb 11, 2024
View reviewed changes
Comment thread src/displayapp/screens/Weather.h Resolved

Uh oh!

There was an error while loading. Please reload this page.

JF002
JF002 requested changes Feb 11, 2024
View reviewed changes

@JF002 JF002 left a comment

Copy link
Copy Markdown
Collaborator

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'm very happy to see this new app since the protocol allows for forecast data, but we had no way of displaying those info so far!

This feature looks good overall and I guess we'll be able to merge it as soon as the remaining code change requests and review are addressed :)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread src/displayapp/screens/Weather.cpp Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread src/displayapp/screens/Weather.cpp Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread src/displayapp/screens/Weather.cpp Outdated
tempUnit = 'F';
}
temp = temp / 100 + (temp % 100 >= 50 ? 1 : 0);
maxTemp = maxTemp / 100 + (maxTemp % 100 >= 50 ? 1 : 0);

@JF002 JF002 Feb 11, 2024

Copy link
Copy Markdown
Collaborator

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 calculation is done multiple times (and it's not easy to understand what it does - rounding?). Consider moving it to a dedicated function. The name of this function will also help understanding the meaning of it.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@FintasticMan FintasticMan Feb 11, 2024

Copy link
Copy Markdown
Member

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.

Yes, this is so that it gets rounded properly. I added it to the PTS watch face, but maybe we should add a function to SimpleWeatherService that does this calculation.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@FintasticMan FintasticMan Feb 12, 2024

Copy link
Copy Markdown
Member

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've just created that function in #2015.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@vkareh vkareh Feb 12, 2024

Copy link
Copy Markdown
Contributor Author

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 added this function directly in the weather app to avoid having to push a new change to InfiniSim, but let me know if we should change this.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@FintasticMan FintasticMan Feb 13, 2024

Copy link
Copy Markdown
Member

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 it would be better to have the function in the weather service tbh, but it doesn't matter so much.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
FintasticMan
FintasticMan requested changes Feb 11, 2024
View reviewed changes
Comment thread src/displayapp/screens/Weather.cpp Resolved

Uh oh!

There was an error while loading. Please reload this page.

@vkareh
vkareh force-pushed the weather-app branch 7 times, most recently from df9306a to a259df9 Compare February 12, 2024 21:13
FintasticMan
FintasticMan approved these changes Feb 13, 2024
View reviewed changes
@vkareh
DateTimeController: Make DayOfWeekShortToStringLow static
0e57f36
This allows it to be used outside of the current datetime context and
makes it consistent with the MonthShortToStringLow function.
@vkareh
vkareh force-pushed the weather-app branch from 0009aec to 5537508 Compare February 14, 2024 18:40
@vkareh
vkareh requested a review from JF002 February 14, 2024 18:51
vkareh added 4 commits February 15, 2024 13:08
@vkareh
weather: Add new app with forecast
6f92eb1
@vkareh
weather: Colorize forecast temperatures
67300e6
@vkareh
weather: Pad forecast temperatures
560bc37
This ensures temperatures are correctly aligned with one another
@vkareh
weather: Define function to round and render temperature
b77911c
@vkareh
vkareh force-pushed the weather-app branch from 5537508 to b77911c Compare February 15, 2024 18:09
JF002
JF002 approved these changes Feb 18, 2024
View reviewed changes

@JF002 JF002 left a comment

Copy link
Copy Markdown
Collaborator

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.

LGTM! Thanks for your work @vkareh !

I've just tested it with Tiny Weather Forecast Germany and Gadgetbridge :
20240218_125006

Reminder : the current version of Gadgetbridge (0.78) has a bug that prevents forecast from being sent to the watch (https://codeberg.org/Freeyourgadget/Gadgetbridge/pulls/3551). The fix is merged, so I guess it'll be available in 0.79 (which is already tagged but not available in f-droid).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

JumpLink and tituscmd reacted with hooray emoji
All reactions
  • 🎉 2 reactions
@JF002
JF002 merged commit 6ab512a into InfiniTimeOrg:main Feb 18, 2024
@tituscmd

tituscmd commented Mar 2, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

Out of curiosity:
Are the weekdays on the bottom displaying the next week, or is it the current week?
If it is the current week, I think it would be a good idea to color the current day - maybe red?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@minacode minacode minacode left review comments
@FintasticMan FintasticMan FintasticMan approved these changes
@JF002 JF002 JF002 approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

new app This thread is about a new app

Projects

None yet

Milestone

1.15.0

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

8 participants

@vkareh @FintasticMan @minacode @KaffeinatedKat @JustScott @tituscmd @JF002 @NeroBurner
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.