Review Request: wetter.com Weather Ion

[Will Stephenson]

> On 2009-11-03 17:31:01, Will Stephenson wrote:
> > You could work out the "Current Conditions" and "Condition Icon" data on each update using the current system time and the UTC time in the forecast periods, and set these on the data source.  This would AFAICS get rid of the 'missing icon' icon and make the applet usable in panels.
> 
> Thilo-Alexander Ginkel wrote:
>     I thought about that before, but found it rather weird to call an interpolated value from the forecast a "current" weather condition. Of course, it would be much more pleasing to the eye to have an icon to display. An alternative would be to fix the weather applet to display no icon instead of a broken one if no data is supplied for the current condition.
> 
> Shawn Starr wrote:
>     I used dptrs because I didn't want to break the internal API, which is public in a way. As more people use the backend changing things get more risky. the dptrs are for the member data vs functions. But, if we don't need the dptrs in the ions, we can get 'em out.
> 
> Aaron Seigo wrote:
>     in classes which are linked to by the ion plugins (e.g. Ion), it makes sense. for the ion plugins themselves it doesn't: those symbols aren't exported or used by anything that needs binary compatibility in their interfaces.

(Umm, threading, folks?)

Re Current Conditions - I see your point.  ATM on wetter.com the current weather is 'cloudy' here but both their morning and midday values are 'rainy' so an interpolation would be off.  Is it worth asking wetter.com to expose this too, since they obviously have it on the site.  If other providers do give this data directly, perhaps they will feel it's ok to.

However what icon should the applet use when it's in a panel with no Current Conditions data?  


- Will


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/2026/#review2904
-----------------------------------------------------------


On 2009-11-03 19:51:12, Thilo-Alexander Ginkel wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/2026/
> -----------------------------------------------------------
> 
> (Updated 2009-11-03 19:51:12)
> 
> 
> Review request for kdelibs, Plasma and Shawn Starr.
> 
> 
> Summary
> -------
> 
> This change brings support for wetter.com (a weather forecast web site especially popular in Germany) as a weather data source to KDE.
> The Ion makes use of wetter.com's officially-supported REST API. The API supports forecasts for up to three days, so the Ion does not supply any information about the current weather conditions. The forecast for the current day is split between night and day whereas the following two days are provided as an aggregated value for the whole day, respectively. 
> 
> Any feedback is much appreciated!
> 
> 
> This addresses bug 204219.
>     https://bugs.kde.org/show_bug.cgi?id=204219
> 
> 
> Diffs
> -----
> 
>   /trunk/kdereview/plasma/dataengines/CMakeLists.txt 1043003 
>   /trunk/kdereview/plasma/dataengines/weather/CMakeLists.txt PRE-CREATION 
>   /trunk/kdereview/plasma/dataengines/weather/ions/CMakeLists.txt PRE-CREATION 
>   /trunk/kdereview/plasma/dataengines/weather/ions/ion-wettercom.desktop PRE-CREATION 
>   /trunk/kdereview/plasma/dataengines/weather/ions/ion_wettercom.h PRE-CREATION 
>   /trunk/kdereview/plasma/dataengines/weather/ions/ion_wettercom.cpp PRE-CREATION 
> 
> Diff: http://reviewboard.kde.org/r/2026/diff
> 
> 
> Testing
> -------
> 
> Tested the Ion under a current KDE installation from trunk, fixed all Krazy warnings before the move to kdereview. Profiled it for memory leaks using Valgrind, which did not bring up any leaks in the control of the Ion. Those two still look somewhat suspicious, so I'd like to get your opinion about them:
> 
> 42 bytes in 1 blocks are definitely lost in loss record 372 of 1,156
>    at 0x4C25153: malloc (vg_replace_malloc.c:195)
>    by 0x60EDCB4: QString::QString(int, Qt::Initialization) (qstring.cpp:1027)
>    by 0x61BD5EE: QUtf8::convertToUnicode(char const*, int, QTextCodec::ConverterState*) (qutfcodec.cpp:169)
>    by 0x60EFDFA: QString::fromUtf8(char const*, int) (qstring.cpp:3850)
>    by 0x614C052: fromPercentEncodingMutable(QByteArray*) (qurl.cpp:223)
>    by 0x614EFB3: QUrlPrivate::parse(QUrlPrivate::ParseOptions) const (qurl.cpp:3781)
>    by 0x61534FE: QUrl::isValid() const (qurl.cpp:4124)
>    by 0x5A23B5A: KUrl::_setEncodedUrl(QByteArray const&) (kurl.cpp:1538)
>    by 0x5A23F19: KUrl::KUrl(QString const&) (kurl.cpp:411)
>    by 0x1FAE37D6: WetterComIon::getForecast(QString const&) (ion_wettercom.cpp:504)
>    by 0x1FAE5BAA: WetterComIon::updateIonSource(QString const&) (ion_wettercom.cpp:320)
>    by 0x1F267CBE: IonInterface::sourceRequestEvent(QString const&) (ion.cpp:47)
> 
> 176 (32 direct, 144 indirect) bytes in 1 blocks are definitely lost in loss record 882 of 1,156
>    at 0x4C2596C: operator new(unsigned long) (vg_replace_malloc.c:220)
>    by 0x1F267795: IonInterface::IonInterface(QObject*, QList<QVariant> const&) (ion.cpp:36)
>    by 0x1FAE151D: WetterComIon::WetterComIon(QObject*, QList<QVariant> const&) (ion_wettercom.cpp:68)
>    by 0x1FAEE366: QObject* KPluginFactory::createInstance<WetterComIon, QObject>(QWidget*, QObject*, QList<QVariant> const&) (kpluginfactory.h:461)
>    by 0x5B3367B: KPluginFactory::create(char const*, QWidget*, QObject*, QList<QVariant> const&, QString const&) (kpluginfactory.cpp:191)
>    by 0x5594EB5: Plasma::DataEngineManager::loadEngine(QString const&) (kpluginfactory.h:515)
>    by 0x1F6C813F: WeatherEngine::loadIon(QString const&) (weatherengine.cpp:92)
>    by 0x1F6C8C7C: WeatherEngine::sourceRequestEvent(QString const&) (weatherengine.cpp:226)
>    by 0x55913AC: Plasma::DataEnginePrivate::requestSource(QString const&, bool*) (dataengine.cpp:670)
>    by 0x5591421: Plasma::DataEngine::connectSource(QString const&, QObject*, unsigned int, Plasma::IntervalAlignment) const (dataengine.cpp:89)
>    by 0x1F05608D: WeatherPopupApplet::connectToEngine() (weatherpopupapplet.cpp:234)
>    by 0x1F05815E: WeatherPopupApplet::init() (weatherpopupapplet.cpp:223)
> 
> 
> Thanks,
> 
> Thilo-Alexander
> 
>