Differential D18936

[weather] Fix weather Notices tab not showing
ClosedPublic
Actions

Authored by Zren on Feb 11 2019, 10:37 PM.

Details

Reviewers
kossebau
Group Reviewers
Plasma
Commits
R114:46204a5e2a84: [weather] Fix weather Notices tab not showing
Summary

Toronto, ON in the Env Canada ion currently has a weather warning. The weather widget's code currently has a bug that will only show the Notices tab if there's both a "warning" AND a "watch".

This patch also hides the empty Warnings/Watches heading, and center aligns the visible heading.

The code populating the noticesModel is found here for reference:
https://github.com/KDE/kdeplasma-addons/blame/e0f0400dc7f263a29f86cf71924de6e9e24836d0/applets/weather/package/contents/ui/main.qml#L316

We also fix the bug in the following screenshot:

I don't see any existing bugs reporting this issue:
https://bugs.kde.org/buglist.cgi?component=weather&list_id=1587716&order=bug_id%20DESC&product=kdeplasma-addons&query_format=advanced

Test Plan
  • Patched NoticesView.qml and SwitchPanel.qml in /usr/share/plasma/plasmoids/org.kde.plasma.weather/contents/ui/
  • Ran plasmawindowed org.kde.plasma.weather
  • Selected a location with a warning

Diff Detail

Repository
R114 Plasma Addons
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
Zren created this revision.Feb 11 2019, 10:37 PM
Restricted Application added a project: Plasma. · View Herald TranscriptFeb 11 2019, 10:37 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
Zren requested review of this revision.Feb 11 2019, 10:37 PM
Zren added a comment.Feb 11 2019, 10:46 PM

I just noticed that the "Warnings Issued" text is left aligned. Might as well center align it while we're at it.

Add this as a child of PlasmaExtras.Heading to debug:

Rectangle { anchors.fill: parent; border.color: "#f00"; border.width: 1; color: "transparent"; }

Zren updated this revision to Diff 51449.Feb 11 2019, 10:53 PM
Zren edited the summary of this revision. (Show Details)

Add Layout.alignment: Qt.AlignHCenter to the PlasmaExtras.Heading

kossebau accepted this revision.Feb 12 2019, 12:15 AM

Well spotted. Seemed I had fooled myself as my test data always had both warnings & watches. And I never saw notices from real services when I tested, also not with the BBC service which I use only for real, so had not hit myself.

Can you push this to 5.12 branch and then merge all the way through 5.14 & 5.15 to master in the next 12 hours? 5.14 is interesting due to debian's upcoming release seeming to stick with 5.14, so such fixes would be good to also have in that branch, to ease anyone pulling improvements from upstream.
If not, would push for you, so its also reaching the 5.12.8 release scheduled for Tuesday.

Works fine for what I quickly tested.

PS: With this working, also time to look again at using overlay emblems to the panel icon in case of warnings existing :)

applets/weather/package/contents/ui/NoticesView.qml
46

Npt sure about this one, in my test data I always had >1 warnings/watches, and left aligned then somehow looked better.

But fine to +1 this for now, we can always change it later if we finally get more real world usage by your fix and a better idea what looks good to most, other than the original developer :)

This revision is now accepted and ready to land.Feb 12 2019, 12:15 AM
kossebau added inline comments.Feb 12 2019, 12:23 AM
applets/weather/package/contents/ui/NoticesView.qml
46

IIRC my issue had been that with centering everything, the result is that all of the "Current observed status" below icon, name of tab "Details", header "Warnings"/"Watches" of section and the warnng entries all have been centered, so by the eye they somehow looked to be one big list.
By having the heading left aligned, that appearance was broken up a bit.
Though as I realize now this only works properly if there are >1 warnings/watches, in case of one item only it looks strange again indeed. Possibly needs other eye guiding helpers. Left for another round of thinking. Or real UI designers :)

Zren added a comment.Feb 12 2019, 12:31 AM

Note: Env Canada lists all their Warnings + Watches for all of Canada at https://weather.gc.ca/warnings/index_e.html should anyone need to test in the future.

Is there any other Ion besides EnvCanada that currently displays "Warnings"?

I can definitely push to 5.12 right now. I'll ping you if I have trouble with merging down to 5.14/5.15/master.

kossebau added a comment.EditedFeb 12 2019, 12:52 AM
In D18936#410267, @Zren wrote:

Note: Env Canada lists all their Warnings + Watches for all of Canada at https://weather.gc.ca/warnings/index_e.html should anyone need to test in the future.

Perhaps something to add as note in the comments of the envcan ion, so future developers have the info available?

Is there any other Ion besides EnvCanada that currently displays "Warnings"?

I do not remember, it has been some years that I looked last into warning/watches, and the last time I touched the display code I only used my test data dummy backend.

Grepping over the dataengine code only envcan seems to process related data though indeed.

I can definitely push to 5.12 right now. I'll ping you if I have trouble with merging down to 5.14/5.15/master.

Okay. Should be avail on #plasma irc as frinring unless I fall asleep soon :) (being CET timezone, but so far missed my bed :) )

Zren added a comment.Feb 12 2019, 1:10 AM

Ah, NoticesView.qml in Plasma 5.12 was slightly different.

Notice.qml used Column, so I can't apply the heading centering in the first patch. Uhg. I forgot it still used a C++ nativeInterface model back then as well so it's... difficult to test.

I'll just do do the (model[0].length > 0 || model[1].length > 0) + Notice { visibility: model.length > 0 } in the 5.12 commit. It'll probably have merge conflicts merging to 5.14 I'll need to fix which I could apply the centering in. Or should I make that a 2nd commit in the 5.14 branch?

kossebau added a comment.Feb 12 2019, 1:25 AM
In D18936#410274, @Zren wrote:

Ah, NoticesView.qml in Plasma 5.12 was slightly different.

Eek, forgot about that as well. So 5.12 is more distant than I thought.

I'll just do do the (model[0].length > 0 || model[1].length > 0) + Notice { visibility: model.length > 0 } in the 5.12 commit.

Okay, sounds good, that should fix the basic issue of not shown warnings at least.

It'll probably have merge conflicts merging to 5.14 I'll need to fix which I could apply the centering in. Or should I make that a 2nd commit in the 5.14 branch?

Whatever works best for you. Myself I usually do fixups directly in the merge commit.

Closed by commit R114:46204a5e2a84: [weather] Fix weather Notices tab not showing (authored by Zren). · Explain WhyFeb 12 2019, 2:00 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 51455

View Options

applets/weather/package/contents/ui/NoticesView.qml

Loading...