'Re: Review Request: wetter.com Weather Ion'

[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:       "Shawn Starr" <shawn.starr () rogers ! com>
Date:       2009-11-04 18:51:19
Message-ID: 20091104185119.16780.92302 () localhost
[Download RAW message or body]

> 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. 
> Will Stephenson wrote:
> (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? 
> Thilo-Alexander Ginkel wrote:
> I asked wetter.com for a way to retrieve the current weather conditions a couple of \
> weeks ago and got the reply that these are only available through an API for paying \
> subscribers. Now, one could fall back to screen scraping, but that is no option as \
> it violates their TOS. 
> The panel use-case - admittedly - is kind of tricky. What about an icon that mixes \
> multiple weather conditions (so it is obvious that this isn't the current weather)?

I have removed all the dptrs from the ions in kdebase. I will do that in playground, \
but those other ions need more work elsewhere.

- Shawn

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

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