[prev in list] [next in list] [prev in thread] [next in thread] 

List:       kde-panel-devel
Subject:    Re: Review Request: wetter.com Weather Ion
From:       "Armin Berres" <armin () space-based ! de>
Date:       2009-10-31 18:48:30
Message-ID: 20091031184830.21980.49851 () localhost
[Download RAW message or body]


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


First: Awesome :)
Finally I can abandon the website.

I did not do a proper review, but I noticed the two things below.


/trunk/kdereview/plasma/dataengines/weather/ions/ion_wettercom.h
<http://reviewboard.kde.org/r/2026/#comment2282>

    Shouldn't this private section completely be moved into the private \
class or am I blind and miss something?



/trunk/kdereview/plasma/dataengines/weather/ions/ion_wettercom.h
<http://reviewboard.kde.org/r/2026/#comment2270>

    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.


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.

- Armin


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
> 
> 

_______________________________________________
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel


[prev in list] [next in list] [prev in thread] [next in thread]