Review Request 127193: Revival of Weather applet (for Plasma 5.6)
Kai Uwe Broulik
kde at privat.broulik.de
Sat Feb 27 10:27:21 UTC 2016
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127193/#review92824
-----------------------------------------------------------
Nice!
Regarding the ComboBox issues, we know, it's horrible, sizing is broken and the popup is always white. Not much we can do here :(
I gave the applet a try and there's a couple of issues:
- It seems Plasma doesn't properly setup the widget. If I drag it to the desktop it works but if I double click to add it it says "Error loading widget: Package does not exist", same when I add it to system tray.
- It doesn't properly load a new weather station, the busy indicator just spins forever; I need to restart Plasma for the weather to show up.
- The popup size when placed in a panel and the default size on the desktop is way too small
- The plasmoid is just empty when it is not configured; Plasmoid has a configurationRequired property which shows a "You need to configure this thing first [Configure]"
- It sometimes seems to forget the weather station when I open settings and when I then save settings the applet is blank again
- Sometimes it doesn't find any weather stations in the search
- The tooltip subtext is blank (but not empty, ie. I get an additional empty line)
- The strip with the temperatures needs to be taller, text barely fits in there. The weather icons also leak outside the strip but this looks like it might be intended.
It also looks like we need some Breeze weather icons :)
applets/weather/package/contents/ui/DetailsView.qml (line 25)
<https://git.reviewboard.kde.org/r/127193/#comment63263>
This seems unused
applets/weather/package/contents/ui/DetailsView.qml (line 41)
<https://git.reviewboard.kde.org/r/127193/#comment63260>
visible: !!elementId ?
applets/weather/package/contents/ui/DetailsView.qml (line 46)
<https://git.reviewboard.kde.org/r/127193/#comment63262>
Better replace the containing Item {} on line 30 by a Row with a spacing, saves you some anchoring head ache
applets/weather/package/contents/ui/DetailsView.qml (line 47)
<https://git.reviewboard.kde.org/r/127193/#comment63261>
avoid pixel sizes, use units.smallSpacing for example
applets/weather/package/contents/ui/DetailsView.qml (line 50)
<https://git.reviewboard.kde.org/r/127193/#comment63259>
Use PlasmaComponents.Label instead of plain Text
This automatically follows plasma color and font settings
applets/weather/package/contents/ui/FiveDaysView.qml (lines 46 - 47)
<https://git.reviewboard.kde.org/r/127193/#comment63265>
Braces, please
applets/weather/package/contents/ui/FiveDaysView.qml (lines 52 - 53)
<https://git.reviewboard.kde.org/r/127193/#comment63266>
Braces
applets/weather/package/contents/ui/FiveDaysView.qml (line 62)
<https://git.reviewboard.kde.org/r/127193/#comment63268>
Again, PlasmaComponents.Label
applets/weather/package/contents/ui/FiveDaysView.qml (lines 63 - 65)
<https://git.reviewboard.kde.org/r/127193/#comment63267>
This could be inlined into the opacity property below
applets/weather/package/contents/ui/Notice.qml (line 27)
<https://git.reviewboard.kde.org/r/127193/#comment63269>
PlasmaComponents.Label
applets/weather/package/contents/ui/Notice.qml (line 36)
<https://git.reviewboard.kde.org/r/127193/#comment63270>
PlasmaComponents.Label
applets/weather/package/contents/ui/Notice.qml (line 43)
<https://git.reviewboard.kde.org/r/127193/#comment63271>
Qt.openUrlExternally(...)
applets/weather/package/contents/ui/NoticesView.qml (line 23)
<https://git.reviewboard.kde.org/r/127193/#comment63272>
property var instead of variant
applets/weather/package/contents/ui/NoticesView.qml (line 25)
<https://git.reviewboard.kde.org/r/127193/#comment63273>
units.largeSpacing or so
applets/weather/package/contents/ui/NoticesView.qml (line 27)
<https://git.reviewboard.kde.org/r/127193/#comment63274>
Avoid clip if not absolutely neccessary
applets/weather/package/contents/ui/TopPanel.qml (line 23)
<https://git.reviewboard.kde.org/r/127193/#comment63276>
property var
applets/weather/package/contents/ui/TopPanel.qml (line 30)
<https://git.reviewboard.kde.org/r/127193/#comment63275>
I think so?
applets/weather/package/contents/ui/TopPanel.qml (line 36)
<https://git.reviewboard.kde.org/r/127193/#comment63277>
PlasmaComponents.Label
applets/weather/package/contents/ui/TopPanel.qml (line 42)
<https://git.reviewboard.kde.org/r/127193/#comment63278>
units.smallSpacing or so
applets/weather/package/contents/ui/TopPanel.qml (line 43)
<https://git.reviewboard.kde.org/r/127193/#comment63279>
Umm, okay? Perhaps Math.round(...)
applets/weather/package/contents/ui/TopPanel.qml (line 50)
<https://git.reviewboard.kde.org/r/127193/#comment63280>
Make that a binding:
font.pointSize: theme.defaultFont.pointSize * 1.4
Or have a look at PlasmaExtras.Heading {} it provides a bunch of pre-defined "levels" (similar to html H1..H6 tags)
applets/weather/package/contents/ui/TopPanel.qml (line 69)
<https://git.reviewboard.kde.org/r/127193/#comment63281>
units.smallSpacing
applets/weather/package/contents/ui/WeatherListView.qml (line 40)
<https://git.reviewboard.kde.org/r/127193/#comment63264>
Qt.color(theme.textColor.r, theme.textColor.g, theme.textColor.b, youralphahere)
applets/weather/package/contents/ui/WeatherListView.qml (line 44)
<https://git.reviewboard.kde.org/r/127193/#comment63282>
property var
applets/weather/package/contents/ui/configGeneral.qml (line 34)
<https://git.reviewboard.kde.org/r/127193/#comment63286>
What's the rationale for not using plasmoid.configuration?
applets/weather/package/contents/ui/configGeneral.qml (line 48)
<https://git.reviewboard.kde.org/r/127193/#comment63287>
I think abusing the ComboBox for the results is a terrible idea and it always was. Can we do better than that, like a search input and then a TableView of results below or something like that?
applets/weather/package/contents/ui/configGeneral.qml (line 103)
<https://git.reviewboard.kde.org/r/127193/#comment63283>
BusyIndicator ?
applets/weather/package/contents/ui/configGeneral.qml (line 160)
<https://git.reviewboard.kde.org/r/127193/#comment63285>
Meh, I would have expected ComboBox to support that out of the box since display is Qt's default role name for the built-in display role.
applets/weather/package/contents/ui/configGeneral.qml (line 161)
<https://git.reviewboard.kde.org/r/127193/#comment63284>
If you're having problems with binding loops there, try the onActivated signal which is only emitted if the *user* changes the value, not if you programmatically change it.
applets/weather/package/contents/ui/main.qml (lines 27 - 31)
<https://git.reviewboard.kde.org/r/127193/#comment63290>
Base those sizes on units.gridUnit * n
applets/weather/package/contents/ui/main.qml (line 33)
<https://git.reviewboard.kde.org/r/127193/#comment63291>
I thin this is not needed
applets/weather/package/contents/ui/main.qml (line 34)
<https://git.reviewboard.kde.org/r/127193/#comment63288>
Don't clip if you don't have to
applets/weather/package/contents/ui/main.qml (line 40)
<https://git.reviewboard.kde.org/r/127193/#comment63289>
Please make use of Plasmoid.fullRepresentation and Plasmoid.compactRepresentation
applets/weather/package/contents/ui/main.qml (line 46)
<https://git.reviewboard.kde.org/r/127193/#comment63292>
units.smallSpacing
The applet should have some inner padding automatically, however, I think
applets/weather/package/contents/ui/main.qml (lines 82 - 83)
<https://git.reviewboard.kde.org/r/127193/#comment63293>
units...
applets/weather/package/contents/ui/main.qml (line 105)
<https://git.reviewboard.kde.org/r/127193/#comment63294>
PlasmaComponents.Label
applets/weather/package/contents/ui/main.qml (line 110)
<https://git.reviewboard.kde.org/r/127193/#comment63295>
units
applets/weather/package/metadata.desktop (line 56)
<https://git.reviewboard.kde.org/r/127193/#comment63257>
That's not you
applets/weather/package/metadata.desktop (line 61)
<https://git.reviewboard.kde.org/r/127193/#comment63256>
Do we have this category?
applets/weather/package/metadata.desktop (line 64)
<https://git.reviewboard.kde.org/r/127193/#comment63258>
Please don't enable it by default, otherwise every Plasma user on the planet will have it in his system tray after the next update :)
applets/weather/plugin/abstractunitlistmodel.cpp (line 28)
<https://git.reviewboard.kde.org/r/127193/#comment63296>
No space after !
applets/weather/plugin/abstractunitlistmodel.cpp (line 52)
<https://git.reviewboard.kde.org/r/127193/#comment63297>
std::find_if ?
applets/weather/plugin/locationlistmodel.cpp (line 31)
<https://git.reviewboard.kde.org/r/127193/#comment63298>
nullptr
Also, you could initialize that in the header:
m_dataengine = nullptr
applets/weather/plugin/locationlistmodel.cpp (lines 119 - 120)
<https://git.reviewboard.kde.org/r/127193/#comment63299>
New connect syntax, also saves you the need to declare those as slots
applets/weather/plugin/plugin.cpp (line 33)
<https://git.reviewboard.kde.org/r/127193/#comment63300>
Use initializer list:
QVector<UnitItem> items{
UnitItem(i18n("foo"), Foo),
UnitItem(i18n("bar"), Bar),
...
}
Same below
applets/weather/weatherapplet.h (line 31)
<https://git.reviewboard.kde.org/r/127193/#comment63253>
I think we use subtext in tooltip context
applets/weather/weatherapplet.h (line 55)
<https://git.reviewboard.kde.org/r/127193/#comment63254>
You can do Qt.openUrlExternally("address") from QML
libs/plasmaweather/CMakeLists.txt (line 23)
<https://git.reviewboard.kde.org/r/127193/#comment63301>
At least for QML plugins we don't do any ABI stability guarantee as they're loaded as plugins. Not sure about native interface for plasmoids, though.
- Kai Uwe Broulik
On Feb. 27, 2016, 1:18 vorm., Friedrich W. H. Kossebau wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127193/
> -----------------------------------------------------------
>
> (Updated Feb. 27, 2016, 1:18 vorm.)
>
>
> Review request for Plasma and Marco Martin.
>
>
> Repository: kdeplasma-addons
>
>
> Description
> -------
>
> So the weather applet in branch kossebau/weatherapplet works okayish now here, both on panel and on containment:
> https://share.kde.org/index.php/s/MxwDYtmRw9TtQ8X
>
> This revival is done as minimal porting, after more considerations I for now kept libplasmaweather as it is where possible (also because I use classes from the lib in the QML plugin and in the applet, so some shared unit is needed).
> I propose to do further modernizing & refactoring of the applet code only once it works again completely and can be part of 5.6 release.
>
> Please help with a quick review. And also with a few issues that still need to be solved for a perfect solution. Where I need your, the experts', help, please read on.
>
> Layout:
> `configGeneral.qml` needs larger minimal sizes of the input fields. How can this be done? I am lost with the QQ2 layout approach for now.
>
> Size of applet in fullRepresentation mode when expanded from panel:
> while the expanded applet had some kind of sane size all the time I was playing (cmp. the screenshot), at one point the size of the expanded representation only turned into some unusable 16x16 or similar size. For existing applets and new ones.
>
>
> Q: Is this ported correctly (a hack I found elsewhere), or is there a better way meanwhile?
> ```
> q->setBusy(false);
> ```
> to
> ```
> QObject *graphicObject = q->property("_plasma_graphicObject").value<QObject *>();
> if (graphicObject) {
> graphicObject->setProperty("busy", false);
> }
> ```
>
> Q: `Plasma::Applet::showMessage()` can be ported to what?
>
> Q: `DataEngine::query()` should be ported to? Similar code is unported also in the weather/ions/ion_noaa dataengine in plasma-workspace.
>
> ```
> Plasma::DataEngine::Data data = timeEngine->query(
> QString(QLatin1String( "Local|Solar|Latitude=%1|Longitude=%2" )).arg(latitude).arg(longitude));
> bool day = data[QLatin1String( "Corrected Elevation" )].toDouble() > 0.0);
> ```
>
>
> Diffs
> -----
>
> CMakeLists.txt 5568251
> applets/CMakeLists.txt bd3ea48
> applets/weather/CMakeLists.txt 354b6ee
> applets/weather/package/contents/config/config.qml PRE-CREATION
> applets/weather/package/contents/ui/DetailsView.qml bbcdd15
> applets/weather/package/contents/ui/FiveDaysView.qml a39f334
> applets/weather/package/contents/ui/Notice.qml d38c108
> applets/weather/package/contents/ui/NoticesView.qml 16923c4
> applets/weather/package/contents/ui/TopPanel.qml af57f45
> applets/weather/package/contents/ui/WeatherListView.qml b29099f
> applets/weather/package/contents/ui/configGeneral.qml PRE-CREATION
> applets/weather/package/contents/ui/main.qml c501ab3
> applets/weather/package/metadata.desktop 79646c2
> applets/weather/plugin/abstractunitlistmodel.h PRE-CREATION
> applets/weather/plugin/abstractunitlistmodel.cpp PRE-CREATION
> applets/weather/plugin/locationlistmodel.h PRE-CREATION
> applets/weather/plugin/locationlistmodel.cpp PRE-CREATION
> applets/weather/plugin/plugin.h PRE-CREATION
> applets/weather/plugin/plugin.cpp PRE-CREATION
> applets/weather/plugin/qmldir PRE-CREATION
> applets/weather/weatherapplet.h c4b376b
> applets/weather/weatherapplet.cpp 60f882a
> libs/CMakeLists.txt bd71119
> libs/plasmaweather/CMakeLists.txt a9faa7b
> libs/plasmaweather/plasmaweather.knsrc 8525f20
> libs/plasmaweather/plasmaweather_export.h 691db23
> libs/plasmaweather/weatherconfig.h 9b3c2d7
> libs/plasmaweather/weatherconfig.cpp 1ec0b42
> libs/plasmaweather/weatherconfig.ui f285fff
> libs/plasmaweather/weatheri18ncatalog.h 0122378
> libs/plasmaweather/weatheri18ncatalog.cpp 1868352
> libs/plasmaweather/weatherlocation.h 004d788
> libs/plasmaweather/weatherlocation.cpp 1d275ea
> libs/plasmaweather/weatherpopupapplet.h ce95a5a
> libs/plasmaweather/weatherpopupapplet.cpp 4533619
> libs/plasmaweather/weathervalidator.h eb33558
> libs/plasmaweather/weathervalidator.cpp 4d016e2
>
> Diff: https://git.reviewboard.kde.org/r/127193/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Friedrich W. H. Kossebau
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20160227/048cb2af/attachment-0001.html>
More information about the Plasma-devel
mailing list