Merge branch 'issue-1744' into calamares

FIXES #1744

CHANGES

@@ -22,6 +22,9 @@ This release contains contributions from (alphabetically by first name):
    no further action is needed.
 
 ## Modules ##
+ - When the *keyboard* module is activated, it no longer replaces
+   an explicit user choice (e.g. for a Belgian layout) by a guessed-for-
+   this-language layout (e.g. Danish if you're installing in Danish).
  - Logic for handling installation lists has been moved around in the
    *packages* module so that package managers can, in principle,
    adjust how to handle critical and non-critical package lists.

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) {}
+    /** @brief Create a setter with no value set
-    ~cPointerSetter() { if ( m_pointer && m_value.has_value() ) { *m_pointer = m_value.value(); } }
+     *
+     * 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<decltype(*p)>;
+template < typename T >
+cPointerSetter( T p )->cPointerSetter< decltype( *p ) >;
 #endif

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"
@@ -168,54 +169,70 @@ Config::Config( QObject* parent )
         emit prettyStatusChanged();
     } );
 
-    connect( m_keyboardVariantsModel, &KeyboardVariantsModel::currentIndexChanged, [&]( int index ) {
+    connect( m_keyboardVariantsModel, &KeyboardVariantsModel::currentIndexChanged, this, &Config::xkbChanged );
-        // Set Xorg keyboard layout + variant
-        m_selectedVariant = m_keyboardVariantsModel->key( index );
 
-        if ( m_setxkbmapTimer.isActive() )
+    // If the user picks something explicitly -- not a consequence of
-        {
+    // a guess -- then move to UserSelected state and stay there.
-            m_setxkbmapTimer.stop();
+    connect( m_keyboardModelsModel, &KeyboardModelsModel::currentIndexChanged, this, &Config::selectionChange );
-            m_setxkbmapTimer.disconnect( this );
+    connect( m_keyboardLayoutsModel, &KeyboardLayoutModel::currentIndexChanged, this, &Config::selectionChange );
-        }
+    connect( m_keyboardVariantsModel, &KeyboardVariantsModel::currentIndexChanged, this, &Config::selectionChange );
-
-        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();
-    } );
 
     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
 {
@@ -254,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;
@@ -356,22 +380,22 @@ Config::createJobs()
     return list;
 }
 
-void
+static void
-Config::guessLayout( const QStringList& langParts )
+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;
             }
@@ -382,14 +406,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 )
+                    if ( variants->key( variantnumber ).compare( *countryPart, Qt::CaseInsensitive ) == 0 )
-                         == 0 )
                     {
-                        m_keyboardVariantsModel->setCurrentIndex( variantnumber );
+                        variants->setCurrentIndex( variantnumber );
                         cDebug() << Logger::SubEntry << "matched variant" << *countryPart << ' '
-                                 << m_keyboardVariantsModel->key( variantnumber );
+                                 << variants->key( variantnumber );
                     }
                 }
             }
@@ -398,8 +421,15 @@ Config::guessLayout( const QStringList& langParts )
 }
 
 void
-Config::onActivate()
+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 <lang>_<country>
      * format to keyboard mappings, which are <country>_<layout>
@@ -485,7 +515,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 );
     }
 }
 
@@ -547,3 +577,12 @@ Config::retranslate()
 {
     retranslateKeyboardModels();
 }
+
+void
+Config::selectionChange()
+{
+    if ( m_state == State::Initial )
+    {
+        m_state = State::UserSelected;
+    }
+}

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,13 +70,26 @@ 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
+     * 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;
@@ -93,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();
 };
 
 

src/modules/keyboard/KeyboardViewStep.cpp

@@ -95,7 +95,7 @@ KeyboardViewStep::jobs() const
 void
 KeyboardViewStep::onActivate()
 {
-    m_config->onActivate();
+    m_config->guessLocaleKeyboardLayout();
 }
 
 

src/modules/keyboardq/KeyboardQmlViewStep.cpp

@@ -71,7 +71,7 @@ KeyboardQmlViewStep::jobs() const
 void
 KeyboardQmlViewStep::onActivate()
 {
-    m_config->onActivate();
+    m_config->guessLocaleKeyboardLayout();
 }
 
 void