----------------------------------------------------------- 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 > >
_______________________________________________ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
