Review Request: wetter.com Weather Ion

[Thilo-Alexander Ginkel]

> On 2009-11-03 17:00:51, Will Stephenson wrote:
> > /trunk/kdereview/plasma/dataengines/weather/ions/ion_wettercom.cpp, line 44
> > <http://reviewboard.kde.org/r/2026/diff/2/?file=13614#file13614line44>
> >
> >     By convention, public member variables of pimpl classes are not names with m_ prefixes - since accessing them via d-> in the outer class makes it obvious that they belong  to the pimpl
> 
>  wrote:
>     this isn't public API. why does it have a pimpl at all? i think some people have missed the point of the FooPrivate classes: maintaining binary compatibility in publicly exported APIs. for anything else they are just a bit more overhead, one more level of indirection and one more opportunity for a memory leak.

Sorry if the following reply reaches you twice (I mistakenly replied by mail before because the Review Board notification left me with the impression that the message did not go through RB, but was sent via mail). I'd still like the reply to be part of the RB discussion for the sake of completeness:

-- 8< --

I chose this approach as that's what the existing Weather Ions are using, but also agree that this is not a public API, so binary compatibility is a non-issue. If you prefer, I can easily remove the pimpl.


- Thilo-Alexander


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


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