Fix/location enums (#142)

* Improve stability of persisted enums (WIP - won't compile) - Specifically when persisted IDs do not match with actual enum values * Use enum struct instead of just enum * Properly cast enum struct values * Implement range-check for Bearing enum - TODO: Do this for all persisted enums - Can we write a generic method for this? * Check enum values whether the are in the expected range * Validate enums stored in the logbook when reading * Update * Add instructions about ssh signing key setup on github.com * Add instructions about win-ssh-askpass (Qt Creator) * Add path to git-askpass.exe * Validate enum domain for plugins. * Validate import / export enumeration settings * Simplify int parsing error handling
till213 · May 9, 2024 · e2cc42c · e2cc42c
1 parent 778fc74
commit e2cc42c
Show file tree

Hide file tree

Showing 46 changed files with 375 additions and 338 deletions.
diff --git a/BUILD.md b/BUILD.md
@@ -104,3 +104,37 @@ When compiling on Windows with MinGW the invoked tool `windres.exe` (which compi
 Also refer e.g. to https://bugreports.qt.io/browse/QTBUG-62918
 
 The workaround for the time being is to place the Sky Dolly sources into a path without spaces. This issue still seems to be present with the MinGW 11 (coming with Qt 6.x) provided windres.exe.
+
+## Signed Commits
+The following setups an optional commit signing:
+
+### Local Setup
+
+- Globally set the signing format to SSH (which is the most convenient in case SSH is used to login to github.com anyway):
+```
+git config --global gpg.format ssh
+git config --global user.signingkey "/path/to/public/ssh/key"
+```
+
+E.g. on Windows:
+```
+git config --global user.signingkey "%HOME%\.ssh\id_rsa.pub"
+```
+
+Take note of the "quotes" around the path.
+
+- Globally enable signing of each commit
+```
+git config --global commit.gpgsign true
+```
+
+### Github Setup
+Github needs to be able to verify the signed commits.
+
+- Add the public ssh key with *New SSH Key* as *Signing Key* to [github.com](https://github.com/settings/keys)
+
+### Qt Creator AskPass Setup
+Qt Creator may need to be told where to find `win-ssh-askpass` (specifically on Windows).
+
+- Edit | Settings... | Version Control -> Common
+- In case Git for Windows is installed the path may be set to `C:\Program Files\Git\mingw64\bin\git-askpass.exe` instead
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -14,9 +14,11 @@
   * The aircraft type is properly set in the imported flight again
 - On Windows 11 the default style is set to "Fusion", as a workaround
   * The new "Windows 11" style still has a few known visual glitches:
-    - Active toolbar buttons are almost unreadable in dark mode
-    - The time display ("time edit" widget) only shows two digits
+    - Active toolbar buttons are almost unreadable in dark mode [[QTBUG-124286](https://bugreports.qt.io/browse/QTBUG-124286)]
+    - The time display ("time edit" widget) only shows two digits [[QTBUG-124150](https://bugreports.qt.io/browse/QTBUG-124150)]
+    - Alternating table rows are not visually distinct [[QTBUG-124564](https://bugreports.qt.io/browse/QTBUG-124564)]
   * However the "Windows 11" style can still be set, under *File | Settings... | User Interface*
+- Properly validate persisted enumeration values when restoring plugin settings
 
 ## 0.17.0
 

diff --git a/src/Kernel/include/Kernel/Enum.h b/src/Kernel/include/Kernel/Enum.h
@@ -34,12 +34,32 @@ namespace Enum
 {
     /*!
      * Returns the underlying type for the given enumeration \c e.
+     *
+     * Note: Can be replaced in C++23 with https://en.cppreference.com/w/cpp/utility/to_underlying
      */
     template<typename E>
     constexpr auto underly(E e) noexcept
     {
         return static_cast<std::underlying_type_t<E>>(e);
     }
+
+    /*!
+     * Returns whether the enumeration \c E contains the given underlying \c value.
+     *
+     * Assumptions:
+     * - The enumeration \c E contains the following members: \c First (with lowest value) and \c Last (with highest value)
+     * - The underlying values are without "gaps", that is First = 0, Value1 = First, Value2 = 1, Value3 = 2, ..., ValueN = 10, Last = ValueN
+     *
+     * \param value
+     *        the underlying value to be checked
+     * \return \c true if the \c value is a valid underlying value contained in enumeration of type \c E; \c false else
+     */
+    template<typename E> requires(std::is_enum_v<E>)
+    constexpr auto contains(std::underlying_type_t<E> value)
+    {
+        return value >= static_cast<std::underlying_type_t<E>>(E::First) &&
+               value <= static_cast<std::underlying_type_t<E>>(E::Last);
+    }
 }
 
 #endif // ENUM_H
diff --git a/src/Kernel/include/Kernel/SampleRate.h b/src/Kernel/include/Kernel/SampleRate.h
@@ -55,7 +55,7 @@ namespace SampleRate
         Hz30,
         Hz45,
         Hz50,
-        Hz60,
+        Hz60
     };
 
     /*!

diff --git a/src/Kernel/include/Kernel/SkyMath.h b/src/Kernel/include/Kernel/SkyMath.h
@@ -78,21 +78,23 @@ namespace SkyMath
     /*!
      * Defines how the aircraft time offset is to be synchronised.
      *
-     * These values are peristed in the application settings.
+     * Implementation note: these values are peristed in the application settings.
      */
     enum struct TimeOffsetSync {
+        First = 0,
         /*! No synchronisation to be done. */
-        None = 0,
+        None = First,
         /*! Both date and time of the flight creation time are taken into account */
-        DateAndTime = 1,
+        DateAndTime,
         /*!
          * Only the time is taken into account. For example a flight that was recorded
          * a day before, on the 2023-02-14 10:45:00Z is only considered to be 15 minutes
          * behind of a flight recorded on the 2023-02-15 11:00:00Z (and not a day plus
          * 15 minutes). This is useful when importing e.g. real-world flights that
          * happened on different days, but should still be synchronised "on the same day".
          */
-        TimeOnly = 2
+        TimeOnly,
+        Last = TimeOnly
     };
 
     /*!

diff --git a/src/Model/include/Model/SimType.h b/src/Model/include/Model/SimType.h
@@ -36,7 +36,7 @@ namespace SimType {
     /*!
      * The state of the aircraft lights.
      *
-     * Implementation note: those values act as actual flag values which get persisted in the database.
+     * Implementation note: these values act as actual flag values that get persisted in the database.
      */
     enum struct LightState {
         None = 0x0000,
@@ -57,10 +57,11 @@ namespace SimType {
     /*!
      * The surface type describes the surface on (over) which the flight has started.
      *
-     * Implementation note: those values act as actual IDs that get persisted in the database.
+     * Implementation note: these values act as actual IDs that get persisted in the database.
      */
     enum struct SurfaceType {
-        Unknown = 0,
+        First = 0,
+        Unknown = First,
         Concrete = 1,
         Grass = 2,
         Water = 3,
@@ -85,49 +86,56 @@ namespace SimType {
         Sand = 22,
         Shale = 23,
         Tarmac = 24,
-        WrightFlyerTrack  = 25
+        WrightFlyerTrack  = 25,
+        Last = WrightFlyerTrack
     };
 
     /*!
      * The surface condition describes the condition of the runway on which the flight has (potentially) started.
      *
-     * Implementation note: those values act as actual IDs that get persisted in the database.
+     * Implementation note: these values act as actual IDs that get persisted in the database.
      */
     enum struct SurfaceCondition {
-        Unknown = 0,
-        Normal = 1,
-        Wet = 2,
-        Icy = 3,
-        Snow = 4
+        First = 0,
+        Unknown = First,
+        Normal,
+        Wet ,
+        Icy,
+        Snow,
+        Last = Snow
     };
 
     /*!
      * The engine type of the aircraft.
      *
-     * Implementation note: those values act as actual IDs which get persisted in the database.
+     * Implementation note: these values act as actual IDs that get persisted in the database.
      */
     enum struct EngineType {
-        Unknown = 0,
-        Piston = 1,
-        Jet = 2,
-        None = 3,
-        HeloBellTurbine = 4,
-        Unsupported = 5,
-        Turboprop = 6,
+        First = 0,
+        Unknown = First,
+        Piston,
+        Jet,
+        None,
+        HeloBellTurbine,
+        Unsupported,
+        Turboprop,
         // Only used for selection (never assigned/persisted)
-        All
+        All,
+        Last = All
     };
 
     /*!
      * The precipitation state at the beginning of the flight.
      *
-     * Implementation note: those values act as actual IDs which get persisted in the database.
+     * Implementation note: these values act as actual IDs that get persisted in the database.
      */
     enum struct PrecipitationState {
-        Unknown = 0,
-        None = 1,
-        Rain = 2,
-        Snow = 3
+        First = 0,
+        Unknown = First,
+        None,
+        Rain,
+        Snow,
+        Last = Snow
     };
 
     inline QString surfaceTypeToString(SurfaceType surfaceType) noexcept {

diff --git a/src/Persistence/include/Persistence/Service/LocationService.h b/src/Persistence/include/Persistence/Service/LocationService.h
@@ -41,10 +41,17 @@ struct LocationServicePrivate;
 class PERSISTENCE_API LocationService final
 {
 public:
+    /*!
+     * Defines how to deal with duplicate locations upon import.
+     *
+     * Implementation note: these values are peristed in the application settings.
+     */
     enum struct Mode {
-        Ignore,
+        First = 0,
+        Ignore = First,
         Update,
-        Insert
+        Insert,
+        Last = Insert
     };
     LocationService(QString connectionName = Const::DefaultConnectionName) noexcept;
     LocationService(const LocationService &rhs) = delete;

diff --git a/src/Persistence/src/Dao/SQLite/SQLiteAircraftTypeDao.cpp b/src/Persistence/src/Dao/SQLite/SQLiteAircraftTypeDao.cpp
@@ -124,7 +124,8 @@ AircraftType SQLiteAircraftTypeDao::getByType(const QString &type, bool *ok) con
             const int nofEnginesIdx = record.indexOf(QStringLiteral("nof_engines"));
             aircraftType.category = query.value(categoryIdx).toString();
             aircraftType.wingSpan = query.value(wingSpanIdx).toInt();
-            aircraftType.engineType = static_cast<SimType::EngineType>(query.value(engineTypeIdx).toInt());
+            const auto enumValue = query.value(engineTypeIdx).toInt();
+            aircraftType.engineType = Enum::contains<SimType::EngineType>(enumValue) ? static_cast<SimType::EngineType>(enumValue) : SimType::EngineType::First;
             aircraftType.numberOfEngines = query.value(nofEnginesIdx).toInt();
         }
     }
@@ -168,8 +169,9 @@ std::vector<AircraftType> SQLiteAircraftTypeDao::getAll(bool *ok) const noexcept
         while (query.next()) {
             const QString type = query.value(typeIdx).toString();
             const QString category = query.value(categoryIdx).toString();
-            const int wingSpan = query.value(wingSpanIdx).toInt();
-            const SimType::EngineType engineType = static_cast<SimType::EngineType>(query.value(engineTypeIdx).toInt());
+            const int wingSpan = query.value(wingSpanIdx).toInt();            
+            const auto enumValue = query.value(engineTypeIdx).toInt();
+            const SimType::EngineType engineType = Enum::contains<SimType::EngineType>(enumValue) ? static_cast<SimType::EngineType>(enumValue) : SimType::EngineType::First;
             const int numberOfEngines = query.value(nofEnginesIdx).toInt();
             aircraftTypes.emplace_back(type, category, wingSpan, engineType, numberOfEngines);
         }

diff --git a/src/Persistence/src/Dao/SQLite/SQLiteFlightDao.cpp b/src/Persistence/src/Dao/SQLite/SQLiteFlightDao.cpp
@@ -146,8 +146,10 @@ bool SQLiteFlightDao::get(std::int64_t id, FlightData &flightData) const noexcep
             flightData.flightNumber = query.value(flightNumberIdx).toString();
 
             FlightCondition &flightCondition = flightData.flightCondition;
-            flightCondition.surfaceType = static_cast<SimType::SurfaceType>(query.value(surfaceTypeIdx).toInt());
-            flightCondition.surfaceCondition = static_cast<SimType::SurfaceCondition>(query.value(surfaceConditionIdx).toInt());
+            auto enumValue = query.value(surfaceTypeIdx).toInt();
+            flightCondition.surfaceType = Enum::contains<SimType::SurfaceType>(enumValue) ? static_cast<SimType::SurfaceType>(enumValue) : SimType::SurfaceType::First;
+            enumValue = query.value(surfaceConditionIdx).toInt();
+            flightCondition.surfaceCondition = Enum::contains<SimType::SurfaceCondition>(enumValue) ? static_cast<SimType::SurfaceCondition>(enumValue) : SimType::SurfaceCondition::First;
             flightCondition.onAnyRunway = query.value(onAnyRunwayIdx).toBool();
             flightCondition.onParkingSpot = query.value(onParkingSpotIdx).toBool();            
             flightCondition.groundAltitude = query.value(groundAltitudeIdx).toFloat();
@@ -159,7 +161,8 @@ bool SQLiteFlightDao::get(std::int64_t id, FlightData &flightData) const noexcep
             flightCondition.seaLevelPressure = query.value(seaLevelPressureIdx).toFloat();
             flightCondition.pitotIcingPercent = query.value(pitotIcingIdx).toInt();
             flightCondition.structuralIcingPercent = query.value(structuralIcingIdx).toInt();
-            flightCondition.precipitationState = static_cast<SimType::PrecipitationState>(query.value(precipitationStateIdx).toInt());
+            enumValue = query.value(precipitationStateIdx).toInt();
+            flightCondition.precipitationState = Enum::contains<SimType::PrecipitationState>(enumValue) ? static_cast<SimType::PrecipitationState>(enumValue) : SimType::PrecipitationState::First;
             flightCondition.inClouds = query.value(inCloudsIdx).toBool();
             // Persisted times is are already local respectively zulu simulation times
             flightCondition.startLocalTime = query.value(startLocalSimulationTimeIdx).toDateTime();

diff --git a/src/PluginManager/include/PluginManager/Connect/SkyConnectIntf.h b/src/PluginManager/include/PluginManager/Connect/SkyConnectIntf.h
@@ -61,19 +61,20 @@ class PLUGINMANAGER_API SkyConnectIntf : public QObject, public PluginWithOption
     /*!
      * Defines of which aircraft to take control of during formation replay.
      *
-     * Implementation note: those values act as actual IDs that get persisted in the database.
+     * Implementation note: these values are peristed in the application settings.
      */
     enum struct ReplayMode {
+        First = 0,
         /*! All aircraft are controlled by Sky Dolly. */
-        Normal = 0,
+        Normal = First,
         /*! User takes control of recorded user aircraft. */
-        UserAircraftManualControl = 1,
+        UserAircraftManualControl,
         /*! User flies along with all recorded aircraft. */
-        FlyWithFormation = 2
+        FlyWithFormation,
+        Last = FlyWithFormation
     };
 
     enum struct SeekMode {
-
         /*! Continuation of a timeline seek operation ("drag timeline") */
         Continuous,
         /*! A single seek operation (to beginning, to end, to selected position) */

diff --git a/src/PluginManager/include/PluginManager/Flight/FlightExportPluginBaseSettings.h b/src/PluginManager/include/PluginManager/Flight/FlightExportPluginBaseSettings.h
@@ -43,16 +43,18 @@ class PLUGINMANAGER_API FlightExportPluginBaseSettings : public QObject
     /*!
      * Defines how formation flights should be exported.
      *
-     * These values are peristed in the application settings.
+     * Implementation note: these values are peristed in the application settings.
      */
     enum struct FormationExport {
+        First = 0,
         /*! Only the user aircraft is to be exported */
-        UserAircraftOnly = 0,
+        UserAircraftOnly = First,
         /*! All aircraft are to be exported, into one file if possible (depending on the actual file format);
             otherwise into separate files */
-        AllAircraftOneFile = 1,
+        AllAircraftOneFile,
         /*! All aircraft are to be exported, into separate files each */
-        AllAircraftSeparateFiles = 2
+        AllAircraftSeparateFiles,
+        Last = AllAircraftSeparateFiles
     };
 
     FlightExportPluginBaseSettings() noexcept;

diff --git a/src/PluginManager/include/PluginManager/Flight/FlightImportPluginBaseSettings.h b/src/PluginManager/include/PluginManager/Flight/FlightImportPluginBaseSettings.h
@@ -43,15 +43,17 @@ class PLUGINMANAGER_API FlightImportPluginBaseSettings : public QObject
     /*!
      * Defines how aircraft are to be imported.
      *
-     * These values are peristed in the application settings.
+     * Implementation note: these values are peristed in the application settings.
      */
     enum struct AircraftImportMode {
+        First = 0,
         /*! All aircraft are added to the existing flight (loaded in memory). */
-        AddToCurrentFlight = 0,
+        AddToCurrentFlight = First,
         /*! A new flight is generated and all aircraft are added to it. */
-        AddToNewFlight = 1,
+        AddToNewFlight,
         /*! For each imported aircraft a new flight is generated. */
-        SeparateFlights  = 2
+        SeparateFlights,
+        Last = SeparateFlights
     };
 
     FlightImportPluginBaseSettings() noexcept;

diff --git a/src/PluginManager/src/Flight/FlightExportPluginBaseSettings.cpp b/src/PluginManager/src/Flight/FlightExportPluginBaseSettings.cpp
@@ -143,18 +143,13 @@ void FlightExportPluginBaseSettings::addKeysWithDefaults(Settings::KeysWithDefau
 void FlightExportPluginBaseSettings::restoreSettings(const Settings::ValuesByKey &valuesByKey) noexcept
 {
     bool ok {true};
-    int enumeration = valuesByKey.at(QString::fromLatin1(::ResamplingPeriodKey)).toInt(&ok);
-    if (ok) {
-        d->resamplingPeriod = static_cast<SampleRate::ResamplingPeriod >(enumeration);
-    } else {
-        d->resamplingPeriod = ::DefaultResamplingPeriod;
-    }
-    enumeration = valuesByKey.at(QString::fromLatin1(::FormationExportKey)).toInt(&ok);
-    if (ok) {
-        d->formationExport = static_cast<FormationExport >(enumeration);
-    } else {
-        d->formationExport = ::DefaultFormationExport;
-    }
+    auto enumValue = valuesByKey.at(QString::fromLatin1(::ResamplingPeriodKey)).toInt(&ok);
+    // No enumeration domain validation: any resampling period (Hz) is valid
+    d->resamplingPeriod = ok ? static_cast<SampleRate::ResamplingPeriod>(enumValue) : ::DefaultResamplingPeriod;
+
+    enumValue = valuesByKey.at(QString::fromLatin1(::FormationExportKey)).toInt(&ok);
+    d->formationExport = ok && Enum::contains<FormationExport>(enumValue) ? static_cast<FormationExport>(enumValue) : ::DefaultFormationExport;
+
     d->openExportedFilesEnabled = valuesByKey.at(QString::fromLatin1(::OpenExportedFilesEnabledKey)).toBool();
 
     restoreSettingsExtn(valuesByKey);
-Original file line number
+Diff line change
@@ Expand Up / @@ -55,7 +55,7 @@ namespace SampleRate @@
             Hz30,
             Hz45,
             Hz50,
-            Hz60,
+            Hz60
         };
         /*!
@@ Expand Down @@