Merge pull request #1802 from calamares/issue-1801

[summary] Use model shared with *summaryq*

FIXES #1801

CHANGES

@@ -18,6 +18,8 @@ This release contains contributions from (alphabetically by first name):
 ## Modules ##
  - The *partition* module now consistently uses the configured EFI
    partition size (defaults to 300MiB).
+ - Internal changes in the *summary* module to increase consistency
+   between *summary* and *summaryq*.
 
 
 # 3.2.44.2 (2021-09-27) #

src/modules/summary/Config.cpp

@@ -26,7 +26,9 @@ SummaryModel::SummaryModel( QObject* parent )
 QHash< int, QByteArray >
 SummaryModel::roleNames() const
 {
-    return { { Qt::DisplayRole, "title" }, { Qt::UserRole, "message" } };
+    // Not including WidgetRole here because that wouldn't make sense
+    // in a QML context which is where the roleNames are important.
+    return { { TitleRole, "title" }, { MessageRole, "message" } };
 }
 
 QVariant
@@ -36,8 +38,18 @@ SummaryModel::data( const QModelIndex& index, int role ) const
     {
         return QVariant();
     }
-    const auto item = m_summary.at( index.row() );
-    return role == Qt::DisplayRole ? item.title : item.message;
+    auto& item = m_summary.at( index.row() );
+    switch ( role )
+    {
+    case TitleRole:
+        return item.title;
+    case MessageRole:
+        return item.message;
+    case WidgetRole:
+        return item.widget ? QVariant::fromValue( item.widget ) : QVariant();
+    default:
+        return QVariant();
+    }
 }
 
 int
@@ -84,19 +96,19 @@ Config::retranslate()
     if ( Calamares::Settings::instance()->isSetupMode() )
     {
         m_message = tr( "This is an overview of what will happen once you start "
-                          "the setup procedure." );
+                        "the setup procedure." );
     }
     else
     {
         m_message = tr( "This is an overview of what will happen once you start "
-                          "the install procedure." );
+                        "the install procedure." );
     }
     Q_EMIT titleChanged( m_title );
     Q_EMIT messageChanged( m_message );
 }
 
-void
-Config::collectSummaries( const Calamares::ViewStep* upToHere )
+Calamares::ViewStepList
+Config::stepsForSummary( const Calamares::ViewStep* upToHere )
 {
     Calamares::ViewStepList steps;
     for ( Calamares::ViewStep* step : Calamares::ViewManager::instance()->viewSteps() )
@@ -122,6 +134,18 @@ Config::collectSummaries( const Calamares::ViewStep* upToHere )
 
         steps.append( step );
     }
-
-    m_summary->setSummaryList( steps );
+    return steps;
+}
+
+
+void
+Config::collectSummaries( const Calamares::ViewStep* upToHere, Widgets withWidgets )
+{
+    m_summary->setSummaryList( stepsForSummary( upToHere ), withWidgets == Widgets::Enabled );
+}
+
+void
+Config::clearSummaries()
+{
+    m_summary->setSummaryList( {}, false );
 }

src/modules/summary/Config.h

@@ -38,6 +38,13 @@ class SummaryModel : public QAbstractListModel
     friend class Config;
 
 public:
+    enum Roles : int
+    {
+        TitleRole = Qt::DisplayRole,  // Name of the step
+        MessageRole = Qt::UserRole,  // String saying what it will do
+        WidgetRole,  // Pointer to widget
+    };
+
     explicit SummaryModel( QObject* parent = nullptr );
     int rowCount( const QModelIndex& = QModelIndex() ) const override;
     QVariant data( const QModelIndex& index, int role ) const override;
@@ -72,8 +79,19 @@ class Config : public QObject
 public:
     explicit Config( QObject* parent = nullptr );
 
+    ///@brief Include widgets in the model?
+    enum class Widgets
+    {
+        Disabled,
+        Enabled
+    };
+
+    static Calamares::ViewStepList stepsForSummary( const Calamares::ViewStep* upToHere );
+
     ///@brief Called later, to load the model once all viewsteps are there
-    void collectSummaries( const Calamares::ViewStep* upToHere );
+    void collectSummaries( const Calamares::ViewStep* upToHere, Widgets withWidgets );
+    ///@brief Clear the model of steps (to avoid dangling widgets)
+    void clearSummaries();
 
     QAbstractListModel* summaryModel() const { return m_summary; }
 
@@ -81,7 +99,6 @@ public:
     QString message() const { return m_message; }
 
 private:
-    Calamares::ViewStepList stepsForSummary( const Calamares::ViewStepList& allSteps ) const;
     void retranslate();
 
     SummaryModel* m_summary;

src/modules/summary/SummaryPage.cpp

