KDE review for KWeatherCore

[Volker Krause]

Having implemented the weather support for Itinerary, rebasing that onto a 
more comprehensive framework would indeed be welcome :)

I haven't looked too deeply at the implementation or the API yet, most of the 
feedback below is based on things learned when implementing this for 
Itinerary.

## api.met.no license and ToS compliance

The data is coming from the Norwegian Meteorological Institute, CC-licensed 
and with an API that doesn't require authentication, well documented and with 
a mailinglist for service changes and roadmap/disruption announcements. As far 
as implementing Open Data by organizations/public administration goes this is 
exemplary and unfortunately quite rare.

So at the very least we should follow their license and terms of services as 
close as possible, in letter and in spirit. Not all of that can be ensured by 
the library, some of this needs to be handled by the application. In the 
latter case those requirements need to be clearly documented though.

In particular I remember implementing the following aspects:

(1) add a point of contact to the User-Agent header, in case your client 
misbehaves. I only see this done in one place, https://invent.kde.org/
libraries/kweathercore/-/blob/master/src/weatherforecastsource.cpp#L52, which 
looks suspiciously like a verbatim copy from Itinerary, without even adjusting 
the contact address.

(2) Request throttling. The ToS seem to have been changed in wording since I 
last looked at this, but the requirement is still similar: Ensure we don't run 
unnecessary many queries. The Itinerary implementation uses a random interval 
between 120-150 minutes as lower bound, based on previous suggestions in the 
ToS.

The already suggested daemon is one way to ensure this globally, however I'm 
very reluctant to suggest a daemon for this, considering the deployment issues 
this causes for e.g. Flatpak or APKs. Itinerary uses a simple file cache and 
file mtimes, something like this should also hold up in a multi-process setup 
with a bit of extra care I think.

(3) Attribution, as required by the CC-BY license. This has to happen in the 
UI, but as a library user I need to know about this requirement, so this needs 
prominent mention in the docs I'd say.

See https://api.met.no/doc/TermsOfService for more details.


I also see other services used there I'm not familiar with, are we complying 
with their licenses and terms of services?


## Privacy considerations

(1) Requested geo coordinates are passed to the online API as-is, ie. this is 
potentially leaking a high-resolution position of the user to the outside. We 
can't avoid this entirely obviously, but we can reduce the impact by reducing 
the coordinate resolution. 

Itinerary uses 1/10th of a degree for this, which unless you get close to the 
poles are areas of a few kilometers in size, ie. rather than leaking your home 
address it's only leaking your home town.

(2) What does the sunrise API offer beyond the existing sunrise API we have in 
KF5::Holidays? The latter has the advantage of doing the entire calculation 
locally.

See https://api.kde.org/frameworks/kholidays/html/
namespaceKHolidays_1_1SunRiseSet.html

(3) Similarly, we do have a full offline implementation for timezone lookup by 
geo coordinates here: https://api.kde.org/kdepim/kitinerary/html/
namespaceKItinerary_1_1KnowledgeDb.html#ac409f468badee3c05438695009d7c67f

(4) There are http: only requests send to api.geonames.org it seems.


Similarly as with the compliance point, some of this can be argued to be the 
job of the using application. It would seem safer though is the library would 
try  to avoid mistakes to begin with.


## Other observations

(1) The license seems to be GPL-2.0-or-later, which is incompatible with the 
Frameworks license policy. See https://community.kde.org/Policies/
Licensing_Policy

(2) The result types (DailyForecast, HourlyForecast, etc) might benefit from 
being Q_GADGETs and having Q_PROPERTYs added, so they can be directly consumed 
by QML?

(3) binary compatibility measures seem to be missing in a number of places: 
https://community.kde.org/Policies/Binary_Compatibility_Issues_With_C%2B%2B


Regards,
Volker

On Montag, 21. Dezember 2020 07:16:09 CET hanyoung wrote:
> KWeatherCore: https://invent.kde.org/libraries/kweathercore is a library for
> querying weather forecast data. During the development of KWeather, we
> found the need to have a weather library. KWeatherCore is the result of
> extracting weather data fetching code from KWeather. I think having a
> dedicated weather library can serve the following propose: - simplify the
> KWeather code
> - easier to develop a weather daemon
> - potentially less code duplication across KDE
> 
> Many of you may have already seen my previous email to kde-devel mailing
> list.
> Thank you for your constructive suggestions. Here are something I want to 
clarify:
> > I would also propose to consider doing a demon instead, so different
> > programs/processes all interested in weather forecast data could share the
> > data
> 
>   The end goal is a daemon indeed, but we want to build the daemon upon the
> library. This would give us flexibility in the future if we don't want a
> daemon. At least KWeather and other projects can still benefit from the
> code.
> > but we want to make sure we don't end up with two things.
> 
>   I admit there are some overlapped functionalities. But KWeatherCore isn't
> designed as a weather data provider as Weather DataEngine. Instead, it's
> trying to be the building block of weather related applications.
> KWeatherCore saves you the hassle of dealing with APIs, getting locations
> and converting timezone. You can build a daemon with it, or you can use it
> in your applications. For example, KItinerary and KWeather use the same
> weather API, but don't share code. That means two code base to maintain.
> Regarding the dynamic nature of online APIs, it's better to have one
> library, so other applications don't need to be worried about their APIs
> being depraved, and they aren't able to update it in time.
> 
>   Though not currently implemented, KWeatherCore does intend to have weather
> alerts added. We hope it can be done in this Sok
> https://community.kde.org/SoK/Ideas/2021#KWeather
>   With this bit added, then the work on weather daemon can be started.
> 
> Regards,
> Han Young