-
Notifications
You must be signed in to change notification settings - Fork 34
Feature/weather uv index #426
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
Conversation
Thanks for the PRs @sebastinto Also, is the UV index always available? |
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.
Pull Request Overview
This PR adds optional UV index information to the weather command functionality. Users can now enable/disable UV index display through a new setting in the settings screen.
- Added a new toggle setting to control UV index display in weather reports
- Modified weather command to conditionally show UV index based on user preference
- Added translations for the new feature across all supported languages (English, Italian, Russian, Spanish, Ukrainian, Serbian)
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
app/src/main/res/values/strings.xml | Added English strings for UV index weather display and settings toggle |
app/src/main/res/values-*/strings.xml | Added translations for UV index strings in Ukrainian, Serbian, Russian, Italian, and Spanish |
app/src/main/res/layout/activity_settings.xml | Added UV index toggle switch to settings UI |
app/src/main/java/com/coderGtm/yantra/commands/weather/Helper.kt | Added conditional UV index display logic in weather command |
app/src/main/java/com/coderGtm/yantra/activities/SettingsActivity.kt | Added UV index preference handling and switch initialization |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
My understanding is that by default it is but can be disabled / enabled:
|
But, at this point can't other fields be added too? Maybe under a setting "advanced weather data" or something like this? |
Yeah ideally you'd have a way to select which data you want displayed and in which order. A Implementing something like this would require a substantial refactor though. 😅 |
At this point a config file could be added |
That or maybe the config could be saved as a json string to I'd probably define the weather config as a data class and serialize it using kotlinx.serialization. Something roughly like this (off the top of my head)? import kotlinx.serialization.Serializable
import kotlinx.serialization.encodeToString
import kotlinx.serialization.json.Json
import kotlinx.serialization.decodeFromString
// define the config data class
@Serializable
data class Config(
val someProp: Int,
val anotherProp: String,
// additional fields
)
// save to SharedPrefs
val jsonString = Json.encodeToString(Config)
sharedPrefs.edit().putString("config", jsonString).apply()
// read from SharedPrefs
val jsonString = sharedPrefs.getString("key", null)
val obj = jsonString?.let { Json.decodeFromString<Config>(it) } A challenge would be to design the architecture so that handleResponse doesn't become a maze of conditional statements. Also., I wonder if the weather data could be configured entirely via commands which would be neat but I can't quite define what that could look like. |
Weather config is a possible solution but I think just using flags in the weather command will be an easier and familiar experience to users. We can modify the weather command to display all data by default. Users can then use flags like -temp -uv -humidity to display only specific parameters. Similar to sysinfo and other commands. What do you guys think about it? |
I think is good, in the end people will end up making an alias anyway to automatically have the weather of their city |
That's what I meant when I said " I wonder if the weather data could be configured entirely via commands". This would definitely be more in line with the concept of the launcher!
Since there are 100+ fields available, that'd be a wild output! Wouldn't it be better to display the current fields as default? This would ensure backward compatibility / prevent forcing a change on current users. I think most users would be satisfied with the weather command output only containing the current fields as a sort "summary". Then power users can use flags and aliases for granular control over what they want returned. But maybe everyone who uses Yantra Launcher is a "power user"? 😄 |
I do not want to judge anyone but i seen some pretty wild(or stupid) questions on the discord server XD so i'd say no
I agree with you |
This brings us back to config file. A default list of args cannot be specified for everyone. I get the backward compatibility and summary part and agree with it, but showing some parameters by default without any args is unpredictable. So a config using shared preferences in the settings where our default args are pre-defined is a more predictable approach for users. Something like a list with checkboxes or an output template. Yes we lose the simplicity of cmd line args here, but then there is also the problem of handling location names with spaces. For example, in
We have no way to deterministically recognise when the flags begin since Yantra Launcher does not use quotes for values with space, ofc for simplicity. |
I think I can make this work! I really like the simplicity of cmd line args:
Would you be open to a PR implementing this? I would close the current one since it would no longer be relevant. |
How will you approach it. What if the location name contains a hyphen in it? |
TBD but I'd probably take a test-driven development approach to ensure edge cases like the one you mentioned are handled correctly. This seems like a fun challenge! |
Ok if you can handle it then I am definitely open to a PR. Would be the best of both worlds. All the best and let me know if you need any assistance! |
Supplanted by #428 |
Summary
Adds optional UV index info to the weather command.
Changes
Screenshots
Testing