@@ -27,18 +27,15 @@
 #include <QLabel>
 #include <QScrollArea>
 
-SummaryPage::SummaryPage( Config* config, const SummaryViewStep* thisViewStep, QWidget* parent )
+SummaryPage::SummaryPage( Config* config, QWidget* parent )
     : QWidget()
-    , m_thisViewStep( thisViewStep )
     , m_contentWidget( nullptr )
     , m_scrollArea( new QScrollArea( this ) )
 {
     Q_UNUSED( parent )
 
-
     this->setObjectName( "summaryStep" );
 
-    Q_ASSERT( m_thisViewStep );
     QVBoxLayout* layout = new QVBoxLayout( this );
     layout->setContentsMargins( 0, 0, 0, 0 );
 
@@ -82,10 +79,52 @@ createBodyLabel( const QString& text, const QPalette& bodyPalette )
     return label;
 }
 
+static QWidget*
+createStepWidget( const QString& description, QWidget* innerWidget, const QPalette& palette )
+{
+    QWidget* w = new QWidget();
+    QHBoxLayout* itemBodyLayout = new QHBoxLayout;
+    w->setLayout( itemBodyLayout );
+
+    // Indent the inner box by a bit
+    itemBodyLayout->addSpacing( CalamaresUtils::defaultFontHeight() * 2 );
+    QVBoxLayout* itemBodyCoreLayout = new QVBoxLayout;
+    itemBodyLayout->addLayout( itemBodyCoreLayout );
+    CalamaresUtils::unmarginLayout( itemBodyLayout );
+
+    itemBodyCoreLayout->addSpacing( CalamaresUtils::defaultFontHeight() / 2 );
+    if ( !description.isEmpty() )
+    {
+        itemBodyCoreLayout->addWidget( createBodyLabel( description, palette ) );
+    }
+    if ( innerWidget )
+    {
+        itemBodyCoreLayout->addWidget( innerWidget );
+    }
+
+    return w;
+}
+
+static void
+ensureSize( QWidget* parent, QScrollArea* container, Calamares::ViewStep* viewstep )
+{
+    auto summarySize = container->widget()->sizeHint();
+    if ( summarySize.height() > container->size().height() )
+    {
+        auto enlarge = 2 + summarySize.height() - container->size().height();
+        auto widgetSize = parent->size();
+        widgetSize.setHeight( widgetSize.height() + enlarge );
+
+        cDebug() << "Summary widget is larger than viewport, enlarge by" << enlarge << "to" << widgetSize;
+
+        emit viewstep->ensureSize( widgetSize );  // Only expand height
+    }
+}
+
 // Adds a widget for those ViewSteps that want a summary;
 // see SummaryPage documentation and also ViewStep docs.
 void
