From 0fda1dcf7df7e7a18fcfc2e7f2a2f1e83a3efd95 Mon Sep 17 00:00:00 2001 From: Adriaan de Groot Date: Sat, 8 Aug 2020 13:26:39 +0200 Subject: [PATCH 1/8] [libcalamares] Refactor finding-TZ algorithm - introduce a distance function and use that, rather than coding it inside the find() function. This is prep-work for unifying the find() calls, based on various coordinate systems. --- src/libcalamares/locale/TimeZone.cpp | 36 ++++++++++++++++++---------- 1 file changed, 23 insertions(+), 13 deletions(-) diff --git a/src/libcalamares/locale/TimeZone.cpp b/src/libcalamares/locale/TimeZone.cpp index e2779281f..6eabef6e8 100644 --- a/src/libcalamares/locale/TimeZone.cpp +++ b/src/libcalamares/locale/TimeZone.cpp @@ -336,6 +336,24 @@ ZonesModel::find( const QString& region, const QString& zone ) const return nullptr; } +STATICTEST const TimeZoneData* +find( const ZoneVector& zones, std::function< double( const TimeZoneData* ) > distance ) +{ + double smallestDistance = 1000000.0; + const TimeZoneData* closest = nullptr; + + for ( const auto* zone : zones ) + { + double thisDistance = distance( zone ); + if ( thisDistance < smallestDistance ) + { + closest = zone; + smallestDistance = thisDistance; + } + } + return closest; +} + const TimeZoneData* ZonesModel::find( double latitude, double longitude ) const { @@ -344,12 +362,7 @@ ZonesModel::find( double latitude, double longitude ) const * either N/S or E/W equal to any other; this obviously * falls apart at the poles. */ - - double largestDifference = 720.0; - const TimeZoneData* closest = nullptr; - - for ( const auto* zone : m_private->m_zones ) - { + auto distance = [&]( const TimeZoneData* zone ) -> double { // Latitude doesn't wrap around: there is nothing north of 90 double latitudeDifference = abs( zone->latitude() - latitude ); @@ -368,13 +381,10 @@ ZonesModel::find( double latitude, double longitude ) const longitudeDifference = abs( westerly - easterly ); } - if ( latitudeDifference + longitudeDifference < largestDifference ) - { - largestDifference = latitudeDifference + longitudeDifference; - closest = zone; - } - } - return closest; + return latitudeDifference + longitudeDifference; + }; + + return CalamaresUtils::Locale::find( m_private->m_zones, distance ); } QObject* From 2f871acbfd3c8def44d8e54887d6c5357259dc1e Mon Sep 17 00:00:00 2001 From: Adriaan de Groot Date: Sat, 8 Aug 2020 13:45:32 +0200 Subject: [PATCH 2/8] [libcalamares] Expose distanceFunc-find for timezones --- src/libcalamares/locale/TimeZone.cpp | 12 +++++++++--- src/libcalamares/locale/TimeZone.h | 16 +++++++++++++++- 2 files changed, 24 insertions(+), 4 deletions(-) diff --git a/src/libcalamares/locale/TimeZone.cpp b/src/libcalamares/locale/TimeZone.cpp index 6eabef6e8..4c641cd17 100644 --- a/src/libcalamares/locale/TimeZone.cpp +++ b/src/libcalamares/locale/TimeZone.cpp @@ -337,14 +337,14 @@ ZonesModel::find( const QString& region, const QString& zone ) const } STATICTEST const TimeZoneData* -find( const ZoneVector& zones, std::function< double( const TimeZoneData* ) > distance ) +find( const ZoneVector& zones, const std::function< double( const TimeZoneData* ) >& distanceFunc ) { double smallestDistance = 1000000.0; const TimeZoneData* closest = nullptr; for ( const auto* zone : zones ) { - double thisDistance = distance( zone ); + double thisDistance = distanceFunc( zone ); if ( thisDistance < smallestDistance ) { closest = zone; @@ -354,6 +354,12 @@ find( const ZoneVector& zones, std::function< double( const TimeZoneData* ) > di return closest; } +const TimeZoneData* +ZonesModel::find( const std::function< double( const TimeZoneData* ) >& distanceFunc ) const +{ + return CalamaresUtils::Locale::find( m_private->m_zones, distanceFunc ); +} + const TimeZoneData* ZonesModel::find( double latitude, double longitude ) const { @@ -384,7 +390,7 @@ ZonesModel::find( double latitude, double longitude ) const return latitudeDifference + longitudeDifference; }; - return CalamaresUtils::Locale::find( m_private->m_zones, distance ); + return find( distance ); } QObject* diff --git a/src/libcalamares/locale/TimeZone.h b/src/libcalamares/locale/TimeZone.h index 1d4b4a8ff..cc0c35ea2 100644 --- a/src/libcalamares/locale/TimeZone.h +++ b/src/libcalamares/locale/TimeZone.h @@ -167,6 +167,17 @@ public: Iterator begin() const { return Iterator( m_private ); } + /** @brief Look up TZ data based on an arbitrary distance function + * + * This is a generic method that can define distance in whatever + * coordinate system is wanted; returns the zone with the smallest + * distance. The @p distanceFunc must return "the distance" for + * each zone. It would be polite to return something non-negative. + * + * Note: not a slot, because the parameter isn't moc-able. + */ + const TimeZoneData* find( const std::function< double( const TimeZoneData* ) >& distanceFunc ) const; + public Q_SLOTS: /** @brief Look up TZ data based on its name. * @@ -176,7 +187,10 @@ public Q_SLOTS: /** @brief Look up TZ data based on the location. * - * Returns the nearest zone to the given lat and lon. + * Returns the nearest zone to the given lat and lon. This is a + * convenience function for calling find(), below, with a standard + * distance function based on the distance between the given + * location (lat and lon) and each zone's given location. */ const TimeZoneData* find( double latitude, double longitude ) const; From 0948963d86c6fdd7c3272da88dd10884cf05e525 Mon Sep 17 00:00:00 2001 From: Adriaan de Groot Date: Sat, 8 Aug 2020 22:01:10 +1000 Subject: [PATCH 3/8] [locale] Port TZ widget lookup to new find() method - The TZ widget uses a different coordinate system (mapping lat and lon to pixel locations, and then calculating Manhattan distance from that), so needs a different distance function. - Simplify code: there's just one "closest TZ" function. --- .../locale/timezonewidget/timezonewidget.cpp | 27 +++++-------------- 1 file changed, 7 insertions(+), 20 deletions(-) diff --git a/src/modules/locale/timezonewidget/timezonewidget.cpp b/src/modules/locale/timezonewidget/timezonewidget.cpp index b1d3cfeaa..778f0535d 100644 --- a/src/modules/locale/timezonewidget/timezonewidget.cpp +++ b/src/modules/locale/timezonewidget/timezonewidget.cpp @@ -190,28 +190,15 @@ TimeZoneWidget::mousePressEvent( QMouseEvent* event ) { return; } - // Set nearest location - int nX = 999999, mX = event->pos().x(); - int nY = 999999, mY = event->pos().y(); - using namespace CalamaresUtils::Locale; - const TimeZoneData* closest = nullptr; - for ( auto it = m_zonesData->begin(); it; ++it ) - { - const auto* zone = *it; - if ( zone ) - { - QPoint locPos = TimeZoneImageList::getLocationPosition( zone->longitude(), zone->latitude() ); - - if ( ( abs( mX - locPos.x() ) + abs( mY - locPos.y() ) < abs( mX - nX ) + abs( mY - nY ) ) ) - { - closest = zone; - nX = locPos.x(); - nY = locPos.y(); - } - } - } + int mX = event->pos().x(); + int mY = event->pos().y(); + auto distance = [&]( const CalamaresUtils::Locale::TimeZoneData* zone ) { + QPoint locPos = TimeZoneImageList::getLocationPosition( zone->longitude(), zone->latitude() ); + return double( abs( mX - locPos.x() ) + abs( mY - locPos.y() ) ); + }; + const auto* closest = m_zonesData->find( distance ); if ( closest ) { // Set zone image and repaint widget From 00626fd96c9341d2a0a1858481d20522a6593067 Mon Sep 17 00:00:00 2001 From: Adriaan de Groot Date: Sat, 8 Aug 2020 22:12:22 +1000 Subject: [PATCH 4/8] [libcalamares] Refactor timezone loading - load from a TextStream. This is prep-work for alternate TZ data sources. --- src/libcalamares/locale/TimeZone.cpp | 130 ++++++++++++++------------- 1 file changed, 68 insertions(+), 62 deletions(-) diff --git a/src/libcalamares/locale/TimeZone.cpp b/src/libcalamares/locale/TimeZone.cpp index 4c641cd17..4cd2bfb9a 100644 --- a/src/libcalamares/locale/TimeZone.cpp +++ b/src/libcalamares/locale/TimeZone.cpp @@ -110,81 +110,87 @@ RegionData::tr() const } static void -loadTZData( RegionVector& regions, ZoneVector& zones ) +loadTZData( RegionVector& regions, ZoneVector& zones, QTextStream& in ) { - QFile file( TZ_DATA_FILE ); - if ( file.open( QIODevice::ReadOnly | QIODevice::Text ) ) + while ( !in.atEnd() ) { - QTextStream in( &file ); - while ( !in.atEnd() ) + QString line = in.readLine().trimmed().split( '#', SplitKeepEmptyParts ).first().trimmed(); + if ( line.isEmpty() ) { - QString line = in.readLine().trimmed().split( '#', SplitKeepEmptyParts ).first().trimmed(); - if ( line.isEmpty() ) - { - continue; - } + continue; + } - QStringList list = line.split( QRegExp( "[\t ]" ), SplitSkipEmptyParts ); - if ( list.size() < 3 ) - { - continue; - } + QStringList list = line.split( QRegExp( "[\t ]" ), SplitSkipEmptyParts ); + if ( list.size() < 3 ) + { + continue; + } - QStringList timezoneParts = list.at( 2 ).split( '/', SplitSkipEmptyParts ); - if ( timezoneParts.size() < 2 ) - { - continue; - } + QStringList timezoneParts = list.at( 2 ).split( '/', SplitSkipEmptyParts ); + if ( timezoneParts.size() < 2 ) + { + continue; + } - QString region = timezoneParts.first().trimmed(); - if ( region.isEmpty() ) - { - continue; - } + QString region = timezoneParts.first().trimmed(); + if ( region.isEmpty() ) + { + continue; + } - QString countryCode = list.at( 0 ).trimmed(); - if ( countryCode.size() != 2 ) - { - continue; - } + QString countryCode = list.at( 0 ).trimmed(); + if ( countryCode.size() != 2 ) + { + continue; + } - timezoneParts.removeFirst(); - QString zone = timezoneParts.join( '/' ); - if ( zone.length() < 2 ) - { - continue; - } + timezoneParts.removeFirst(); + QString zone = timezoneParts.join( '/' ); + if ( zone.length() < 2 ) + { + continue; + } - QString position = list.at( 1 ); - int cooSplitPos = position.indexOf( QRegExp( "[-+]" ), 1 ); - double latitude; - double longitude; - if ( cooSplitPos > 0 ) - { - latitude = getRightGeoLocation( position.mid( 0, cooSplitPos ) ); - longitude = getRightGeoLocation( position.mid( cooSplitPos ) ); - } - else - { - continue; - } + QString position = list.at( 1 ); + int cooSplitPos = position.indexOf( QRegExp( "[-+]" ), 1 ); + double latitude; + double longitude; + if ( cooSplitPos > 0 ) + { + latitude = getRightGeoLocation( position.mid( 0, cooSplitPos ) ); + longitude = getRightGeoLocation( position.mid( cooSplitPos ) ); + } + else + { + continue; + } - // Now we have region, zone, country, lat and longitude - const RegionData* existingRegion = nullptr; - for ( const auto* p : regions ) - { - if ( p->key() == region ) - { - existingRegion = p; - break; - } - } - if ( !existingRegion ) + // Now we have region, zone, country, lat and longitude + const RegionData* existingRegion = nullptr; + for ( const auto* p : regions ) + { + if ( p->key() == region ) { - regions.append( new RegionData( region ) ); + existingRegion = p; + break; } - zones.append( new TimeZoneData( region, zone, countryCode, latitude, longitude ) ); } + if ( !existingRegion ) + { + regions.append( new RegionData( region ) ); + } + zones.append( new TimeZoneData( region, zone, countryCode, latitude, longitude ) ); + } +} + +static void +loadTZData( RegionVector& regions, ZoneVector& zones ) +{ + QFile file( TZ_DATA_FILE ); + if ( file.open( QIODevice::ReadOnly | QIODevice::Text ) ) + { + QTextStream in( &file ); + loadTZData( regions, zones, in ); } } From 6a33e72b58a8412a9c6db5474f6eb6ad6c9495be Mon Sep 17 00:00:00 2001 From: Adriaan de Groot Date: Sat, 8 Aug 2020 23:14:20 +1000 Subject: [PATCH 5/8] [libcalamares] Refactor test to be data-driven - this test is going to get a lot more cases, so prepare for that --- src/libcalamares/locale/Tests.cpp | 30 +++++++++++++++++++----------- 1 file changed, 19 insertions(+), 11 deletions(-) diff --git a/src/libcalamares/locale/Tests.cpp b/src/libcalamares/locale/Tests.cpp index e498ac039..d73331d44 100644 --- a/src/libcalamares/locale/Tests.cpp +++ b/src/libcalamares/locale/Tests.cpp @@ -52,6 +52,7 @@ private Q_SLOTS: void testComplexZones(); void testTZLookup(); void testTZIterator(); + void testLocationLookup_data(); void testLocationLookup(); }; @@ -391,23 +392,30 @@ LocaleTests::testTZIterator() } void -LocaleTests::testLocationLookup() +LocaleTests::testLocationLookup_data() { - const CalamaresUtils::Locale::ZonesModel zones; + QTest::addColumn< double >( "latitude" ); + QTest::addColumn< double >( "longitude" ); + QTest::addColumn< QString >( "name" ); - const auto* zone = zones.find( 50.0, 0.0 ); - QVERIFY( zone ); - QCOMPARE( zone->zone(), QStringLiteral( "London" ) ); + QTest::newRow( "London" ) << 50.0 << 0.0 << QString( "London" ); + QTest::newRow( "Tarawa E" ) << 0.0 << 179.0 << QString( "Tarawa" ); + QTest::newRow( "Tarawa W" ) << 0.0 << -179.0 << QString( "Tarawa" ); +} - // Tarawa is close to "the other side of the world" from London - zone = zones.find( 0.0, 179.0 ); - QVERIFY( zone ); - QCOMPARE( zone->zone(), QStringLiteral( "Tarawa" ) ); +void +LocaleTests::testLocationLookup() +{ + const CalamaresUtils::Locale::ZonesModel zones; + + QFETCH( double, latitude ); + QFETCH( double, longitude ); + QFETCH( QString, name ); - zone = zones.find( 0.0, -179.0 ); + const auto* zone = zones.find( latitude, longitude ); QVERIFY( zone ); - QCOMPARE( zone->zone(), QStringLiteral( "Tarawa" ) ); + QCOMPARE( zone->zone(), name ); } From 028d424c73bbe129128b5314a83698bb076b70d7 Mon Sep 17 00:00:00 2001 From: Adriaan de Groot Date: Sat, 8 Aug 2020 23:40:13 +1000 Subject: [PATCH 6/8] [libcalamares] Expand testing of TZ location lookup - Cape Town is in South Africa, so one might expect it to get South Africa's timezone -- which is Africa/Johannesburg -- but Windhoek is closer, so it gets that. - Port Elisabeth is similar: Maseru lies between it an Johannesburg, so it gets the wrong timezone, too. These both illustrate how the limited resolution of the map, together with the "closest location" lookup, can give poor results. For most of South Africa, the "wrong" timezone is closer than the right one. --- src/libcalamares/locale/Tests.cpp | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/libcalamares/locale/Tests.cpp b/src/libcalamares/locale/Tests.cpp index d73331d44..d9bfb9133 100644 --- a/src/libcalamares/locale/Tests.cpp +++ b/src/libcalamares/locale/Tests.cpp @@ -402,6 +402,11 @@ LocaleTests::testLocationLookup_data() QTest::newRow( "Tarawa E" ) << 0.0 << 179.0 << QString( "Tarawa" ); QTest::newRow( "Tarawa W" ) << 0.0 << -179.0 << QString( "Tarawa" ); + QTest::newRow( "Johannesburg" ) << -26.0 << 28.0 << QString( "Johannesburg" ); // South Africa + QTest::newRow( "Maseru" ) << -29.0 << 27.0 << QString( "Maseru" ); // Lesotho + QTest::newRow( "Windhoek" ) << -22.0 << 17.0 << QString( "Windhoek" ); // Namibia + QTest::newRow( "Port Elisabeth" ) << -33.0 << 25.0 << QString( "Johannesburg" ); // South Africa + QTest::newRow( "Cape Town" ) << -33.0 << 18.0 << QString( "Johannesburg" ); // South Africa } void From e35992cf0bc346da2f833d1a35c7326f00c2b1a2 Mon Sep 17 00:00:00 2001 From: Adriaan de Groot Date: Sun, 9 Aug 2020 00:05:40 +1000 Subject: [PATCH 7/8] [libcalamares] Add spot-patches to timezone data - for the purposes of Calamares's nearest-location selection algorithm for timezone selection, introduce spot patches: alternate markers on the map to indicate "things close to here belong in this timezone". - hide the implementation detail in the find() methods. --- src/libcalamares/locale/TimeZone.cpp | 64 +++++++++++++++++++++------- 1 file changed, 49 insertions(+), 15 deletions(-) diff --git a/src/libcalamares/locale/TimeZone.cpp b/src/libcalamares/locale/TimeZone.cpp index 4cd2bfb9a..ddc705548 100644 --- a/src/libcalamares/locale/TimeZone.cpp +++ b/src/libcalamares/locale/TimeZone.cpp @@ -183,17 +183,30 @@ loadTZData( RegionVector& regions, ZoneVector& zones, QTextStream& in ) } } -static void -loadTZData( RegionVector& regions, ZoneVector& zones ) -{ - QFile file( TZ_DATA_FILE ); - if ( file.open( QIODevice::ReadOnly | QIODevice::Text ) ) - { - QTextStream in( &file ); - loadTZData( regions, zones, in ); - } -} - +/** @brief Extra, fake, timezones + * + * The timezone locations in zone.tab are not always very useful, + * given Calamares's standard "nearest zone" algorithm: for instance, + * in most locations physically in the country of South Africa, + * Maseru (the capital of Lesotho, and location for timezone Africa/Maseru) + * is closer than Johannesburg (the location for timezone Africa/Johannesburg). + * + * The algorithm picks the wrong place. This is for instance annoying + * when clicking on Cape Town, you get Maseru, and to get Johannesburg + * you need to click somewhere very carefully north of Maserru. + * + * These alternate zones are used to introduce "extra locations" + * into the timezone database, in order to influence the closest-location + * algorithm. Lines are formatted just like in zone.tab: remember the \n + */ +static const char altZones[] = + /* This extra zone is north-east of Karoo National park, + * and means that Western Cape province and a good chunk of + * Northern- and Eastern- Cape provinces get pulled in to Johannesburg. + * Bloemfontein is still closer to Maseru than either correct zone, + * but this is a definite improvement. + */ + "ZA -3230+02259 Africa/Johannesburg\n"; class Private : public QObject { @@ -201,13 +214,27 @@ class Private : public QObject public: RegionVector m_regions; ZoneVector m_zones; + ZoneVector m_altZones; //< Extra locations for zones Private() { m_regions.reserve( 12 ); // reasonable guess m_zones.reserve( 452 ); // wc -l /usr/share/zoneinfo/zone.tab - loadTZData( m_regions, m_zones ); + // Load the official timezones + { + QFile file( TZ_DATA_FILE ); + if ( file.open( QIODevice::ReadOnly | QIODevice::Text ) ) + { + QTextStream in( &file ); + loadTZData( m_regions, m_zones, in ); + } + } + // Load the alternate zones (see documentation at altZones) + { + QTextStream in( altZones ); + loadTZData( m_regions, m_altZones, in ); + } std::sort( m_regions.begin(), m_regions.end(), []( const RegionData* lhs, const RegionData* rhs ) { return lhs->key() < rhs->key(); @@ -343,9 +370,9 @@ ZonesModel::find( const QString& region, const QString& zone ) const } STATICTEST const TimeZoneData* -find( const ZoneVector& zones, const std::function< double( const TimeZoneData* ) >& distanceFunc ) +find( double startingDistance, const ZoneVector& zones, const std::function< double( const TimeZoneData* ) >& distanceFunc ) { - double smallestDistance = 1000000.0; + double smallestDistance = startingDistance; const TimeZoneData* closest = nullptr; for ( const auto* zone : zones ) @@ -363,7 +390,14 @@ find( const ZoneVector& zones, const std::function< double( const TimeZoneData* const TimeZoneData* ZonesModel::find( const std::function< double( const TimeZoneData* ) >& distanceFunc ) const { - return CalamaresUtils::Locale::find( m_private->m_zones, distanceFunc ); + const auto* officialZone = CalamaresUtils::Locale::find( 1000000.0, m_private->m_zones, distanceFunc ); + const auto* altZone = CalamaresUtils::Locale::find( distanceFunc( officialZone ), m_private->m_altZones, distanceFunc ); + + // If nothing was closer than the official zone already was, altZone is + // nullptr; but if there is a spot-patch, then we need to re-find + // the zone by name, since we want to always return pointers into + // m_zones, not into the alternative spots. + return altZone ? find( altZone->region(), altZone->zone() ) : officialZone; } const TimeZoneData* From 5e5701363c0bea958e20497d736e144c17685780 Mon Sep 17 00:00:00 2001 From: Adriaan de Groot Date: Sun, 9 Aug 2020 00:20:04 +1000 Subject: [PATCH 8/8] [libcalamares] Test the spot-patch for Johannesburg - Add a note about notation, degrees-minutes --- src/libcalamares/locale/Tests.cpp | 32 +++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/src/libcalamares/locale/Tests.cpp b/src/libcalamares/locale/Tests.cpp index d9bfb9133..b755a61a5 100644 --- a/src/libcalamares/locale/Tests.cpp +++ b/src/libcalamares/locale/Tests.cpp @@ -54,6 +54,7 @@ private Q_SLOTS: void testTZIterator(); void testLocationLookup_data(); void testLocationLookup(); + void testLocationLookup2(); }; LocaleTests::LocaleTests() {} @@ -423,6 +424,37 @@ LocaleTests::testLocationLookup() QCOMPARE( zone->zone(), name ); } +void +LocaleTests::testLocationLookup2() +{ + // Official + // ZA -2615+02800 Africa/Johannesburg + // Spot patch + // "ZA -3230+02259 Africa/Johannesburg\n"; + + const CalamaresUtils::Locale::ZonesModel zones; + const auto* zone = zones.find( -26.15, 28.00 ); + QCOMPARE( zone->zone(), QString( "Johannesburg" ) ); + // The TZ data sources use minutes-and-seconds notation, + // so "2615" is 26 degrees, 15 minutes, and 15 minutes is + // one-quarter of a degree. + QCOMPARE( zone->latitude(), -26.25 ); + QCOMPARE( zone->longitude(), 28.00 ); + + // Elsewhere in South Africa + const auto* altzone = zones.find( -32.0, 22.0 ); + QCOMPARE( altzone, zone ); // same pointer + QCOMPARE( altzone->zone(), QString( "Johannesburg" ) ); + QCOMPARE( altzone->latitude(), -26.25 ); + QCOMPARE( altzone->longitude(), 28.00 ); + + altzone = zones.find( -29.0, 27.0 ); + QCOMPARE( altzone->zone(), QString( "Maseru" ) ); + // -2928, that's -29 and 28/60 of a degree, is almost half, but we don't want + // to fall foul of variations in double-precision + QCOMPARE( trunc( altzone->latitude() * 1000.0 ), -29466 ); +} + QTEST_GUILESS_MAIN( LocaleTests )