Review Request: wetter.com Weather Ion

Sun Nov 1 12:22:06 CET 2009

> On 2009-10-31 18:48:39, Armin Berres wrote:
> > /trunk/kdereview/plasma/dataengines/weather/ions/ion_wettercom.h, line 146
> > <http://reviewboard.kde.org/r/2026/diff/1/?file=13579#file13579line146>
> >
> >     Nitpicking: What about fetchForecast instead of getForecast? When I looked at the header a getter which returned void was looking a bit weird to be.

Definitely makes sense. Will be part of the next updated version.

> On 2009-10-31 18:48:39, Armin Berres wrote:
> > /trunk/kdereview/plasma/dataengines/weather/ions/ion_wettercom.h, line 121
> > <http://reviewboard.kde.org/r/2026/diff/1/?file=13579#file13579line121>
> >
> >     Shouldn't this private section completely be moved into the private class or am I blind and miss something?

I'll look into this. Basically I borrowed the class layout from the BBC Weather Ion, so I did not put too much thought into this.

On 2009-10-31 18:48:39, Thilo-Alexander Ginkel wrote:
> > Oh, and personally I would use some kind of shared pointer for the WeatherData::Forecast[Period|INFO]* objects. But this is just something personal. I try to avoid memory I have to delete explicitly these days.
> 
> Will Stephenson wrote:
>     When I search for 'Nürnberg' with umlaut, I get a list of hits that does not include Nürnberg.  Then i close the hits, close the location dialog, reopen the location dialog, and it is set to 'Nürnberg, Bayern, DE'.  Expected behaviour - find the city in the hit list

Agreed. I can reproduce this umlaut issue, but am almost certain that searching for city names containing umlauts (and other special characters) used to work correctly in the past. The URLs being generated are correctly URL-encoded for UTF-8. I'll get in touch with the service operator to figure out whether this is a server-side bug.

- Thilo-Alexander

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

On 2009-10-31 14:02:05, Thilo-Alexander Ginkel wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/2026/
> -----------------------------------------------------------
> 
> (Updated 2009-10-31 14:02:05)
> 
> 
> 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
> 
>