-SummaryPage::onActivate()
+SummaryPage::buildWidgets( Config* config, SummaryViewStep* viewstep )
 {
     const int SECTION_SPACING = 12;
 
@@ -101,91 +140,37 @@ SummaryPage::onActivate()
     QPalette bodyPalette( palette() );
     bodyPalette.setColor( WindowBackground, palette().window().color().lighter( 108 ) );
 
-    bool first = true;
-    const Calamares::ViewStepList steps = stepsForSummary( Calamares::ViewManager::instance()->viewSteps() );
+    const auto* model = config->summaryModel();
+    const auto rowCount = model->rowCount();
 
-    for ( Calamares::ViewStep* step : steps )
+    for ( int row = 0; row < rowCount; row++ )
     {
-        QString text = step->prettyStatus();
-        QWidget* widget = step->createSummaryWidget();
+        const auto rowIndex = model->index( row );
+        QString title = model->data( rowIndex, SummaryModel::TitleRole ).toString();
+        QString text = model->data( rowIndex, SummaryModel::MessageRole ).toString();
+        QWidget* widget = model->data( rowIndex, SummaryModel::WidgetRole ).value< QWidget* >();
 
         if ( text.isEmpty() && !widget )
         {
             continue;
         }
 
-        if ( !first )
+        if ( row > 0 )
         {
             m_layout->addSpacing( SECTION_SPACING );
         }
-        first = false;
 
-        m_layout->addWidget( createTitleLabel( step->prettyName(), titleFont ) );
-        QHBoxLayout* itemBodyLayout = new QHBoxLayout;
-        m_layout->addSpacing( CalamaresUtils::defaultFontHeight() / 2 );
-        m_layout->addLayout( itemBodyLayout );
-        itemBodyLayout->addSpacing( CalamaresUtils::defaultFontHeight() * 2 );
-        QVBoxLayout* itemBodyCoreLayout = new QVBoxLayout;
-        itemBodyLayout->addLayout( itemBodyCoreLayout );
-        CalamaresUtils::unmarginLayout( itemBodyLayout );
-        if ( !text.isEmpty() )
-        {
-            itemBodyCoreLayout->addWidget( createBodyLabel( text, bodyPalette ) );
-        }
-        if ( widget )
-        {
-            itemBodyCoreLayout->addWidget( widget );
-        }
-        itemBodyLayout->addSpacing( CalamaresUtils::defaultFontHeight() * 2 );
+        m_layout->addWidget( createTitleLabel( title, titleFont ) );
+        m_layout->addWidget( createStepWidget( text, widget, bodyPalette ) );
     }
     m_layout->addStretch();
 
     m_scrollArea->setWidget( m_contentWidget );
-
-    auto summarySize = m_contentWidget->sizeHint();
-    if ( summarySize.height() > m_scrollArea->size().height() )
-    {
-        auto enlarge = 2 + summarySize.height() - m_scrollArea->size().height();
-        auto widgetSize = this->size();
-        widgetSize.setHeight( widgetSize.height() + enlarge );
-
-        cDebug() << "Summary widget is larger than viewport, enlarge by" << enlarge << "to" << widgetSize;
-
-        emit m_thisViewStep->ensureSize( widgetSize );  // Only expand height
-    }
-}
-
-Calamares::ViewStepList
-SummaryPage::stepsForSummary( const Calamares::ViewStepList& allSteps ) const
-{
-    Calamares::ViewStepList steps;
-    for ( Calamares::ViewStep* step : allSteps )
-    {
-        // We start from the beginning of the complete steps list. If we encounter any
-        // ExecutionViewStep, it means there was an execution phase in the past, and any
-        // jobs from before that phase were already executed, so we can safely clear the
-        // list of steps to summarize and start collecting from scratch.
-        if ( qobject_cast< Calamares::ExecutionViewStep* >( step ) )
-        {
-            steps.clear();
-            continue;
-        }
-
-        // If we reach the parent step of this page, we're done collecting the list of
-        // steps to summarize.
-        if ( m_thisViewStep == step )
-        {
-            break;
-        }
-
-        steps.append( step );
-    }
-
-    return steps;
+    ensureSize( this, m_scrollArea, viewstep );
 }
 
 void
-SummaryPage::onLeave()
+SummaryPage::cleanup()
 {
     delete m_contentWidget;
     m_contentWidget = nullptr;

src/modules/summary/SummaryPage.h

@@ -45,22 +45,17 @@ class SummaryPage : public QWidget
 {
     Q_OBJECT
 public:
-    explicit SummaryPage( Config* config, const SummaryViewStep* thisViewStep, QWidget* parent = nullptr );
+    explicit SummaryPage( Config* config, QWidget* parent = nullptr );
 
     /// @brief Create contents showing all of the summary
-    void onActivate();
+    void buildWidgets( Config* config, SummaryViewStep* viewstep );
     /// @brief Clean up the widgets
-    void onLeave();
+    void cleanup();
 
 private:
-    Calamares::ViewStepList stepsForSummary( const Calamares::ViewStepList& allSteps ) const;
-
-    const SummaryViewStep* m_thisViewStep;
-
     QVBoxLayout* m_layout = nullptr;
     QWidget* m_contentWidget = nullptr;
-
-    QScrollArea* m_scrollArea;
+    QScrollArea* m_scrollArea = nullptr;
 };
 
 #endif  // SUMMARYPAGE_H

src/modules/summary/SummaryViewStep.cpp

@@ -16,7 +16,7 @@ CALAMARES_PLUGIN_FACTORY_DEFINITION( SummaryViewStepFactory, registerPlugin< Sum
 SummaryViewStep::SummaryViewStep( QObject* parent )
     : Calamares::ViewStep( parent )
     , m_config( new Config( this ) )
-    , m_widget( new SummaryPage( m_config, this ) )
+    , m_widget( new SummaryPage( m_config ) )
 {
     emit nextStatusChanged( true );
 }
@@ -84,13 +84,14 @@ SummaryViewStep::jobs() const
 void
 SummaryViewStep::onActivate()
 {
-    m_config->collectSummaries( this );
-    m_widget->onActivate();
+    m_config->collectSummaries( this, Config::Widgets::Enabled );
+    m_widget->buildWidgets( m_config, this );
 }
 
 
 void
 SummaryViewStep::onLeave()
 {
-    m_widget->onLeave();
+    m_config->clearSummaries();
+    m_widget->cleanup();
 }

src/modules/summaryq/SummaryQmlViewStep.cpp

@@ -69,5 +69,5 @@ SummaryQmlViewStep::onActivate()
 {
     // Collect the steps before this one: those need to have their
     // summary (text or widget) displayed.
-    m_config->collectSummaries( this );
+    m_config->collectSummaries( this, Config::Widgets::Disabled );
 }