-
Notifications
You must be signed in to change notification settings - Fork 543
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
New example: weather demo #5601
New example: weather demo #5601
Conversation
While change callbacks are still experimental and not enabled by default, we'd like to avoid their use in examples and demos (that folks copy & paste from). Would it be possible to change the code to not use them? |
Yes, they should. I can help fix that up afterwards if necessary, no worries :) |
examples/weather-demo/Cargo.toml
Outdated
# Using different Slint configuration on Windows to disable the accessibility feature (enabled by default). | ||
# The feature caused performance issues on Windows (see https://github.com/slint-ui/slint/issues/5338). | ||
# Note: might be fixed with v1.7 |
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.
Since this is being built with "1.7", does this still apply?
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 believe so since the ticket (the one that mine was a duplicate of) is still open. But I can double-check this.
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.
Ok :). We can deal with this separately and leave this.
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.
Ok, I checked and this is fixed now in master AFAICS.
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 checked and can confirm that the issue is gone. Let me update the file.
EDIT: Done
use std::env; | ||
|
||
fn main() { | ||
env::set_var("SLINT_ENABLE_EXPERIMENTAL_FEATURES", "true"); |
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 hesitant about the use of change callbacks in the demos/examples. @ogoffart what's your opinion?
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.
Initially, we tried to avoid them. I believe there are at least one or two use cases where I could not find a workaround for not using them and still providing the functionality.
Maybe I'm missing something. Please let me know if you want me to examine this again and provide you with concrete examples.
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.
Alright, let's keep the use of the change handlers as they are :-)
The demo looks great overall, btw! Thanks for contributing it. Please see the message from GitHub about the mismatch between GitHub user and email address. You'll also see that our CI complains about copyright headers and SPDX license tags. |
ff5494a
to
4a0ed30
Compare
examples/weather-demo/Cargo.toml
Outdated
[target.'cfg(all(not(target_os = "windows"), not(target_os = "android")))'.dependencies] | ||
slint = { path = "../../api/rs/slint" } | ||
|
||
# Using different Slint configuration on Windows to disable the accessibility feature (enabled by default). | ||
# The feature caused performance issues on Windows (see https://github.com/slint-ui/slint/issues/5338). | ||
# Note: might be fixed with v1.7 | ||
[target.'cfg(target_os = "windows")'.dependencies] | ||
slint = { path = "../../api/rs/slint", default-features = false, features = [ "compat-1-2", "backend-winit", "renderer-femtovg" ] } | ||
|
||
[target.'cfg(all(not(target_arch = "wasm32"), not(target_os = "android")))'.dependencies] | ||
env_logger = "0.11.3" | ||
|
||
[target.'cfg(target_os = "android")'.dependencies] | ||
android_logger = "0.14.1" | ||
openssl = { version = "0.10", features = ["vendored"] } | ||
slint = { path = "../../api/rs/slint", features = [ "backend-android-activity-06" ] } |
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.
[target.'cfg(all(not(target_os = "windows"), not(target_os = "android")))'.dependencies] | |
slint = { path = "../../api/rs/slint" } | |
# Using different Slint configuration on Windows to disable the accessibility feature (enabled by default). | |
# The feature caused performance issues on Windows (see https://github.com/slint-ui/slint/issues/5338). | |
# Note: might be fixed with v1.7 | |
[target.'cfg(target_os = "windows")'.dependencies] | |
slint = { path = "../../api/rs/slint", default-features = false, features = [ "compat-1-2", "backend-winit", "renderer-femtovg" ] } | |
[target.'cfg(all(not(target_arch = "wasm32"), not(target_os = "android")))'.dependencies] | |
env_logger = "0.11.3" | |
[target.'cfg(target_os = "android")'.dependencies] | |
android_logger = "0.14.1" | |
openssl = { version = "0.10", features = ["vendored"] } | |
slint = { path = "../../api/rs/slint", features = [ "backend-android-activity-06" ] } | |
slint = { path = "../../api/rs/slint", features = [ "backend-android-activity-06" ] } | |
[target.'cfg(all(not(target_arch = "wasm32"), not(target_os = "android")))'.dependencies] | |
env_logger = "0.11.3" | |
[target.'cfg(target_os = "android")'.dependencies] | |
android_logger = "0.14.1" | |
openssl = { version = "0.10", features = ["vendored"] } |
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.
Done
For the "reuse" tool failures in the CI:
For every file in the repo we need to know who the copyright holder is and under what license we're using the file(s). For our examples we typically record this in the file |
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 file is rather large. Is this needed for some icons? Would it be possible to extract them as .svgs instead? We use icons from font-awesome in a few other place and opted into shipping smaller .svg files instead of the whole ttf.
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.
Ok, I can change this. From what I remember, they are only a few usages.
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.
Done
examples/weather-demo/Cargo.toml
Outdated
|
||
[package] | ||
name = "rusty-weather" | ||
version = "1.0.0" |
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 we usually match the version with the rest of Slint in the repo :)
version = "1.0.0" | |
version = "1.7.0" |
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.
Done
|
||
const CITIES_STORED_FILE_NAME: &str = "cities_data.json"; | ||
const ORGANIZATION_QUALIFIER: &str = "com"; // have to match android app name in cargo.toml | ||
const ORGANIZATION_NAME: &str = "felgo.demos"; // have to match android app name in cargo.toml |
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 this needs changing/adapting? Just a drive-by thought.
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 guess it makes sense, but let me double-check this and get back to you 🙂
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.
Sorry for the delay. I was offline last week.
I changed this. Please let me know if the new version is okay with you.
</p> | ||
|
||
## Weather data | ||
To enable real weather data from the [OpenWeather](https://openweathermap.org/) API, you must provide the `RUSTY_WEATHER_API_KEY` environment variable with your API key at build time. The [OpenCall API](https://openweathermap.org/price#onecall) subscription is required. |
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 thought is beyond the scope of this PR: We have similar needs in the energy monitor demo, where we use weatherapi.com and we've got an API key in the CI set, so that the demos built have weather data support. It might make sense to unify these, i.e. either to switch the energy monitor over to use openweathermap.org or the other way around. The key from weatherapi.com is free.
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.
That sounds like a good idea 🙂
Just so you know, the OpenWeather API is also mostly free. Only the OneCall API we used requires a paid subscription, and it is only paid for calls above 1000 per day.
The other APIs available for free keys can provide similar functionalities. We only switched to the OneCall version because we were facing issues. However, as we later discovered, those were issues in the crate we used rather than in the API itself.
4a0ed30
to
bb6af49
Compare
Regarding the "reuse" specification, I'm still waiting for confirmation on our internal images licensing. Anything else should already be covered. |
bb6af49
to
b2afa3a
Compare
b2afa3a
to
4a8106c
Compare
4a8106c
to
67a0990
Compare
I think it's all done now; I synced with the latest master, and the CI is passing. |
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.
Awesome, thank you!
Hi,
Please let me know if something needs to be added.
I assumed that links like:
https://slint.dev/snapshots/master/demos/weather-demo/
or
https://slint.dev/snapshots/master/editor?load_url=https://raw.githubusercontent.com/slint-ui/slint/master/examples/weather-demo/ui/main.slint
would start working when the PR is merged into the master and the wasm is built with the GitHub actions.
Let me know if I'm wrong.
Anyway, the preview with slint-pad is not working because we're using changed callbacks in the demo (see here). Should I remove the link, then?