From a7e1a1f9fcd003fc0e4afbe7d3eb61d6aaeb31f0 Mon Sep 17 00:00:00 2001 From: Adriaan de Groot Date: Sun, 12 Jan 2020 11:37:22 +0100 Subject: [PATCH 1/3] [libcalamaresui] Refactor Module::initFrom() - generic initFrom() also sets the instance id - subclass-specific initFrom() now pure virtual in base - chase changes in subclasses --- .../modulesystem/CppJobModule.cpp | 1 - src/libcalamaresui/modulesystem/Module.cpp | 35 +++++++++---------- src/libcalamaresui/modulesystem/Module.h | 7 +++- .../modulesystem/ProcessJobModule.cpp | 1 - .../modulesystem/PythonJobModule.cpp | 1 - .../modulesystem/PythonQtViewModule.cpp | 1 - .../modulesystem/ViewModule.cpp | 1 - 7 files changed, 23 insertions(+), 24 deletions(-) diff --git a/src/libcalamaresui/modulesystem/CppJobModule.cpp b/src/libcalamaresui/modulesystem/CppJobModule.cpp index c6571dbf6..2eddeda86 100644 --- a/src/libcalamaresui/modulesystem/CppJobModule.cpp +++ b/src/libcalamaresui/modulesystem/CppJobModule.cpp @@ -86,7 +86,6 @@ CppJobModule::jobs() const void CppJobModule::initFrom( const QVariantMap& moduleDescriptor ) { - Module::initFrom( moduleDescriptor ); QDir directory( location() ); QString load; if ( !moduleDescriptor.value( "load" ).toString().isEmpty() ) diff --git a/src/libcalamaresui/modulesystem/Module.cpp b/src/libcalamaresui/modulesystem/Module.cpp index 799da941e..4bc759cb1 100644 --- a/src/libcalamaresui/modulesystem/Module.cpp +++ b/src/libcalamaresui/modulesystem/Module.cpp @@ -48,8 +48,24 @@ static const char EMERGENCY[] = "emergency"; namespace Calamares { +Module::Module() + : m_loaded( false ) +{ +} + Module::~Module() {} +void +Module::initFrom( const QVariantMap& moduleDescriptor, const QString& id ) +{ + m_name = moduleDescriptor.value( "name" ).toString(); + m_instanceId = id; + if ( moduleDescriptor.contains( EMERGENCY ) ) + { + m_maybe_emergency = moduleDescriptor[ EMERGENCY ].toBool(); + } +} + Module* Module::fromDescriptor( const QVariantMap& moduleDescriptor, const QString& instanceId, @@ -131,8 +147,7 @@ Module::fromDescriptor( const QVariantMap& moduleDescriptor, return nullptr; } - m->m_instanceId = instanceId; - + m->initFrom( moduleDescriptor, instanceId ); m->initFrom( moduleDescriptor ); try { @@ -290,22 +305,6 @@ Module::configurationMap() } -Module::Module() - : m_loaded( false ) -{ -} - - -void -Module::initFrom( const QVariantMap& moduleDescriptor ) -{ - m_name = moduleDescriptor.value( "name" ).toString(); - if ( moduleDescriptor.contains( EMERGENCY ) ) - { - m_maybe_emergency = moduleDescriptor[ EMERGENCY ].toBool(); - } -} - RequirementsList Module::checkRequirements() { diff --git a/src/libcalamaresui/modulesystem/Module.h b/src/libcalamaresui/modulesystem/Module.h index e4101b767..03eb09cb0 100644 --- a/src/libcalamaresui/modulesystem/Module.h +++ b/src/libcalamaresui/modulesystem/Module.h @@ -173,7 +173,12 @@ public: protected: explicit Module(); - virtual void initFrom( const QVariantMap& moduleDescriptor ); + + /// @brief For subclasses to read their part of the descriptor + virtual void initFrom( const QVariantMap& moduleDescriptor ) = 0; + /// @brief Generic part of descriptor reading (and instance id) + void initFrom( const QVariantMap& moduleDescriptor, const QString& id ); + QVariantMap m_configurationMap; bool m_loaded = false; diff --git a/src/libcalamaresui/modulesystem/ProcessJobModule.cpp b/src/libcalamaresui/modulesystem/ProcessJobModule.cpp index fc4b5f254..405d9efd8 100644 --- a/src/libcalamaresui/modulesystem/ProcessJobModule.cpp +++ b/src/libcalamaresui/modulesystem/ProcessJobModule.cpp @@ -63,7 +63,6 @@ ProcessJobModule::jobs() const void ProcessJobModule::initFrom( const QVariantMap& moduleDescriptor ) { - Module::initFrom( moduleDescriptor ); QDir directory( location() ); m_workingPath = directory.absolutePath(); diff --git a/src/libcalamaresui/modulesystem/PythonJobModule.cpp b/src/libcalamaresui/modulesystem/PythonJobModule.cpp index 46ec1dff1..c9e90a707 100644 --- a/src/libcalamaresui/modulesystem/PythonJobModule.cpp +++ b/src/libcalamaresui/modulesystem/PythonJobModule.cpp @@ -64,7 +64,6 @@ PythonJobModule::jobs() const void PythonJobModule::initFrom( const QVariantMap& moduleDescriptor ) { - Module::initFrom( moduleDescriptor ); QDir directory( location() ); m_workingPath = directory.absolutePath(); diff --git a/src/libcalamaresui/modulesystem/PythonQtViewModule.cpp b/src/libcalamaresui/modulesystem/PythonQtViewModule.cpp index 08e4c5c08..b59126eaa 100644 --- a/src/libcalamaresui/modulesystem/PythonQtViewModule.cpp +++ b/src/libcalamaresui/modulesystem/PythonQtViewModule.cpp @@ -174,7 +174,6 @@ PythonQtViewModule::jobs() const void PythonQtViewModule::initFrom( const QVariantMap& moduleDescriptor ) { - Module::initFrom( moduleDescriptor ); QDir directory( location() ); m_workingPath = directory.absolutePath(); diff --git a/src/libcalamaresui/modulesystem/ViewModule.cpp b/src/libcalamaresui/modulesystem/ViewModule.cpp index 7506e5348..02c771ee2 100644 --- a/src/libcalamaresui/modulesystem/ViewModule.cpp +++ b/src/libcalamaresui/modulesystem/ViewModule.cpp @@ -91,7 +91,6 @@ ViewModule::jobs() const void ViewModule::initFrom( const QVariantMap& moduleDescriptor ) { - Module::initFrom( moduleDescriptor ); QDir directory( location() ); QString load; if ( !moduleDescriptor.value( "load" ).toString().isEmpty() ) From f89c137c90f5b0904b914aa052f05882244550b6 Mon Sep 17 00:00:00 2001 From: Adriaan de Groot Date: Sun, 12 Jan 2020 11:49:10 +0100 Subject: [PATCH 2/3] [libcalamaresui] Migrate module to using InstanceKey - Trying to get away from untyped strings with special meaning. - The "split identifier" branch tried the same thing, but was duplicating the existing InstanceKey.h work. --- src/libcalamaresui/modulesystem/Module.cpp | 34 ++++++---------------- src/libcalamaresui/modulesystem/Module.h | 11 +++---- 2 files changed, 15 insertions(+), 30 deletions(-) diff --git a/src/libcalamaresui/modulesystem/Module.cpp b/src/libcalamaresui/modulesystem/Module.cpp index 4bc759cb1..7608bd3ec 100644 --- a/src/libcalamaresui/modulesystem/Module.cpp +++ b/src/libcalamaresui/modulesystem/Module.cpp @@ -58,8 +58,7 @@ Module::~Module() {} void Module::initFrom( const QVariantMap& moduleDescriptor, const QString& id ) { - m_name = moduleDescriptor.value( "name" ).toString(); - m_instanceId = id; + m_key = ModuleSystem::InstanceKey( moduleDescriptor.value( "name" ).toString(), id ); if ( moduleDescriptor.contains( EMERGENCY ) ) { m_maybe_emergency = moduleDescriptor[ EMERGENCY ].toBool(); @@ -148,6 +147,12 @@ Module::fromDescriptor( const QVariantMap& moduleDescriptor, } m->initFrom( moduleDescriptor, instanceId ); + if ( !m->m_key.isValid() ) + { + cError() << "Module" << instanceId << "invalid ID"; + return nullptr; + } + m->initFrom( moduleDescriptor ); try { @@ -205,7 +210,7 @@ moduleConfigurationCandidates( bool assumeBuildDir, const QString& moduleName, c void Module::loadConfigurationFile( const QString& configFileName ) //throws YAML::Exception { QStringList configCandidates - = moduleConfigurationCandidates( Settings::instance()->debugMode(), m_name, configFileName ); + = moduleConfigurationCandidates( Settings::instance()->debugMode(), name(), configFileName ); for ( const QString& path : configCandidates ) { QFile configFile( path ); @@ -234,28 +239,7 @@ void Module::loadConfigurationFile( const QString& configFileName ) //throws YA return; } } - cDebug() << "No config file for" << m_name << "found anywhere at" << Logger::DebugList( configCandidates ); -} - - -QString -Module::name() const -{ - return m_name; -} - - -QString -Module::instanceId() const -{ - return m_instanceId; -} - - -QString -Module::instanceKey() const -{ - return QString( "%1@%2" ).arg( m_name ).arg( m_instanceId ); + cDebug() << "No config file for" << name() << "found anywhere at" << Logger::DebugList( configCandidates ); } diff --git a/src/libcalamaresui/modulesystem/Module.h b/src/libcalamaresui/modulesystem/Module.h index 03eb09cb0..d96ac152d 100644 --- a/src/libcalamaresui/modulesystem/Module.h +++ b/src/libcalamaresui/modulesystem/Module.h @@ -24,6 +24,8 @@ #include "Requirement.h" #include "UiDllMacro.h" +#include "modulesystem/InstanceKey.h" + #include #include @@ -83,13 +85,13 @@ public: * @brief name returns the name of this module. * @return a string with this module's name. */ - virtual QString name() const final; + QString name() const { return m_key.module(); } /** * @brief instanceId returns the instance id of this module. * @return a string with this module's instance id. */ - virtual QString instanceId() const final; + QString instanceId() const { return m_key.id(); } /** * @brief instanceKey returns the instance key of this module. @@ -98,7 +100,7 @@ public: * For instance, "partition\@partition" (default configuration) or * "locale\@someconfig" (custom configuration) */ - virtual QString instanceKey() const final; + QString instanceKey() const { return m_key.toString(); } /** * @brief location returns the full path of this module's directory. @@ -188,9 +190,8 @@ protected: private: void loadConfigurationFile( const QString& configFileName ); //throws YAML::Exception - QString m_name; QString m_directory; - QString m_instanceId; + ModuleSystem::InstanceKey m_key; }; } // namespace Calamares From ed4127f6616b4db5720d6a2a2b546a70c11d5ef1 Mon Sep 17 00:00:00 2001 From: Adriaan de Groot Date: Sun, 12 Jan 2020 12:18:13 +0100 Subject: [PATCH 3/3] [libcalamaresui] Shuffle the module interface - introduce NamedEnum lookup tables for interface and type - drop "final" and "virtual" from methods that don't make sense as virtual - shuffle declaration order so the virtual API for modules sits together --- src/libcalamaresui/modulesystem/Module.cpp | 64 ++++++++++------- src/libcalamaresui/modulesystem/Module.h | 84 +++++++++++----------- 2 files changed, 81 insertions(+), 67 deletions(-) diff --git a/src/libcalamaresui/modulesystem/Module.cpp b/src/libcalamaresui/modulesystem/Module.cpp index 7608bd3ec..2d2ea3def 100644 --- a/src/libcalamaresui/modulesystem/Module.cpp +++ b/src/libcalamaresui/modulesystem/Module.cpp @@ -27,6 +27,7 @@ #include "utils/Dirs.h" #include "utils/Logger.h" +#include "utils/NamedEnum.h" #include "utils/Yaml.h" #ifdef WITH_PYTHON @@ -152,7 +153,7 @@ Module::fromDescriptor( const QVariantMap& moduleDescriptor, cError() << "Module" << instanceId << "invalid ID"; return nullptr; } - + m->initFrom( moduleDescriptor ); try { @@ -243,42 +244,55 @@ void Module::loadConfigurationFile( const QString& configFileName ) //throws YA } -QString -Module::location() const +static const NamedEnumTable< Module::Type >& +typeNames() { - return m_directory; + using Type = Module::Type; + // *INDENT-OFF* + // clang-format off + static const NamedEnumTable< Type > table{ + { QStringLiteral( "job" ), Type::Job }, + { QStringLiteral( "view" ), Type::View }, + { QStringLiteral( "viewmodule" ), Type::View }, + { QStringLiteral( "jobmodule" ), Type::Job } + }; + // *INDENT-ON* + // clang-format on + return table; } - QString Module::typeString() const { - switch ( type() ) - { - case Type::Job: - return "Job Module"; - case Type::View: - return "View Module"; - } - return QString(); + bool ok = false; + QString v = typeNames().find( type(), ok ); + return ok ? v : QString(); } +static const NamedEnumTable< Module::Interface >& +interfaceNames() +{ + using Interface = Module::Interface; + // *INDENT-OFF* + // clang-format off + static const NamedEnumTable< Interface > table { + { QStringLiteral("process"), Interface::Process }, + { QStringLiteral("qtplugin"), Interface::QtPlugin }, + { QStringLiteral("python"), Interface::Python }, + { QStringLiteral("pythonqt"), Interface::PythonQt } + }; + // *INDENT-ON* + // clang-format on + return table; +} + QString Module::interfaceString() const { - switch ( interface() ) - { - case Interface::Process: - return "External process"; - case Interface::Python: - return "Python (Boost.Python)"; - case Interface::PythonQt: - return "Python (experimental)"; - case Interface::QtPlugin: - return "Qt Plugin"; - } - return QString(); + bool ok = false; + QString v = interfaceNames().find( interface(), ok ); + return ok ? v : QString(); } diff --git a/src/libcalamaresui/modulesystem/Module.h b/src/libcalamaresui/modulesystem/Module.h index d96ac152d..88eba4dcf 100644 --- a/src/libcalamaresui/modulesystem/Module.h +++ b/src/libcalamaresui/modulesystem/Module.h @@ -106,43 +106,7 @@ public: * @brief location returns the full path of this module's directory. * @return the path. */ - virtual QString location() const final; - - /** - * @brief type returns the Type of this module object. - * @return the type enum value. - */ - virtual Type type() const = 0; - - /** - * @brief typeString returns a user-visible string for the module's type. - * @return the type string. - */ - virtual QString typeString() const; - - /** - * @brief interface the Interface used by this module. - * @return the interface enum value. - */ - virtual Interface interface() const = 0; - - /** - * @brief interface returns a user-visible string for the module's interface. - * @return the interface string. - */ - virtual QString interfaceString() const; - - /** - * @brief isLoaded reports on the loaded status of a module. - * @return true if the module's loading phase has finished, otherwise false. - */ - bool isLoaded() const { return m_loaded; } - - /** - * @brief loadSelf initialized the module. - * Subclasses must reimplement this depending on the module type and interface. - */ - virtual void loadSelf() = 0; + QString location() const { return m_directory; } /** * @brief Is this an emergency module? @@ -156,10 +120,10 @@ public: bool isEmergency() const { return m_emergency; } /** - * @brief jobs returns any jobs exposed by this module. - * @return a list of jobs (can be empty). + * @brief isLoaded reports on the loaded status of a module. + * @return true if the module's loading phase has finished, otherwise false. */ - virtual JobList jobs() const = 0; + bool isLoaded() const { return m_loaded; } /** * @brief configurationMap returns the contents of the configuration file for @@ -168,6 +132,42 @@ public: */ QVariantMap configurationMap(); + /** + * @brief typeString returns a user-visible string for the module's type. + * @return the type string. + */ + QString typeString() const; + + /** + * @brief interface returns a user-visible string for the module's interface. + * @return the interface string. + */ + QString interfaceString() const; + + /** + * @brief loadSelf initialized the module. + * Subclasses must reimplement this depending on the module type and interface. + */ + virtual void loadSelf() = 0; + + /** + * @brief jobs returns any jobs exposed by this module. + * @return a list of jobs (can be empty). + */ + virtual JobList jobs() const = 0; + + /** + * @brief type returns the Type of this module object. + * @return the type enum value. + */ + virtual Type type() const = 0; + + /** + * @brief interface the Interface used by this module. + * @return the interface enum value. + */ + virtual Interface interface() const = 0; + /** * @brief Check the requirements of this module. */ @@ -175,12 +175,12 @@ public: protected: explicit Module(); - + /// @brief For subclasses to read their part of the descriptor virtual void initFrom( const QVariantMap& moduleDescriptor ) = 0; /// @brief Generic part of descriptor reading (and instance id) void initFrom( const QVariantMap& moduleDescriptor, const QString& id ); - + QVariantMap m_configurationMap; bool m_loaded = false;