From f38b518e861d154611461bf9740f0a7359082081 Mon Sep 17 00:00:00 2001 From: Adriaan de Groot Date: Fri, 30 Jul 2021 13:22:40 +0200 Subject: [PATCH 1/4] [keyboard] Factor out lambdas to regular slots - Long and complicated, nested, lambdas are not convenient for reasoning. - The debug messages from the innermost lambda have a totally useless function name, which makes debugging harder. --- src/modules/keyboard/Config.cpp | 94 ++++++++++++++++++--------------- src/modules/keyboard/Config.h | 12 +++++ 2 files changed, 64 insertions(+), 42 deletions(-) diff --git a/src/modules/keyboard/Config.cpp b/src/modules/keyboard/Config.cpp index d286e26fd..489f4a1ac 100644 --- a/src/modules/keyboard/Config.cpp +++ b/src/modules/keyboard/Config.cpp @@ -168,54 +168,64 @@ Config::Config( QObject* parent ) emit prettyStatusChanged(); } ); - connect( m_keyboardVariantsModel, &KeyboardVariantsModel::currentIndexChanged, [&]( int index ) { - // Set Xorg keyboard layout + variant - m_selectedVariant = m_keyboardVariantsModel->key( index ); - - if ( m_setxkbmapTimer.isActive() ) - { - m_setxkbmapTimer.stop(); - m_setxkbmapTimer.disconnect( this ); - } - - connect( &m_setxkbmapTimer, &QTimer::timeout, this, [=] { - m_additionalLayoutInfo = getAdditionalLayoutInfo( m_selectedLayout ); - - if ( !m_additionalLayoutInfo.additionalLayout.isEmpty() ) - { - m_additionalLayoutInfo.groupSwitcher = xkbmap_query_grp_option(); - - if ( m_additionalLayoutInfo.groupSwitcher.isEmpty() ) - { - m_additionalLayoutInfo.groupSwitcher = "grp:alt_shift_toggle"; - } - - QProcess::execute( "setxkbmap", - xkbmap_layout_args( { m_additionalLayoutInfo.additionalLayout, m_selectedLayout }, - { m_additionalLayoutInfo.additionalVariant, m_selectedVariant }, - m_additionalLayoutInfo.groupSwitcher ) ); - - - cDebug() << "xkbmap selection changed to: " << m_selectedLayout << '-' << m_selectedVariant << "(added " - << m_additionalLayoutInfo.additionalLayout << "-" << m_additionalLayoutInfo.additionalVariant - << " since current layout is not ASCII-capable)"; - } - else - { - QProcess::execute( "setxkbmap", xkbmap_layout_args( m_selectedLayout, m_selectedVariant ) ); - cDebug() << "xkbmap selection changed to: " << m_selectedLayout << '-' << m_selectedVariant; - } - m_setxkbmapTimer.disconnect( this ); - } ); - m_setxkbmapTimer.start( QApplication::keyboardInputInterval() ); - emit prettyStatusChanged(); - } ); + connect( m_keyboardVariantsModel, &KeyboardVariantsModel::currentIndexChanged, this, &Config::xkbChanged ); m_selectedModel = m_keyboardModelsModel->key( m_keyboardModelsModel->currentIndex() ); m_selectedLayout = m_keyboardLayoutsModel->item( m_keyboardLayoutsModel->currentIndex() ).first; m_selectedVariant = m_keyboardVariantsModel->key( m_keyboardVariantsModel->currentIndex() ); } +void +Config::xkbChanged( int index ) +{ + // Set Xorg keyboard layout + variant + m_selectedVariant = m_keyboardVariantsModel->key( index ); + + if ( m_setxkbmapTimer.isActive() ) + { + m_setxkbmapTimer.stop(); + m_setxkbmapTimer.disconnect( this ); + } + + connect( &m_setxkbmapTimer, &QTimer::timeout, this, &Config::xkbApply ); + + m_setxkbmapTimer.start( QApplication::keyboardInputInterval() ); + emit prettyStatusChanged(); +} + +void +Config::xkbApply() +{ + m_additionalLayoutInfo = getAdditionalLayoutInfo( m_selectedLayout ); + + if ( !m_additionalLayoutInfo.additionalLayout.isEmpty() ) + { + m_additionalLayoutInfo.groupSwitcher = xkbmap_query_grp_option(); + + if ( m_additionalLayoutInfo.groupSwitcher.isEmpty() ) + { + m_additionalLayoutInfo.groupSwitcher = "grp:alt_shift_toggle"; + } + + QProcess::execute( "setxkbmap", + xkbmap_layout_args( { m_additionalLayoutInfo.additionalLayout, m_selectedLayout }, + { m_additionalLayoutInfo.additionalVariant, m_selectedVariant }, + m_additionalLayoutInfo.groupSwitcher ) ); + + + cDebug() << "xkbmap selection changed to: " << m_selectedLayout << '-' << m_selectedVariant << "(added " + << m_additionalLayoutInfo.additionalLayout << "-" << m_additionalLayoutInfo.additionalVariant + << " since current layout is not ASCII-capable)"; + } + else + { + QProcess::execute( "setxkbmap", xkbmap_layout_args( m_selectedLayout, m_selectedVariant ) ); + cDebug() << "xkbmap selection changed to: " << m_selectedLayout << '-' << m_selectedVariant; + } + m_setxkbmapTimer.disconnect( this ); +} + + KeyboardModelsModel* Config::keyboardModels() const { diff --git a/src/modules/keyboard/Config.h b/src/modules/keyboard/Config.h index 90eeb0e7b..4919a154c 100644 --- a/src/modules/keyboard/Config.h +++ b/src/modules/keyboard/Config.h @@ -76,6 +76,18 @@ private: void guessLayout( const QStringList& langParts ); void updateVariants( const QPersistentModelIndex& currentItem, QString currentVariant = QString() ); + /* These two methods are used in tandem to apply changes to the + * keyboard layout. This introduces a slight delay between selecting + * a keyboard, and applying it to the system -- so that if you + * scroll through or down-arrow through the list of keyboards, + * you don't get buried under xkbset processes. + * + * xkbChanged() is called when the selection changes, and triggers + * a delayed call to xkbApply() which does the actual work. + */ + void xkbChanged( int index ); + void xkbApply(); + KeyboardModelsModel* m_keyboardModelsModel; KeyboardLayoutModel* m_keyboardLayoutsModel; KeyboardVariantsModel* m_keyboardVariantsModel; From 12b23db28613cad4eb2f0c1c4fe37c0fa2bcf0c0 Mon Sep 17 00:00:00 2001 From: Adriaan de Groot Date: Fri, 30 Jul 2021 22:17:26 +0200 Subject: [PATCH 2/4] [keyboard] Re-phrase API - expose only intended API, guessLayout() becomes internal and static - rename onActivate() since it was *called* for activation, but does something totally different. --- src/modules/keyboard/Config.cpp | 23 +++++++++---------- src/modules/keyboard/Config.h | 10 ++++---- src/modules/keyboard/KeyboardViewStep.cpp | 2 +- src/modules/keyboardq/KeyboardQmlViewStep.cpp | 2 +- 4 files changed, 19 insertions(+), 18 deletions(-) diff --git a/src/modules/keyboard/Config.cpp b/src/modules/keyboard/Config.cpp index 489f4a1ac..b137927b4 100644 --- a/src/modules/keyboard/Config.cpp +++ b/src/modules/keyboard/Config.cpp @@ -366,22 +366,22 @@ Config::createJobs() return list; } -void -Config::guessLayout( const QStringList& langParts ) +static void +guessLayout( const QStringList& langParts, KeyboardLayoutModel* layouts, KeyboardVariantsModel* variants ) { bool foundCountryPart = false; for ( auto countryPart = langParts.rbegin(); !foundCountryPart && countryPart != langParts.rend(); ++countryPart ) { cDebug() << Logger::SubEntry << "looking for locale part" << *countryPart; - for ( int i = 0; i < m_keyboardLayoutsModel->rowCount(); ++i ) + for ( int i = 0; i < layouts->rowCount(); ++i ) { - QModelIndex idx = m_keyboardLayoutsModel->index( i ); + QModelIndex idx = layouts->index( i ); QString name = idx.isValid() ? idx.data( KeyboardLayoutModel::KeyboardLayoutKeyRole ).toString() : QString(); if ( idx.isValid() && ( name.compare( *countryPart, Qt::CaseInsensitive ) == 0 ) ) { cDebug() << Logger::SubEntry << "matched" << name; - m_keyboardLayoutsModel->setCurrentIndex( i ); + layouts->setCurrentIndex( i ); foundCountryPart = true; break; } @@ -392,14 +392,13 @@ Config::guessLayout( const QStringList& langParts ) if ( countryPart != langParts.rend() ) { cDebug() << "Next level:" << *countryPart; - for ( int variantnumber = 0; variantnumber < m_keyboardVariantsModel->rowCount(); ++variantnumber ) + for ( int variantnumber = 0; variantnumber < variants->rowCount(); ++variantnumber ) { - if ( m_keyboardVariantsModel->key( variantnumber ).compare( *countryPart, Qt::CaseInsensitive ) - == 0 ) + if ( variants->key( variantnumber ).compare( *countryPart, Qt::CaseInsensitive ) == 0 ) { - m_keyboardVariantsModel->setCurrentIndex( variantnumber ); + variants->setCurrentIndex( variantnumber ); cDebug() << Logger::SubEntry << "matched variant" << *countryPart << ' ' - << m_keyboardVariantsModel->key( variantnumber ); + << variants->key( variantnumber ); } } } @@ -408,7 +407,7 @@ Config::guessLayout( const QStringList& langParts ) } void -Config::onActivate() +Config::guessLocaleKeyboardLayout() { /* Guessing a keyboard layout based on the locale means * mapping between language identifiers in _ @@ -495,7 +494,7 @@ Config::onActivate() QString country = QLocale::countryToString( QLocale( lang ).country() ); cDebug() << Logger::SubEntry << "extracted country" << country << "::" << langParts; - guessLayout( langParts ); + guessLayout( langParts, m_keyboardLayoutsModel, m_keyboardVariantsModel ); } } diff --git a/src/modules/keyboard/Config.h b/src/modules/keyboard/Config.h index 4919a154c..d4090bafc 100644 --- a/src/modules/keyboard/Config.h +++ b/src/modules/keyboard/Config.h @@ -32,16 +32,17 @@ class Config : public QObject public: Config( QObject* parent = nullptr ); + /// @brief Based on current xkb settings, pick a layout void detectCurrentKeyboardLayout(); + /// @brief Based on current locale, pick a layout + void guessLocaleKeyboardLayout(); Calamares::JobList createJobs(); QString prettyStatus() const; - void onActivate(); + /// @brief When leaving the page, write to GS void finalize(); - void setConfigurationMap( const QVariantMap& configurationMap ); - static AdditionalLayoutInfo getAdditionalLayoutInfo( const QString& layout ); /* A model is a physical configuration of a keyboard, e.g. 105-key PC @@ -69,11 +70,12 @@ public: */ void retranslate(); + void setConfigurationMap( const QVariantMap& configurationMap ); + signals: void prettyStatusChanged(); private: - void guessLayout( const QStringList& langParts ); void updateVariants( const QPersistentModelIndex& currentItem, QString currentVariant = QString() ); /* These two methods are used in tandem to apply changes to the diff --git a/src/modules/keyboard/KeyboardViewStep.cpp b/src/modules/keyboard/KeyboardViewStep.cpp index d1eb3eb68..029e1ae85 100644 --- a/src/modules/keyboard/KeyboardViewStep.cpp +++ b/src/modules/keyboard/KeyboardViewStep.cpp @@ -95,7 +95,7 @@ KeyboardViewStep::jobs() const void KeyboardViewStep::onActivate() { - m_config->onActivate(); + m_config->guessLocaleKeyboardLayout(); } diff --git a/src/modules/keyboardq/KeyboardQmlViewStep.cpp b/src/modules/keyboardq/KeyboardQmlViewStep.cpp index e8ae630e7..231d2a090 100644 --- a/src/modules/keyboardq/KeyboardQmlViewStep.cpp +++ b/src/modules/keyboardq/KeyboardQmlViewStep.cpp @@ -71,7 +71,7 @@ KeyboardQmlViewStep::jobs() const void KeyboardQmlViewStep::onActivate() { - m_config->onActivate(); + m_config->guessLocaleKeyboardLayout(); } void From a65723d4da1af40f4a790b689b8a95a5748a892e Mon Sep 17 00:00:00 2001 From: Adriaan de Groot Date: Sat, 31 Jul 2021 00:20:27 +0200 Subject: [PATCH 3/4] [libcalamares] Extend cPointerSetter with initial-value This is a convenience for "set to at end of scope". --- src/libcalamares/utils/RAII.h | 39 +++++++++++++++++++++++++++++++---- 1 file changed, 35 insertions(+), 4 deletions(-) diff --git a/src/libcalamares/utils/RAII.h b/src/libcalamares/utils/RAII.h index 0b8c9b2aa..00e276ec6 100644 --- a/src/libcalamares/utils/RAII.h +++ b/src/libcalamares/utils/RAII.h @@ -70,11 +70,42 @@ struct cPointerSetter std::optional< T > m_value; T* m_pointer; - cPointerSetter( T* p ) : m_pointer(p) {} - ~cPointerSetter() { if ( m_pointer && m_value.has_value() ) { *m_pointer = m_value.value(); } } + /** @brief Create a setter with no value set + * + * Until a value is set via operator=(), this pointer-setter + * will do nothing on destruction, leaving the pointed-to + * value unchanged. + */ + cPointerSetter( T* p ) + : m_pointer( p ) + { + } + /** @brief Create a setter with a value already set + * + * This ensures that on destruction, the value @p v will be written; + * it is equivalent to assigning @p v immediately. The pointed-to + * value is **not** changed (until destruction). + */ + cPointerSetter( T* p, T v ) + : m_value( v ) + , m_pointer( p ) + { + } + ~cPointerSetter() + { + if ( m_pointer && m_value.has_value() ) + { + *m_pointer = m_value.value(); + } + } - const T& operator=(const T& v) { m_value = v; return v; } + const T& operator=( const T& v ) + { + m_value = v; + return v; + } }; -template < typename T > cPointerSetter( T p ) -> cPointerSetter; +template < typename T > +cPointerSetter( T p )->cPointerSetter< decltype( *p ) >; #endif From 2b485a5e5951dfea1b47bf8e142b054a748dac85 Mon Sep 17 00:00:00 2001 From: Adriaan de Groot Date: Sat, 31 Jul 2021 00:24:10 +0200 Subject: [PATCH 4/4] [keyboard] Only guess layouts until the user picks one - when activating the page, the "guess" functions do their work and afterwards the config is left in a "guessable" state, but if the user makes a specific choice, then the config leaves the "guessable" state and the user's explicit choice is preserved. FIXES #1744 --- src/modules/keyboard/Config.cpp | 30 ++++++++++++++++++++++++++++++ src/modules/keyboard/Config.h | 18 ++++++++++++++++++ 2 files changed, 48 insertions(+) diff --git a/src/modules/keyboard/Config.cpp b/src/modules/keyboard/Config.cpp index b137927b4..b77282a18 100644 --- a/src/modules/keyboard/Config.cpp +++ b/src/modules/keyboard/Config.cpp @@ -16,6 +16,7 @@ #include "GlobalStorage.h" #include "JobQueue.h" #include "utils/Logger.h" +#include "utils/RAII.h" #include "utils/Retranslator.h" #include "utils/String.h" #include "utils/Variant.h" @@ -170,6 +171,12 @@ Config::Config( QObject* parent ) connect( m_keyboardVariantsModel, &KeyboardVariantsModel::currentIndexChanged, this, &Config::xkbChanged ); + // If the user picks something explicitly -- not a consequence of + // a guess -- then move to UserSelected state and stay there. + connect( m_keyboardModelsModel, &KeyboardModelsModel::currentIndexChanged, this, &Config::selectionChange ); + connect( m_keyboardLayoutsModel, &KeyboardLayoutModel::currentIndexChanged, this, &Config::selectionChange ); + connect( m_keyboardVariantsModel, &KeyboardVariantsModel::currentIndexChanged, this, &Config::selectionChange ); + m_selectedModel = m_keyboardModelsModel->key( m_keyboardModelsModel->currentIndex() ); m_selectedLayout = m_keyboardLayoutsModel->item( m_keyboardLayoutsModel->currentIndex() ).first; m_selectedVariant = m_keyboardVariantsModel->key( m_keyboardVariantsModel->currentIndex() ); @@ -264,6 +271,13 @@ findLayout( const KeyboardLayoutModel* klm, const QString& currentLayout ) void Config::detectCurrentKeyboardLayout() { + if ( m_state != State::Initial ) + { + return; + } + cPointerSetter returnToIntial( &m_state, State::Initial ); + m_state = State::Guessing; + //### Detect current keyboard layout and variant QString currentLayout; QString currentVariant; @@ -409,6 +423,13 @@ guessLayout( const QStringList& langParts, KeyboardLayoutModel* layouts, Keyboar void Config::guessLocaleKeyboardLayout() { + if ( m_state != State::Initial ) + { + return; + } + cPointerSetter returnToIntial( &m_state, State::Initial ); + m_state = State::Guessing; + /* Guessing a keyboard layout based on the locale means * mapping between language identifiers in _ * format to keyboard mappings, which are _ @@ -556,3 +577,12 @@ Config::retranslate() { retranslateKeyboardModels(); } + +void +Config::selectionChange() +{ + if ( m_state == State::Initial ) + { + m_state = State::UserSelected; + } +} diff --git a/src/modules/keyboard/Config.h b/src/modules/keyboard/Config.h index d4090bafc..436ead4b5 100644 --- a/src/modules/keyboard/Config.h +++ b/src/modules/keyboard/Config.h @@ -107,6 +107,24 @@ private: QString m_xOrgConfFileName; QString m_convertedKeymapPath; bool m_writeEtcDefaultKeyboard = true; + + // The state determines whether we guess settings or preserve them: + // - Initial -> Guessing + // - Initial -> UserSelected + // - Guessing -> Initial + enum class State + { + Initial, // after configuration, nothing special going on + Guessing, // on activation + UserSelected // explicit choice is made, preserve that + }; + State m_state = State::Initial; + + /** @brief Handles state change when selections in model, variant, layout + * + * This handles the Initial -> UserSelected transition in particular. + */ + void selectionChange(); };