Bug 330773 - "bbc.co.uk" (bbcukmet) ion does not display weather data
Summary: "bbc.co.uk" (bbcukmet) ion does not display weather data
Status: RESOLVED FIXED
Alias: None
Product: plasma4
Classification: Plasma
Component: widget-weather (show other bugs)
Version: 4.12.1
Platform: openSUSE Linux
: NOR normal
Target Milestone: ---
Assignee: Plasma Bugs List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-02-04 22:02 UTC by Christoph Feck
Modified: 2016-04-04 14:13 UTC (History)
4 users (show)

See Also:
Latest Commit:
Version Fixed In:


Attachments
Update to BBC's new json-based search and modified xml (9.45 KB, patch)
2014-03-15 13:33 UTC, Raphael Geissert
Details
Handle cases where min. or max. temperatures are not reported (2.39 KB, patch)
2014-03-15 13:37 UTC, Raphael Geissert
Details
Handle cases where min. or max. temperatures are not reported (4.11.7 backport) (2.07 KB, patch)
2014-03-16 00:06 UTC, Kevin Kofler
Details
Trivial fix for the "clear sky" typo (910 bytes, patch)
2014-03-24 01:31 UTC, Kevin Kofler
Details
Fix for crash bug #332392 and error handling improvements (7.49 KB, patch)
2014-03-25 00:31 UTC, Kevin Kofler
Details
Fix for crash bug #332392 and error handling improvements (v2) (7.50 KB, patch)
2014-03-25 17:58 UTC, Kevin Kofler
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Christoph Feck 2014-02-04 22:02:14 UTC
Since a few days, weather information in Plasma did not update. Selecting on "Running the associated application" in the context menu opens a web page with correct weather information, so I guess the parser needs to be updated to parse that page correctly again.
Comment 1 Kevin Kofler 2014-02-11 01:27:55 UTC
Same here, on both desktop and notebook. Also, a search does not return any entries from bbcukmet anymore (repeatable from several locations, so it's not a network issue on my end).
Comment 2 Shawn Starr 2014-02-11 19:54:14 UTC
hmm, this does look broken, I hope BBC just changed URL vs parsing...
Comment 3 Kevin Kofler 2014-02-28 17:17:55 UTC
They have done some website redesigning, as mentioned in passing in http://www.bbc.co.uk/blogs/internet/posts/BBC-Weather-app-hits-5m-mark ("We are also using the lessons learnt on apps to start building a new responsive website for Weather").

It looks like they broke our bbcukmet ion in the process of the redesign. :-(

This is the current documentation for the RSS feeds:
http://www.bbc.com/weather/about/17543675
The search URLs changed, too, I cannot find any documentation for those. (Have they ever been documented?)
Comment 4 Kevin Kofler 2014-02-28 17:19:39 UTC
BTW, the credit of the ion claims "Supported by backstage.bbc.co.uk", but backstage.bbc.co.uk was discontinued in 2010!
http://www.bbc.co.uk/blogs/bbcbackstage/2010/10/bbc-backstage-to-close.shtml
Comment 5 Raphael Geissert 2014-03-15 13:33:22 UTC
Created attachment 85589 [details]
Update to BBC's new json-based search and modified xml
Comment 6 Raphael Geissert 2014-03-15 13:37:52 UTC
Created attachment 85590 [details]
Handle cases where min. or max. temperatures are not reported

With those two patches the ion should be working again. I've mainly tested the weather observations part, and only tested briefly the forecast.

Both patches sent to the Shawn a few weeks ago, but adding them here to prevent them from getting lost.
Comment 7 Kevin Kofler 2014-03-15 22:50:00 UTC
Next time please submit the patches to Bugzilla, or (better) to ReviewBoard with a link in Bugzilla, rather than to one individual developer right away, so that everyone interested can find them.
Comment 8 Kevin Kofler 2014-03-15 22:52:58 UTC
(In particular, distro packagers who want to ship a package that actually works in their distribution.)
Comment 9 Shawn Starr 2014-03-15 23:14:53 UTC
Thanks for the patch, I'm going to review it tomorrow (Sunday) please also submit to review board, but if it works I'll commit it.
Comment 10 Kevin Kofler 2014-03-16 00:06:21 UTC
Created attachment 85596 [details]
Handle cases where min. or max. temperatures are not reported (4.11.7 backport)

Here's a rebased version of the patch from comment #6 (the temperature one) that applies against kde-workspace 4.11.7. (Note: This patch does NOT have the git metadata attached. And please credit Raphael Geissert for the patch, not me, I only did a trivial rebase.)

(The patch from comment #5 applies against 4.11.7 as is.)
Comment 11 Kevin Kofler 2014-03-16 15:56:11 UTC
There is now a condition "Clear Sky" that is not mapped to any icon, so I get the "not available" one.
Comment 12 Kevin Kofler 2014-03-16 15:59:53 UTC
Oh, this is a typo that has always been there:
    dayList["clar sky"] = ClearDay;
should be "clear", not "clar"!
Comment 13 Kevin Kofler 2014-03-18 14:54:35 UTC
A Fedora user reported the following crash to us:
http://pastebin.com/rBSnRxzf

ion_bbcukmet.cpp:561 corresponds to this line in the source code:
                m_dateFormat = QDateTime::fromString(data.obsTime.split("-")[1].trimmed(), "hh:mm 'GMT'");

It looks like data.obsTime doesn't always contain '-'.
Comment 14 Kevin Kofler 2014-03-18 15:00:21 UTC
The user has the location configured to Szczecin, Poland (station ID: 3083829). Looking at the feed, I see only forecasts and no observation at all, which is why the observation time cannot be processed. The code needs to handle this robustly.
Comment 15 Christoph Feck 2014-03-21 10:38:40 UTC
See also bug 332392.
Comment 16 Kevin Kofler 2014-03-24 01:31:29 UTC
Created attachment 85705 [details]
Trivial fix for the "clear sky" typo

Here's the trivial 1-character fix for the "clear sky" typo.

I'm working on fixing the crash (bug #332392) now (since I'm apparently the only one who cares, grrr…).
Comment 17 Kevin Kofler 2014-03-24 01:55:55 UTC
Looks like the problem was not that the current conditions are not available at all, but that the observation time was missing or not in the expected format. Unfortunately, I cannot reproduce the crash right now.
Comment 18 Kevin Kofler 2014-03-24 02:32:08 UTC
In addition to the crash, the parsing code also has other issues:

>                 data.obsTime = conditionString.midRef(0, splitIndex).toString();

pointless use of midRef, use mid or, better, left instead

>                 // Saturday - 13:00 CET
>                 // Saturday - 12:00 GMT
>                 m_dateFormat = QDateTime::fromString(data.obsTime.split("-")[1].trimmed(), "hh:mm 'GMT'");

This hardcodes "GMT" in the format string, which does not match the local time zones actually used, such as "CET". Either you should just remove the time zone altogether (quick hack which does the right thing as long as you're considering the weather for your current location), or you should use a parser that supports time zones, such as:
m_dateFormat = KDateTime::fromString(data.obsTime.split("-")[1].trimmed(), "%H:%M %Z").toLocalZone().dateTime();
Comment 19 Kevin Kofler 2014-03-25 00:31:30 UTC
Created attachment 85721 [details]
Fix for crash bug #332392 and error handling improvements

This patch improves several instances of string manipulation and error handling:
* crash kde#332392 should be gone, we now verify that the string contains '-' before splitting on '-', I also simplified the string handling a bit
* parsing the time correctly parses the timezone instead of hardcoding "GMT"
* unavailable weather conditions are now reported as "N/A" (or the translated version thereof), not "null"
* unavailable temperatures are now correctly treated as missing and no longer reported as "0°C"

The patch compiles and runs fine for me, but since I cannot reproduce the crash, I cannot absolutely verify that it is gone.
Comment 20 Kevin Kofler 2014-03-25 17:58:26 UTC
Created attachment 85741 [details]
Fix for crash bug #332392 and error handling improvements (v2)

I've just noticed I had accidentally messed up the reporting of the current weather conditions so that it always reported "N/A" even when the condition is being reported just fine in the RSS.

This fixed version of my latest patch fixes that. (It still requires the other 3 patches to be applied first.)
Comment 21 Friedrich W. H. Kossebau 2016-04-04 14:13:57 UTC
Git commit 84fe5785bd1520f17a801cfe2e263c8ba872b273 by Friedrich W. H. Kossebau, on behalf of Raphael Geissert.
Committed on 02/04/2016 at 23:47.
Pushed by kossebau into branch 'Plasma/5.6'.

[weather bbcukmet] Update to BBC's new json-based search and modified xml

M  +51   -39   dataengines/weather/ions/bbcukmet/ion_bbcukmet.cpp

http://commits.kde.org/plasma-workspace/84fe5785bd1520f17a801cfe2e263c8ba872b273