[weather] Add configuration option which weather services providers to use
ClosedPublic

Authored by kossebau on Jan 9 2018, 7:07 AM.

Details

Summary

When searching a weather station/location to select for the weather applet,
so far the user had no chance to control which weather services providers
are queried. Instead simply all are given the search string. Which can both
result in unwanted hits due to regions covered, as well as some minimal
privacy breach.

This patch adds a dropdown menu to the search form where the user can
control which services providers are used. The settings selected is stored
in the config for the given applet instance.

The setting defaults to an empty list, so the user has to opt-in to the use
of any provider.

Ideally the dropdown menu listing the providers with checkboxes would stay
open after toggling a selection, but QtComponents Menu seems to not allow
any modification of that behaviour. This is a small annoyance, but only once
in a while.

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.
kossebau created this revision.Jan 9 2018, 7:07 AM
Restricted Application added a project: Plasma. · View Herald TranscriptJan 9 2018, 7:07 AM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
kossebau requested review of this revision.Jan 9 2018, 7:07 AM
kossebau added a comment.EditedJan 9 2018, 7:12 AM

Drop-down menu looks like this (first item selected, hovering over second):

Additional advantage of the menu: makes it more transparent what services providers are currrently supported. Their names are available in translated version since ever in the plugin desktop files, so everything prepared on their side.

(And yes, no shadow for that dropdown menu. Seems QtComponents Menu only has _NET_WM_WINDOW_TYPE(ATOM) = _NET_WM_WINDOW_TYPE_NORMAL set, on my Qt 5.9.3. Anyone any insight whether that is worth filing?)

Cool.

Anyone any insight whether that is worth filing?

QtQuick Controls 1 is basically dead..

applets/weather/package/contents/ui/configWeatherStation.qml
101

Can you verify that toggling the MenuItem does not break this binding? It shouldn't cause much trouble, though, as you only change selected services in response to this being toggled, right?

149

Is that the actual icon we use for such popups? A blue flag isn't very descriptive imho, though I can see that Dolphin's "Service" menu also uses that. (I can't think of a better solution, though, other than the funnel filter icon)

applets/weather/plugin/servicelistmodel.cpp
33

Use range-for in new code, given plugins is already const, safe to do:
for (const QVariant &plugin : plugins)

87

I think it would be better to also check if it actually changed, I recall QML's "model" property not being as smart with that as I thought it would be.

applets/weather/plugin/servicelistmodel.h
55

There's a Qt::CheckStateRole, using that might save you some boilerplate code in various places

kossebau marked 4 inline comments as done.Jan 10 2018, 5:08 AM
kossebau added inline comments.
applets/weather/package/contents/ui/configWeatherStation.qml
101

Good catch.
Yes, it breaks it, also creates a binding loop which QML warns about. Somehow completely missed that. Shows why I can not put QML expert on my business card :)

Tried to solve by removing the bindings and instead adding a readonly proxy(?) property for the model item to the menu item and then updating the values of the menu item and the model item in the respective changed/toggled handlers. Seems to work, is that a proper solution? Had not found anything to copycat on a quick search, so assembled this from implementation hints across the places. How expensive is such a proxy property?

149

Agreed that the flag is quite strange.
While the icon name might be good enough, the current Breeze icon theme implementation with the flag is rather strange and get a new approach. No idea which global(?) metapher is referred to with a flag for services...

applets/weather/plugin/servicelistmodel.h
55

Thanks, missed that. Sadly no rolename set, so I still have to do this myself. But yes, saved some lines still :P

kossebau updated this revision to Diff 25049.Jan 10 2018, 5:08 AM
kossebau marked 2 inline comments as done.

integrate Kai's feedback

broulik accepted this revision.Jan 10 2018, 10:05 AM
This revision is now accepted and ready to land.Jan 10 2018, 10:05 AM
This revision was automatically updated to reflect the committed changes.