KOSMIndoorMap in kdereview

[Volker Krause]

On Freitag, 23. Oktober 2020 00:45:46 CEST Albert Astals Cid wrote:
> El dijous, 22 d’octubre de 2020, a les 17:25:32 CEST, Volker Krause va 
escriure:
> > Hi,
> > 
> > KOSMIndoorMap is a QML component for showing multi-floor OSM indoor maps
> > (as its very creative name might suggest). It's using maps.kde.org as a
> > data source (same as Marble), and has been created to show interactive
> > maps of train stations for KDE Itinerary.
> > 
> > https://invent.kde.org/libraries/kosmindoormap
> > 
> > KOSMIndoorMap has been growing inside the KPublicTransport repo (due to
> > some building blocks being available there alreay and me being lazy), and
> > has now been split into its own repo. So technically this is coming out
> > of a reviewed repo already rather than from playground, but better be on
> > the safe side :)
> > 
> > The aim is having this (together with KPublicTransport and KDE Itinerary)
> > join the release service for 20.12.
> 
> Is this fixable?
> 
> [libprotobuf WARNING google/protobuf/compiler/parser.cc:648] No syntax
> specified for the proto file: osmformat.proto. Please use 'syntax =
> "proto2";' or 'syntax = "proto3";' to specify a syntax version. (Defaulted
> to proto2 syntax.) [libprotobuf WARNING
> google/protobuf/compiler/parser.cc:648] No syntax specified for the proto
> file: fileformat.proto. Please use 'syntax = "proto2";' or 'syntax =
> "proto3";' to specify a syntax version. (Defaulted to proto2 syntax.)

fixed, although that requires modifying the 3rdparty proto files

> and this?
> 
> style/mapcsslexer.l:65: warning, no es pot satisfer la regla
> style/mapcssparser.y:143.11-14: advertiment: valor sense usar: $2 [-Wother]
>   143 | | Ruleset Rule
> 
>       |           ^~~~
> 
> and this?
> 
> src/map/scene/scenegeometry.cpp: In function ‘double
> KOSMIndoorMap::SceneGeometry::polylineMidPointAngle(const QPolygonF&)’:
> src/map/scene/scenegeometry.cpp:75:24: warning: unused variable ‘r’
> [-Wunused-variable] 75 |             const auto r = ((length + l) -
> lineLength / 2.0) / l;

both fixed

> Should ./bin/indoormap do anything? I have one empty combo and one combo
> that lets me change the style.

Good point, I've documented the test apps in the README now. Try `indoormap -c 
52.52512,13.36966` or alternatively `qmlscene tests/indoormap.qml` to see a 
few more features (after updating to latest master to make this work with your 
locale too, see below).

> You have one qsTr in the qml that it's not going to work, also a "TODO
> i18n?" that i would say yes?

Fixed, leftovers from before switching to ki18n.

> It has tests \o/ the tests don't pass /o\ https://pastebin.com/CFcdVJHh

Ouch, you actually found some quite serious breakage that I had entirely 
missed since neither my local nor the CI locale is affected... This all goes 
down to some string to floating point number conversion being done locale-
aware while they shouldn't be.

Dirty fixes applied, once std::from_chars becomes available for floating point 
types this can be addressed more cleanly.

We should run the CI with rotating/random locales :) 

Thanks for the feedback!
Volker