- Port to QQC2+Kirigami FormLayout style
- Adjust some strings to sound a bit more natural
- Hide "Compact Mode" header since all the settings currently apply to compact mode; it only makes sense to display this when there's an additional mode to contrast with it
Details
- Reviewers
kossebau filipf - Group Reviewers
VDG Plasma - Maniphest Tasks
- T10586: Modernize widget configuration settings
- Commits
- R114:007a86a603df: [Weather] Port settings window to QQC2+Kirigami FormLayout and modernize UI
Weather station page, before selecting a station:
Weather station page, after selecting a station:
Appearance page:
Units page:
The blank Visibility combobox is a pre-existing bug not introduced (or fixed)
with this patch.
Diff Detail
- Repository
- R114 Plasma Addons
- Branch
- modernize-settings-window (branched from master)
- Lint
No Linters Available - Unit
No Unit Test Coverage - Build Status
Buildable 11340 Build 11358: arc lint + arc unit
Thanks for the patch, good to have consistency, Cannot test the next days, but could not spot anything raising my eyebrows on a quick look, so if you feel confident, please go ahead and push for field testing ;)
About the blank Visibility combobox, that works for me in Plasma 5.15. Must be some regression elsewhere, please file a bug if you can, I will try to look at it next week.
applets/weather/package/contents/ui/config/ConfigWeatherStation.qml | ||
---|---|---|
85 | While I see this: what are all those "-symbolic" variants and when would one use it? Is that documented anywhere? I expeted something on hig.kde.org, but found nothing. |
applets/weather/package/contents/ui/config/ConfigAppearance.qml | ||
---|---|---|
69 | Actually not really correct, things are not shown _on_, but besides the icon. |
applets/weather/package/contents/ui/config/ConfigUnits.qml | ||
---|---|---|
89 | It was the same before the patch, but how come there's no preselected value for this option? | |
applets/weather/package/contents/ui/config/ConfigWeatherStation.qml | ||
69 | selectButton is not horizontally aligned with the combobox below it. To fix it I think you can do something like visible: source.length > 0 for this label. However the button is still not as wide as the combobox, so for that you'd want to (see next inline comment): | |
84 | add Layout.fillWidth: true here |
applets/weather/package/contents/ui/config/ConfigUnits.qml | ||
---|---|---|
89 | Ignore this, but I can add I'm hitting this bug in 5.15 as well. |
applets/weather/package/contents/ui/config/ConfigUnits.qml | ||
---|---|---|
89 | Which version of KDE Frameworks? |
applets/weather/package/contents/ui/config/ConfigUnits.qml | ||
---|---|---|
89 | 5.56 |
applets/weather/package/contents/ui/config/ConfigWeatherStation.qml | ||
---|---|---|
85 | The -symbolic suffix means "this icon is always supposed to be monochrome". However in this case I used the -symbolic version simply because there was no non-symbolic version in Breeze icons. However it's not semantically incorrect to use that here anyway since buttons are generally only supposed to have monochrome icons in the first place. |
applets/weather/package/contents/ui/config/ConfigWeatherStation.qml | ||
---|---|---|
85 | Where is that documented? Asking because as developer writing new code one day I am now very confused what to use. So if possible, I would prefer that on changing this icon this is backed by something proper documented (and fixed where needed, e.g. ensuring there is an action icon with a normal name if needed) :) |
applets/weather/package/contents/ui/config/ConfigWeatherStation.qml | ||
---|---|---|
85 | You're not the only one confused. :) It's a bit of a mess at the moment, TBH. See T10413 For now I've added a normally-named icon without the -symbolic suffix to breeze icons (in Frameworks 5.58, so we can use it), and we'll use that here. |
@ngraham @filipf For the Visibility combobox, could you test if changing "ml" to "mi" in https://phabricator.kde.org/source/kdeplasma-addons/browse/master/applets/weather/weatherapplet.cpp$97 fixes things for you? Rather sure it does, but cannot create a diff currently.
Okay, tested now myself, pretty sure this is the fix, and you ran into the bug because your locale != QLocale::MetricSystem :) So pushing fix directly now.