[mobile] make sshd feature optional and related navigation refactoring #6
Conversation
Organize the screens into features and replace all navTo() calls with hardcoded screen names with new navNext() and navNextFeature() functions.
Skip whole features if a featureSshd (example for feature "sshd") or smilar config key exists and is set to false.
Add featureSshd config key to hide related UI screens and skip related logic in UsersJob.
| @@ -19,6 +19,16 @@ cfgStr( const QVariantMap& cfgMap, QString key, QString defaultStr ) | |||
| return ret; | |||
| } | |||
|
|
|||
| bool | |||
| cfgBool( const QVariantMap& cfgMap, QString key, bool defaultBool ) | |||
There was a problem hiding this comment.
From utils/variant.h:
/**
* Get a bool value from a mapping with a given key; returns @p d if no value.
*/
DLLEXPORT bool getBool( const QVariantMap& map, const QString& key, bool d = false );
You should be able to use that instead (this just seems like unnecessary code duplication)
There was a problem hiding this comment.
(Also makes me think that cfgStr() is probably CalamaresUtils::getString() )
| @@ -54,7 +54,8 @@ MobileQmlViewStep::onLeave() | |||
| /* Put users job in queue (should run after unpackfs) */ | |||
| m_jobs.clear(); | |||
| QString cmdSshd = m_config->isSshEnabled() ? m_config->cmdSshdEnable() : m_config->cmdSshdDisable(); | |||
| users = new UsersJob( m_config->cmdPasswd(), | |||
| users = new UsersJob( m_config->featureSshd(), | |||
| m_config->cmdPasswd(), | |||
There was a problem hiding this comment.
This is a slightly weird construction that we're trying to drop from other view modules: why does the view module build its jobs in the view module, and, weirder still, when leaving the page? Most other modules are moving this to a createJobs() function in the Config object, which is called by the jobs() function of the ViewModule if needed.
But .. not directly related to your PR.
| @@ -8,7 +8,8 @@ class UsersJob : public Calamares::Job | |||
| { | |||
| Q_OBJECT | |||
| public: | |||
| UsersJob( QString cmdPasswd, | |||
| UsersJob( bool featureSshd, | |||
| QString cmdPasswd, | |||
There was a problem hiding this comment.
Idiomatic is const QString&, but that should have been spotted previously.
| @@ -23,6 +24,7 @@ class UsersJob : public Calamares::Job | |||
| Calamares::JobList createJobs(); | |||
|
|
|||
| private: | |||
| bool m_featureSshd; | |||
There was a problem hiding this comment.
Idiomatic -- well, in C++ code that cares for memory size, which we dont -- would be to sort things by size, putting like-sized things together for optimal packing. You might consider a C++17 style initializer here, too, for completeness (e.g. bool m_featureSshd = true;)
There was a problem hiding this comment.
Thanks for the suggestion! I'll leave the = true default out though, because then it would be defined in yet another place. It is already in two places, the config (mobile.conf) and Config.cpp, where m_* gets filled from the config.
I've applied your other suggestions though, will make a follow-up PR soon.
| @@ -122,6 +145,7 @@ Page | |||
|
|
|||
| /* Navigation related */ | |||
| function navTo(name, historyPush=true) { | |||
| console.log("Navigating to screen: " + name); | |||
There was a problem hiding this comment.
Misplaced debugging? Should we introduce something for QML to explicitly log to the Calamares log file?
There was a problem hiding this comment.
No, that's on purpose. My follow up PR will introduce a few more log calls, so maybe the explicit log to calamares log file makes sense. The console.log messages are printed in the log file currently (but you can't control the verbosity, which is probably why you suggested to introduce a new thing).
|
Merged, but you can tell from the comments there's a bunch of things that can be done to tighten up the C++ code. They're not necessary, but good future cleanup. |
Don't create the user's job in MobileQmlViewStep::onLeave() anymore. Instead, have Config::createJobs() and call that from MobileQmlViewStep::jobs(), as suggested by Adriaan, and that's also how it's done in the keyboard module in calamares.git. This allows getting rid of m_jobs, and makes it less awkward to pass parameters to the job. Related: calamares#6 (comment)
Don't create the user's job in MobileQmlViewStep::onLeave() anymore. Instead, have Config::createJobs() and call that from MobileQmlViewStep::jobs(), as suggested by Adriaan, and that's also how it's done in the keyboard module in calamares.git. This allows getting rid of m_jobs, and makes it less awkward to pass parameters to the job. Related: calamares#6 (comment)
Some distributions need the sshd feature disabled for now, so make it optional. Refactor the navigation code so we won't get into trouble when making other (possibly new) features optional, too. See commit messages for details.