[Weather] Port settings window to QQC2+Kirigami FormLayout and modernize UI
ClosedPublic

Authored by ngraham on Apr 29 2019, 1:42 PM.

Details

Summary
  • 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
Test Plan

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
ngraham created this revision.Apr 29 2019, 1:42 PM
Restricted Application added a project: Plasma. · View Herald TranscriptApr 29 2019, 1:42 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
ngraham requested review of this revision.Apr 29 2019, 1:42 PM

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.

kossebau added inline comments.Apr 29 2019, 1:57 PM
applets/weather/package/contents/ui/config/ConfigAppearance.qml
69

Actually not really correct, things are not shown _on_, but besides the icon.
No idea for better wording, but the current proposal seems confusing to me.

filipf added a subscriber: filipf.Apr 29 2019, 2:06 PM
filipf added inline comments.
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

filipf added inline comments.Apr 29 2019, 2:07 PM
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.

kossebau added inline comments.Apr 29 2019, 2:09 PM
applets/weather/package/contents/ui/config/ConfigUnits.qml
89

Which version of KDE Frameworks?

filipf added inline comments.Apr 29 2019, 2:11 PM
applets/weather/package/contents/ui/config/ConfigUnits.qml
89

5.56

ngraham updated this revision to Diff 57200.Apr 29 2019, 3:52 PM
ngraham marked 3 inline comments as done.

Address review comments

ngraham edited the test plan for this revision. (Show Details)Apr 29 2019, 3:53 PM
ngraham added inline comments.
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.

kossebau added inline comments.Apr 29 2019, 6:20 PM
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.
Besides why would there be action icons where this is not the case (at least with the Breeze style)? After all HIG says "Action and status icons [...] always use the monochrome style." (https://hig.kde.org/style/icon.html)
If other icon themes have different style ideas, that should be fine, after all that is the idea of themes to also have their own style guide, no?

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) :)

ngraham added inline comments.Apr 29 2019, 6:33 PM
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 updated this revision to Diff 57210.Apr 29 2019, 6:33 PM

Use non-symbolic version of the find-location icon

ngraham marked 4 inline comments as done.Apr 29 2019, 6:36 PM
filipf accepted this revision.Apr 30 2019, 1:32 AM

+1 for visuals, everything works too

This revision is now accepted and ready to land.Apr 30 2019, 1:32 AM
This revision was automatically updated to reflect the committed changes.

@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.

@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.

Thanks, can confirm the fix!