Review Request 127193: Revival of Weather applet (for Plasma 5.6)
Friedrich W. H. Kossebau
kossebau at kde.org
Sun Feb 28 20:08:05 UTC 2016
> On Feb. 27, 2016, 10:27 a.m., Kai Uwe Broulik wrote:
> > 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 :)
Thanks a lot for your detailed review and testing, very appreciated!
> if I double click to add it it says "Error loading widget: Package does not exist", same when I add it to system tray.
Confirmed. Same issue here also with the comic applet. Perhaps something related to being cpp-backed applets? Shall I file a bug so it can be tracked? At least I assume nothing I can fix in the applet itself.
> 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.
Can reproduce, at least the busy indicator on the compact applet, still investigating how the busy flag is set to wrong value. But here the config itself works, the new weather station is used and data shown as expected. Just the busy indicator stays.
So still ISSUE.
> The popup size when placed in a panel and the default size on the desktop is way too small
Yes. Seems there are binding issues with the width & height values, I am still lost what the Plasma5 approach is and need to investigate more.
So still ISSUE.
> 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]"
The C++-code (for the base class) uses `Applet::setConfigurationRequired(false)`, is that no longer working or not enough?
> It sometimes seems to forget the weather station when I open settings and when I then save settings the applet is blank again
I have not seen that, any chance you can tell how to reproduce?
> Sometimes it doesn't find any weather stations in the search
Wetter.com might sometimes tack a few secs, though usually replies instantly. The service behind "noaa" data engine though seems to
Needs perhaps inspection and fixing in the weather dataengines (in plasma-workspace/dataengines/weather/. The code/logic in the weather applet I mainly moved around (from old WeatherConfig class to LocationListModel), still possible I messed something up.
> The tooltip subtext is blank (but not empty, ie. I get an additional empty line)
Fixed, catching the case where "%1 %2" is filled with empty strings due to no data.
> 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.
I think it is by design. The Plasma4 version looked the same. And I do not want to mess around with the design, that I happily leave to VDG once the applet itself is ported and works in a first version :)
> On Feb. 27, 2016, 10:27 a.m., Kai Uwe Broulik wrote:
> > applets/weather/package/contents/ui/DetailsView.qml, line 25
> > <https://git.reviewboard.kde.org/r/127193/diff/1/?file=445617#file445617line25>
> >
> > This seems unused
Investigated and found it is used: it is a property of WeatherListView, used with default value by FiveDaysView.qml and different set value by DetailsView.qml
> On Feb. 27, 2016, 10:27 a.m., Kai Uwe Broulik wrote:
> > applets/weather/package/contents/ui/DetailsView.qml, line 41
> > <https://git.reviewboard.kde.org/r/127193/diff/1/?file=445617#file445617line41>
> >
> > visible: !!elementId ?
If that shortcut is okay with Plasma JavaScript pattern policy, okay. Picked up.
> On Feb. 27, 2016, 10:27 a.m., Kai Uwe Broulik wrote:
> > applets/weather/package/contents/ui/TopPanel.qml, line 50
> > <https://git.reviewboard.kde.org/r/127193/diff/1/?file=445621#file445621line50>
> >
> > 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)
Picked up PlasmaExtras.Heading.
> On Feb. 27, 2016, 10:27 a.m., Kai Uwe Broulik wrote:
> > applets/weather/package/contents/ui/main.qml, line 46
> > <https://git.reviewboard.kde.org/r/127193/diff/1/?file=445624#file445624line46>
> >
> > units.smallSpacing
> >
> > The applet should have some inner padding automatically, however, I think
While plasmawindowed does not add padding, in Plasma itself indeed there is padding given.
Still, seems these margins here are used to have the aligned with the rounded ends of the bar behind the data rows. Removing them made things look less nice IMHO, so kept them, with a note.
> On Feb. 27, 2016, 10:27 a.m., Kai Uwe Broulik wrote:
> > applets/weather/package/metadata.desktop, line 56
> > <https://git.reviewboard.kde.org/r/127193/diff/1/?file=445625#file445625line56>
> >
> > That's not you
Yes, I only ported the code to Plasma5, original design and architecture is no-where my credit (cmp. also "minimal porting" plan ;) ).
> On Feb. 27, 2016, 10:27 a.m., Kai Uwe Broulik wrote:
> > applets/weather/package/metadata.desktop, line 61
> > <https://git.reviewboard.kde.org/r/127193/diff/1/?file=445625#file445625line61>
> >
> > Do we have this category?
https://techbase.kde.org/Projects/Plasma/PIG lists it at least. And referenced from https://techbase.kde.org/Development/Tutorials/Plasma5/QML2/GettingStarted
> On Feb. 27, 2016, 10:27 a.m., Kai Uwe Broulik wrote:
> > applets/weather/package/contents/ui/FiveDaysView.qml, line 62
> > <https://git.reviewboard.kde.org/r/127193/diff/1/?file=445618#file445618line62>
> >
> > Again, PlasmaComponents.Label
Done. But this shows a strange bug on initial layout, the first item in the row is lower (disappears if e.g. the size of the applet is changed). Marked as TODO in the code for now.
> On Feb. 27, 2016, 10:27 a.m., Kai Uwe Broulik wrote:
> > applets/weather/weatherapplet.h, line 36
> > <https://git.reviewboard.kde.org/r/127193/diff/1/?file=445633#file445633line36>
> >
> > I think we use subtext in tooltip context
Picked up and aligned the name of the properties with the plasma lingo :)
> On Feb. 27, 2016, 10:27 a.m., Kai Uwe Broulik wrote:
> > applets/weather/plugin/locationlistmodel.cpp, line 31
> > <https://git.reviewboard.kde.org/r/127193/diff/1/?file=445629#file445629line31>
> >
> > nullptr
> >
> > Also, you could initialize that in the header:
> > m_dataengine = nullptr
Stayed with init in constructor impl for now (to have init in one place).
> On Feb. 27, 2016, 10:27 a.m., Kai Uwe Broulik wrote:
> > applets/weather/package/contents/ui/configGeneral.qml, line 48
> > <https://git.reviewboard.kde.org/r/127193/diff/1/?file=445623#file445623line48>
> >
> > 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?
Agree that the approach is rather "interesting" and should be revisited by UI experts. As said, for now concentrated on porting the existing applet and would like to postpone this to after the applet is ported as it is. Okay to drop this for this review?
> On Feb. 27, 2016, 10:27 a.m., Kai Uwe Broulik wrote:
> > applets/weather/package/contents/ui/main.qml, lines 27-31
> > <https://git.reviewboard.kde.org/r/127193/diff/1/?file=445624#file445624line27>
> >
> > Base those sizes on units.gridUnit * n
Any rule of thumbs I could use mapping the old pixel values to their `n` counterparts?
Also wondering why one has to explicitely set min size, can't that be calculated from the items and layout?
- Friedrich W. H.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127193/#review92824
-----------------------------------------------------------
On Feb. 27, 2016, 1:18 a.m., 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 a.m.)
>
>
> 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/20160228/067cc2d3/attachment-0001.html>
More information about the Plasma-devel
mailing list