From 17eb3f5e33e35e778d2064fd615101a8bd4b5ae1 Mon Sep 17 00:00:00 2001 From: Adriaan de Groot Date: Tue, 18 Feb 2020 11:02:53 +0100 Subject: [PATCH 1/8] [netinstall] Apply coding style --- src/modules/netinstall/NetInstallViewStep.cpp | 19 ++- src/modules/netinstall/NetInstallViewStep.h | 2 +- src/modules/netinstall/PackageModel.cpp | 151 ++++++++++++------ src/modules/netinstall/PackageModel.h | 21 ++- src/modules/netinstall/PackageTreeItem.cpp | 55 +++++-- src/modules/netinstall/PackageTreeItem.h | 9 +- 6 files changed, 167 insertions(+), 90 deletions(-) diff --git a/src/modules/netinstall/NetInstallViewStep.cpp b/src/modules/netinstall/NetInstallViewStep.cpp index fcb76be5a..94efb5f86 100644 --- a/src/modules/netinstall/NetInstallViewStep.cpp +++ b/src/modules/netinstall/NetInstallViewStep.cpp @@ -20,15 +20,15 @@ #include "NetInstallViewStep.h" -#include "JobQueue.h" #include "GlobalStorage.h" +#include "JobQueue.h" #include "utils/Logger.h" #include "utils/Variant.h" #include "NetInstallPage.h" -CALAMARES_PLUGIN_FACTORY_DEFINITION( NetInstallViewStepFactory, registerPlugin(); ) +CALAMARES_PLUGIN_FACTORY_DEFINITION( NetInstallViewStepFactory, registerPlugin< NetInstallViewStep >(); ) NetInstallViewStep::NetInstallViewStep( QObject* parent ) : Calamares::ViewStep( parent ) @@ -36,15 +36,16 @@ NetInstallViewStep::NetInstallViewStep( QObject* parent ) , m_nextEnabled( false ) { emit nextStatusChanged( true ); - connect( m_widget, &NetInstallPage::checkReady, - this, &NetInstallViewStep::nextIsReady ); + connect( m_widget, &NetInstallPage::checkReady, this, &NetInstallViewStep::nextIsReady ); } NetInstallViewStep::~NetInstallViewStep() { if ( m_widget && m_widget->parent() == nullptr ) + { m_widget->deleteLater(); + } } @@ -131,28 +132,32 @@ NetInstallViewStep::onLeave() // with the more complicated datastructure. if ( !package.preScript.isEmpty() || !package.postScript.isEmpty() ) { - QMap sdetails; + QMap< QString, QVariant > sdetails; sdetails.insert( "pre-script", package.preScript ); sdetails.insert( "package", package.packageName ); sdetails.insert( "post-script", package.postScript ); details = sdetails; } if ( package.isCritical ) + { installPackages.append( details ); + } else + { tryInstallPackages.append( details ); + } } if ( !installPackages.empty() ) { - QMap op; + QMap< QString, QVariant > op; op.insert( "install", QVariant( installPackages ) ); packageOperations.append( op ); cDebug() << Logger::SubEntry << installPackages.length() << "critical packages."; } if ( !tryInstallPackages.empty() ) { - QMap op; + QMap< QString, QVariant > op; op.insert( "try_install", QVariant( tryInstallPackages ) ); packageOperations.append( op ); cDebug() << Logger::SubEntry << tryInstallPackages.length() << "non-critical packages."; diff --git a/src/modules/netinstall/NetInstallViewStep.h b/src/modules/netinstall/NetInstallViewStep.h index f26ba74ed..be92f5cc3 100644 --- a/src/modules/netinstall/NetInstallViewStep.h +++ b/src/modules/netinstall/NetInstallViewStep.h @@ -70,4 +70,4 @@ private: CALAMARES_PLUGIN_FACTORY_DECLARATION( NetInstallViewStepFactory ) -#endif // NETINSTALLVIEWSTEP_H +#endif // NETINSTALLVIEWSTEP_H diff --git a/src/modules/netinstall/PackageModel.cpp b/src/modules/netinstall/PackageModel.cpp index 2fde7695e..331586869 100644 --- a/src/modules/netinstall/PackageModel.cpp +++ b/src/modules/netinstall/PackageModel.cpp @@ -21,9 +21,9 @@ #include "utils/Yaml.h" -PackageModel::PackageModel( const YAML::Node& data, QObject* parent ) : - QAbstractItemModel( parent ), - m_columnHeadings() +PackageModel::PackageModel( const YAML::Node& data, QObject* parent ) + : QAbstractItemModel( parent ) + , m_columnHeadings() { m_rootItem = new PackageTreeItem(); setupModelData( data, m_rootItem ); @@ -38,33 +38,47 @@ QModelIndex PackageModel::index( int row, int column, const QModelIndex& parent ) const { if ( !hasIndex( row, column, parent ) ) + { return QModelIndex(); + } PackageTreeItem* parentItem; if ( !parent.isValid() ) + { parentItem = m_rootItem; + } else - parentItem = static_cast( parent.internalPointer() ); + { + parentItem = static_cast< PackageTreeItem* >( parent.internalPointer() ); + } PackageTreeItem* childItem = parentItem->child( row ); if ( childItem ) + { return createIndex( row, column, childItem ); + } else + { return QModelIndex(); + } } QModelIndex PackageModel::parent( const QModelIndex& index ) const { if ( !index.isValid() ) + { return QModelIndex(); + } - PackageTreeItem* child = static_cast( index.internalPointer() ); + PackageTreeItem* child = static_cast< PackageTreeItem* >( index.internalPointer() ); PackageTreeItem* parent = child->parentItem(); if ( parent == m_rootItem ) + { return QModelIndex(); + } return createIndex( parent->row(), 0, parent ); } @@ -72,13 +86,19 @@ int PackageModel::rowCount( const QModelIndex& parent ) const { if ( parent.column() > 0 ) + { return 0; + } PackageTreeItem* parentItem; if ( !parent.isValid() ) + { parentItem = m_rootItem; + } else - parentItem = static_cast( parent.internalPointer() ); + { + parentItem = static_cast< PackageTreeItem* >( parent.internalPointer() ); + } return parentItem->childCount(); } @@ -87,7 +107,9 @@ int PackageModel::columnCount( const QModelIndex& parent ) const { if ( parent.isValid() ) - return static_cast( parent.internalPointer() )->columnCount(); + { + return static_cast< PackageTreeItem* >( parent.internalPointer() )->columnCount(); + } return m_rootItem->columnCount(); } @@ -95,17 +117,25 @@ QVariant PackageModel::data( const QModelIndex& index, int role ) const { if ( !index.isValid() ) + { return QVariant(); + } - PackageTreeItem* item = static_cast( index.internalPointer() ); + PackageTreeItem* item = static_cast< PackageTreeItem* >( index.internalPointer() ); if ( index.column() == 0 && role == Qt::CheckStateRole ) + { return item->isSelected(); + } - if ( item->isHidden() && role == Qt::DisplayRole ) // Hidden group + if ( item->isHidden() && role == Qt::DisplayRole ) // Hidden group + { return QVariant(); + } if ( role == Qt::DisplayRole ) + { return item->data( index.column() ); + } return QVariant(); } @@ -114,27 +144,31 @@ PackageModel::setData( const QModelIndex& index, const QVariant& value, int role { if ( role == Qt::CheckStateRole && index.isValid() ) { - PackageTreeItem* item = static_cast( index.internalPointer() ); - item->setSelected( static_cast( value.toInt() ) ); + PackageTreeItem* item = static_cast< PackageTreeItem* >( index.internalPointer() ); + item->setSelected( static_cast< Qt::CheckState >( value.toInt() ) ); - emit dataChanged( this->index( 0, 0 ), index.sibling( index.column(), index.row() + 1 ), - QVector( Qt::CheckStateRole ) ); + emit dataChanged( this->index( 0, 0 ), + index.sibling( index.column(), index.row() + 1 ), + QVector< int >( Qt::CheckStateRole ) ); } return true; } bool -PackageModel::setHeaderData( int section, Qt::Orientation orientation, - const QVariant& value, int role ) +PackageModel::setHeaderData( int section, Qt::Orientation orientation, const QVariant& value, int role ) { Q_UNUSED( role ) if ( orientation == Qt::Horizontal ) { if ( m_columnHeadings.value( section ) != QVariant() ) + { m_columnHeadings.replace( section, value ); + } else + { m_columnHeadings.insert( section, value ); + } emit headerDataChanged( orientation, section, section ); } return true; @@ -144,9 +178,13 @@ Qt::ItemFlags PackageModel::flags( const QModelIndex& index ) const { if ( !index.isValid() ) + { return Qt::ItemFlags(); + } if ( index.column() == 0 ) + { return Qt::ItemIsUserCheckable | QAbstractItemModel::flags( index ); + } return QAbstractItemModel::flags( index ); } @@ -154,46 +192,55 @@ QVariant PackageModel::headerData( int section, Qt::Orientation orientation, int role ) const { if ( orientation == Qt::Horizontal && role == Qt::DisplayRole ) + { return m_columnHeadings.value( section ); + } return QVariant(); } -QList +QList< PackageTreeItem::ItemData > PackageModel::getPackages() const { - QList items = getItemPackages( m_rootItem ); + QList< PackageTreeItem* > items = getItemPackages( m_rootItem ); for ( auto package : m_hiddenItems ) if ( package->hiddenSelected() ) + { items.append( getItemPackages( package ) ); - QList packages; + } + QList< PackageTreeItem::ItemData > packages; for ( auto item : items ) { PackageTreeItem::ItemData itemData; - itemData.preScript = item->parentItem()->preScript(); // Only groups have hooks - itemData.packageName = item->packageName(); // this seg faults - itemData.postScript = item->parentItem()->postScript(); // Only groups have hooks - itemData.isCritical = item->parentItem()->isCritical(); // Only groups are critical + itemData.preScript = item->parentItem()->preScript(); // Only groups have hooks + itemData.packageName = item->packageName(); // this seg faults + itemData.postScript = item->parentItem()->postScript(); // Only groups have hooks + itemData.isCritical = item->parentItem()->isCritical(); // Only groups are critical packages.append( itemData ); } return packages; } -QList +QList< PackageTreeItem* > PackageModel::getItemPackages( PackageTreeItem* item ) const { - QList selectedPackages; + QList< PackageTreeItem* > selectedPackages; for ( int i = 0; i < item->childCount(); i++ ) { if ( item->child( i )->isSelected() == Qt::Unchecked ) + { continue; + } - if ( !item->child( i )->childCount() ) // package + if ( !item->child( i )->childCount() ) // package + { selectedPackages.append( item->child( i ) ); + } else + { selectedPackages.append( getItemPackages( item->child( i ) ) ); + } } return selectedPackages; - } void @@ -203,49 +250,49 @@ PackageModel::setupModelData( const YAML::Node& data, PackageTreeItem* parent ) { const YAML::Node itemDefinition = *it; - QString name( - tr( CalamaresUtils::yamlToVariant( itemDefinition["name"] ).toByteArray() ) ); - QString description( - tr( CalamaresUtils::yamlToVariant( itemDefinition["description"] ).toByteArray() ) ); + QString name( tr( CalamaresUtils::yamlToVariant( itemDefinition[ "name" ] ).toByteArray() ) ); + QString description( tr( CalamaresUtils::yamlToVariant( itemDefinition[ "description" ] ).toByteArray() ) ); PackageTreeItem::ItemData itemData; itemData.name = name; itemData.description = description; - if ( itemDefinition["pre-install"] ) - itemData.preScript = - CalamaresUtils::yamlToVariant( itemDefinition["pre-install"] ).toString(); - if ( itemDefinition["post-install"] ) - itemData.postScript = - CalamaresUtils::yamlToVariant( itemDefinition["post-install"] ).toString(); + if ( itemDefinition[ "pre-install" ] ) + itemData.preScript = CalamaresUtils::yamlToVariant( itemDefinition[ "pre-install" ] ).toString(); + if ( itemDefinition[ "post-install" ] ) + itemData.postScript = CalamaresUtils::yamlToVariant( itemDefinition[ "post-install" ] ).toString(); PackageTreeItem* item = new PackageTreeItem( itemData, parent ); - if ( itemDefinition["selected"] ) - item->setSelected( - CalamaresUtils::yamlToVariant( itemDefinition["selected"] ).toBool() ? - Qt::Checked : Qt::Unchecked ); + if ( itemDefinition[ "selected" ] ) + item->setSelected( CalamaresUtils::yamlToVariant( itemDefinition[ "selected" ] ).toBool() ? Qt::Checked + : Qt::Unchecked ); else - item->setSelected( parent->isSelected() ); // Inherit from it's parent + { + item->setSelected( parent->isSelected() ); // Inherit from it's parent + } - if ( itemDefinition["hidden"] ) - item->setHidden( - CalamaresUtils::yamlToVariant( itemDefinition["hidden"] ).toBool() ); + if ( itemDefinition[ "hidden" ] ) + item->setHidden( CalamaresUtils::yamlToVariant( itemDefinition[ "hidden" ] ).toBool() ); - if ( itemDefinition["critical"] ) - item->setCritical( - CalamaresUtils::yamlToVariant( itemDefinition["critical"] ).toBool() ); + if ( itemDefinition[ "critical" ] ) + item->setCritical( CalamaresUtils::yamlToVariant( itemDefinition[ "critical" ] ).toBool() ); - if ( itemDefinition["packages"] ) - for ( YAML::const_iterator packageIt = itemDefinition["packages"].begin(); - packageIt != itemDefinition["packages"].end(); ++packageIt ) + if ( itemDefinition[ "packages" ] ) + for ( YAML::const_iterator packageIt = itemDefinition[ "packages" ].begin(); + packageIt != itemDefinition[ "packages" ].end(); + ++packageIt ) item->appendChild( new PackageTreeItem( CalamaresUtils::yamlToVariant( *packageIt ).toString(), item ) ); - if ( itemDefinition["subgroups"] ) - setupModelData( itemDefinition["subgroups"], item ); + if ( itemDefinition[ "subgroups" ] ) + { + setupModelData( itemDefinition[ "subgroups" ], item ); + } if ( item->isHidden() ) + { m_hiddenItems.append( item ); + } else { item->setCheckable( true ); diff --git a/src/modules/netinstall/PackageModel.h b/src/modules/netinstall/PackageModel.h index f84b2779d..cd8f676c8 100644 --- a/src/modules/netinstall/PackageModel.h +++ b/src/modules/netinstall/PackageModel.h @@ -29,7 +29,7 @@ namespace YAML { - class Node; +class Node; } class PackageModel : public QAbstractItemModel @@ -43,27 +43,24 @@ public: ~PackageModel() override; QVariant data( const QModelIndex& index, int role ) const override; - bool setData( const QModelIndex& index, const QVariant& value, - int role = Qt::EditRole ) override; - bool setHeaderData( int section, Qt::Orientation orientation, - const QVariant& value, int role = Qt::EditRole ) override; + bool setData( const QModelIndex& index, const QVariant& value, int role = Qt::EditRole ) override; + bool + setHeaderData( int section, Qt::Orientation orientation, const QVariant& value, int role = Qt::EditRole ) override; Qt::ItemFlags flags( const QModelIndex& index ) const override; - QVariant headerData( int section, Qt::Orientation orientation, - int role = Qt::DisplayRole ) const override; - QModelIndex index( int row, int column, - const QModelIndex& parent = QModelIndex() ) const override; + QVariant headerData( int section, Qt::Orientation orientation, int role = Qt::DisplayRole ) const override; + QModelIndex index( int row, int column, const QModelIndex& parent = QModelIndex() ) const override; QModelIndex parent( const QModelIndex& index ) const override; int rowCount( const QModelIndex& parent = QModelIndex() ) const override; int columnCount( const QModelIndex& parent = QModelIndex() ) const override; PackageItemDataList getPackages() const; - QList getItemPackages( PackageTreeItem* item ) const; + QList< PackageTreeItem* > getItemPackages( PackageTreeItem* item ) const; private: void setupModelData( const YAML::Node& data, PackageTreeItem* parent ); PackageTreeItem* m_rootItem; - QList m_hiddenItems; + QList< PackageTreeItem* > m_hiddenItems; QVariantList m_columnHeadings; }; -#endif // PACKAGEMODEL_H +#endif // PACKAGEMODEL_H diff --git a/src/modules/netinstall/PackageTreeItem.cpp b/src/modules/netinstall/PackageTreeItem.cpp index b3dc6fae7..0b4dfb694 100644 --- a/src/modules/netinstall/PackageTreeItem.cpp +++ b/src/modules/netinstall/PackageTreeItem.cpp @@ -24,25 +24,30 @@ PackageTreeItem::PackageTreeItem( const ItemData& data, PackageTreeItem* parent ) : m_parentItem( parent ) , m_data( data ) -{ } +{ +} -PackageTreeItem::PackageTreeItem( const QString packageName, PackageTreeItem* parent ) : - m_parentItem( parent ) +PackageTreeItem::PackageTreeItem( const QString packageName, PackageTreeItem* parent ) + : m_parentItem( parent ) { m_data.packageName = packageName; if ( parent != nullptr ) + { m_data.selected = parent->isSelected(); + } else + { m_data.selected = Qt::Unchecked; + } } -PackageTreeItem::PackageTreeItem( PackageTreeItem* parent ) : - m_parentItem( parent ) +PackageTreeItem::PackageTreeItem( PackageTreeItem* parent ) + : m_parentItem( parent ) { } -PackageTreeItem::PackageTreeItem::PackageTreeItem() : - PackageTreeItem( QString(), nullptr ) +PackageTreeItem::PackageTreeItem::PackageTreeItem() + : PackageTreeItem( QString(), nullptr ) { m_data.selected = Qt::Checked; m_data.name = QLatin1String( "" ); @@ -75,7 +80,9 @@ int PackageTreeItem::row() const { if ( m_parentItem ) - return m_parentItem->m_childItems.indexOf( const_cast( this ) ); + { + return m_parentItem->m_childItems.indexOf( const_cast< PackageTreeItem* >( this ) ); + } return 0; } @@ -88,13 +95,15 @@ PackageTreeItem::columnCount() const QVariant PackageTreeItem::data( int column ) const { - if ( packageName() != nullptr ) // package + if ( packageName() != nullptr ) // package { if ( !column ) + { return QVariant( packageName() ); + } return QVariant(); } - switch ( column ) // group + switch ( column ) // group { case 0: return QVariant( prettyName() ); @@ -164,14 +173,18 @@ bool PackageTreeItem::hiddenSelected() const { Q_ASSERT( m_data.isHidden ); - if (! m_data.selected ) + if ( !m_data.selected ) + { return false; + } const PackageTreeItem* currentItem = parentItem(); while ( currentItem != nullptr ) { if ( !currentItem->isHidden() ) + { return currentItem->isSelected() != Qt::Unchecked; + } currentItem = currentItem->parentItem(); } @@ -202,8 +215,10 @@ void PackageTreeItem::setSelected( Qt::CheckState isSelected ) { if ( parentItem() == nullptr ) - // This is the root, it is always checked so don't change state + // This is the root, it is always checked so don't change state + { return; + } m_data.selected = isSelected; setChildrenSelected( isSelected ); @@ -216,8 +231,10 @@ PackageTreeItem::setSelected( Qt::CheckState isSelected ) currentItem = currentItem->parentItem(); } if ( currentItem == nullptr ) - // Reached the root .. don't bother + // Reached the root .. don't bother + { return; + } // Figure out checked-state based on the children int childrenSelected = 0; @@ -225,16 +242,26 @@ PackageTreeItem::setSelected( Qt::CheckState isSelected ) for ( int i = 0; i < currentItem->childCount(); i++ ) { if ( currentItem->child( i )->isSelected() == Qt::Checked ) + { childrenSelected++; + } if ( currentItem->child( i )->isSelected() == Qt::PartiallyChecked ) + { childrenPartiallySelected++; + } } - if ( !childrenSelected && !childrenPartiallySelected) + if ( !childrenSelected && !childrenPartiallySelected ) + { currentItem->setSelected( Qt::Unchecked ); + } else if ( childrenSelected == currentItem->childCount() ) + { currentItem->setSelected( Qt::Checked ); + } else + { currentItem->setSelected( Qt::PartiallyChecked ); + } } void diff --git a/src/modules/netinstall/PackageTreeItem.h b/src/modules/netinstall/PackageTreeItem.h index 9c1c8c5a5..c35dc5733 100644 --- a/src/modules/netinstall/PackageTreeItem.h +++ b/src/modules/netinstall/PackageTreeItem.h @@ -21,8 +21,8 @@ #define PACKAGETREEITEM_H #include -#include #include +#include class PackageTreeItem : public QStandardItem { @@ -78,11 +78,12 @@ public: void setSelected( Qt::CheckState isSelected ); void setChildrenSelected( Qt::CheckState isSelected ); int type() const override; + private: PackageTreeItem* m_parentItem; - QList m_childItems; + QList< PackageTreeItem* > m_childItems; ItemData m_data; - const int m_columns = 2; // Name, description + const int m_columns = 2; // Name, description }; -#endif // PACKAGETREEITEM_H +#endif // PACKAGETREEITEM_H From 8286bff95fe945604aeebd40f9eee8274c2e260c Mon Sep 17 00:00:00 2001 From: Adriaan de Groot Date: Tue, 18 Feb 2020 11:28:42 +0100 Subject: [PATCH 2/8] [netinstall] Shuffle code around a bit - introduce char const for key name (consistency, it's used lots) - polish debugging a bit - add some inline code-docs --- src/modules/netinstall/NetInstallViewStep.cpp | 21 ++++++++++++------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/src/modules/netinstall/NetInstallViewStep.cpp b/src/modules/netinstall/NetInstallViewStep.cpp index 94efb5f86..be163a422 100644 --- a/src/modules/netinstall/NetInstallViewStep.cpp +++ b/src/modules/netinstall/NetInstallViewStep.cpp @@ -115,15 +115,21 @@ NetInstallViewStep::onActivate() void NetInstallViewStep::onLeave() { - cDebug() << "Leaving netinstall, adding packages to be installed" - << "to global storage"; - PackageModel::PackageItemDataList packages = m_widget->selectedPackages(); + cDebug() << "Netinstall: Processing" << packages.length() << "packages."; + + static const char PACKAGEOP[] = "packageOperations"; + + // Check if there's already a PACAKGEOP entry in GS, and if so we'll + // extend that one (overwriting the value in GS at the end of this method) + Calamares::GlobalStorage* gs = Calamares::JobQueue::instance()->globalStorage(); + QVariantList packageOperations = gs->contains( PACKAGEOP ) ? gs->value( PACKAGEOP ).toList() : QVariantList(); + cDebug() << Logger::SubEntry << "Existing package operations length" << packageOperations.length(); + + // This netinstall module may add two sub-steps to the packageOperations, + // one for installing and one for try-installing. QVariantList installPackages; QVariantList tryInstallPackages; - QVariantList packageOperations; - - cDebug() << "Processing" << packages.length() << "packages from netinstall."; for ( auto package : packages ) { @@ -165,8 +171,7 @@ NetInstallViewStep::onLeave() if ( !packageOperations.isEmpty() ) { - Calamares::GlobalStorage* gs = Calamares::JobQueue::instance()->globalStorage(); - gs->insert( "packageOperations", QVariant( packageOperations ) ); + gs->insert( PACKAGEOP, packageOperations ); } } From d5675508fa4c02de55447c88ebb52947e69b01f7 Mon Sep 17 00:00:00 2001 From: Adriaan de Groot Date: Tue, 18 Feb 2020 11:39:54 +0100 Subject: [PATCH 3/8] [netinstall] More coding-style The tools don't always pick up all the style changes in one go (I think astyle has trouble parsing some Calamares code) --- src/modules/netinstall/PackageModel.cpp | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/modules/netinstall/PackageModel.cpp b/src/modules/netinstall/PackageModel.cpp index 331586869..b5b256ae0 100644 --- a/src/modules/netinstall/PackageModel.cpp +++ b/src/modules/netinstall/PackageModel.cpp @@ -258,9 +258,13 @@ PackageModel::setupModelData( const YAML::Node& data, PackageTreeItem* parent ) itemData.description = description; if ( itemDefinition[ "pre-install" ] ) + { itemData.preScript = CalamaresUtils::yamlToVariant( itemDefinition[ "pre-install" ] ).toString(); + } if ( itemDefinition[ "post-install" ] ) + { itemData.postScript = CalamaresUtils::yamlToVariant( itemDefinition[ "post-install" ] ).toString(); + } PackageTreeItem* item = new PackageTreeItem( itemData, parent ); if ( itemDefinition[ "selected" ] ) @@ -272,10 +276,14 @@ PackageModel::setupModelData( const YAML::Node& data, PackageTreeItem* parent ) } if ( itemDefinition[ "hidden" ] ) + { item->setHidden( CalamaresUtils::yamlToVariant( itemDefinition[ "hidden" ] ).toBool() ); + } if ( itemDefinition[ "critical" ] ) + { item->setCritical( CalamaresUtils::yamlToVariant( itemDefinition[ "critical" ] ).toBool() ); + } if ( itemDefinition[ "packages" ] ) for ( YAML::const_iterator packageIt = itemDefinition[ "packages" ].begin(); From 056b0d7548de342277d3fc50ddb8f8ff274a2d6f Mon Sep 17 00:00:00 2001 From: Adriaan de Groot Date: Tue, 18 Feb 2020 11:40:43 +0100 Subject: [PATCH 4/8] [netinstall] Refactor variant-from-ItemData --- src/modules/netinstall/NetInstallViewStep.cpp | 18 +++--------------- src/modules/netinstall/PackageTreeItem.cpp | 19 +++++++++++++++++++ src/modules/netinstall/PackageTreeItem.h | 7 +++++++ 3 files changed, 29 insertions(+), 15 deletions(-) diff --git a/src/modules/netinstall/NetInstallViewStep.cpp b/src/modules/netinstall/NetInstallViewStep.cpp index be163a422..53250ba13 100644 --- a/src/modules/netinstall/NetInstallViewStep.cpp +++ b/src/modules/netinstall/NetInstallViewStep.cpp @@ -111,7 +111,6 @@ NetInstallViewStep::onActivate() m_widget->onActivate(); } - void NetInstallViewStep::onLeave() { @@ -131,26 +130,15 @@ NetInstallViewStep::onLeave() QVariantList installPackages; QVariantList tryInstallPackages; - for ( auto package : packages ) + for ( const auto& package : packages ) { - QVariant details( package.packageName ); - // If it's a package with a pre- or post-script, replace - // with the more complicated datastructure. - if ( !package.preScript.isEmpty() || !package.postScript.isEmpty() ) - { - QMap< QString, QVariant > sdetails; - sdetails.insert( "pre-script", package.preScript ); - sdetails.insert( "package", package.packageName ); - sdetails.insert( "post-script", package.postScript ); - details = sdetails; - } if ( package.isCritical ) { - installPackages.append( details ); + installPackages.append( package.toOperation() ); } else { - tryInstallPackages.append( details ); + tryInstallPackages.append( package.toOperation() ); } } diff --git a/src/modules/netinstall/PackageTreeItem.cpp b/src/modules/netinstall/PackageTreeItem.cpp index 0b4dfb694..bbd85975e 100644 --- a/src/modules/netinstall/PackageTreeItem.cpp +++ b/src/modules/netinstall/PackageTreeItem.cpp @@ -21,6 +21,25 @@ #include "utils/Logger.h" +QVariant +PackageTreeItem::ItemData::toOperation() const +{ + // If it's a package with a pre- or post-script, replace + // with the more complicated datastructure. + if ( !preScript.isEmpty() || !postScript.isEmpty() ) + { + QMap< QString, QVariant > sdetails; + sdetails.insert( "pre-script", preScript ); + sdetails.insert( "package", packageName ); + sdetails.insert( "post-script", postScript ); + return sdetails; + } + else + { + return packageName; + } +} + PackageTreeItem::PackageTreeItem( const ItemData& data, PackageTreeItem* parent ) : m_parentItem( parent ) , m_data( data ) diff --git a/src/modules/netinstall/PackageTreeItem.h b/src/modules/netinstall/PackageTreeItem.h index c35dc5733..c2672b4d4 100644 --- a/src/modules/netinstall/PackageTreeItem.h +++ b/src/modules/netinstall/PackageTreeItem.h @@ -37,6 +37,13 @@ public: bool isCritical = false; bool isHidden = false; Qt::CheckState selected = Qt::Unchecked; + + /** @brief Turns this item into a variant for PackageOperations use + * + * For "plain" items, this is just the package name; items with + * scripts return a map. See the package module for how it's interpreted. + */ + QVariant toOperation() const; }; explicit PackageTreeItem( const ItemData& data, PackageTreeItem* parent = nullptr ); explicit PackageTreeItem( const QString packageName, PackageTreeItem* parent = nullptr ); From 7cadfb8dddb3b1f2e2068024b9334917d309f90c Mon Sep 17 00:00:00 2001 From: Adriaan de Groot Date: Tue, 18 Feb 2020 12:02:16 +0100 Subject: [PATCH 5/8] [packages] Log unfamiliar package operations - unknown operations get a warning - "source" will be added from netinstall shortly --- src/modules/packages/main.py | 5 ++++- src/modules/packages/packages.conf | 9 ++++++--- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/src/modules/packages/main.py b/src/modules/packages/main.py index aa4bb8b12..685cea0e5 100644 --- a/src/modules/packages/main.py +++ b/src/modules/packages/main.py @@ -478,7 +478,10 @@ def run_operations(pkgman, entry): else: for package in package_list: pkgman.install_package(package, from_local=True) - + elif key == "source": + libcalamares.utils.debug("Package-list from {!s}".format(entry[key])) + else: + libcalamares.utils.warning("Unknown package-operation key {!s}".format(key)) completed_packages += len(package_list) libcalamares.job.setprogress(completed_packages * 1.0 / total_packages) libcalamares.utils.debug(pretty_name()) diff --git a/src/modules/packages/packages.conf b/src/modules/packages/packages.conf index e42e8e9b8..7b631d79e 100644 --- a/src/modules/packages/packages.conf +++ b/src/modules/packages/packages.conf @@ -55,22 +55,25 @@ update_system: false # that is in this configuration file). # # Allowed package operations are: -# - install, try_install: will call the package manager to +# - *install*, *try_install*: will call the package manager to # install one or more packages. The install target will # abort the whole installation if package-installation # fails, while try_install carries on. Packages may be # listed as (localized) names, or as (localized) package-data. # See below for the description of the format. -# - localInstall: this is used to call the package manager +# - *localInstall*: this is used to call the package manager # to install a package from a path-to-a-package. This is # useful if you have a static package archive on the install media. # The *pacman* package manager is the only one to specially support # this operation (all others treat this the same as *install*). -# - remove, try_remove: will call the package manager to +# - *remove*, *try_remove*: will call the package manager to # remove one or more packages. The remove target will # abort the whole installation if package-removal fails, # while try_remove carries on. Packages may be listed as # (localized) names. +# One additional key is recognized, to help netinstall out: +# - *source*: ignored, does get logged +# Any other key is ignored, and logged as a warning. # # There are two formats for naming packages: as a name or as package-data, # which is an object notation providing package-name, as well as pre- and From 74169c166aa75904490bb948cf7c7c74b51ce2b2 Mon Sep 17 00:00:00 2001 From: Adriaan de Groot Date: Tue, 18 Feb 2020 14:18:48 +0100 Subject: [PATCH 6/8] [netinstall] Mark operations with source-module - This will allow us to find the operations later, by looking for the same source-module. - While here, tidy up types --- src/modules/netinstall/NetInstallViewStep.cpp | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/modules/netinstall/NetInstallViewStep.cpp b/src/modules/netinstall/NetInstallViewStep.cpp index 53250ba13..e7648cd61 100644 --- a/src/modules/netinstall/NetInstallViewStep.cpp +++ b/src/modules/netinstall/NetInstallViewStep.cpp @@ -144,15 +144,17 @@ NetInstallViewStep::onLeave() if ( !installPackages.empty() ) { - QMap< QString, QVariant > op; + QVariantMap op; op.insert( "install", QVariant( installPackages ) ); + op.insert( "source", moduleInstanceKey().toString() ); packageOperations.append( op ); cDebug() << Logger::SubEntry << installPackages.length() << "critical packages."; } if ( !tryInstallPackages.empty() ) { - QMap< QString, QVariant > op; + QVariantMap op; op.insert( "try_install", QVariant( tryInstallPackages ) ); + op.insert( "source", moduleInstanceKey().toString() ); packageOperations.append( op ); cDebug() << Logger::SubEntry << tryInstallPackages.length() << "non-critical packages."; } From 5f1bd4396e97eaeb0ec03264cdcb871fe88015be Mon Sep 17 00:00:00 2001 From: Adriaan de Groot Date: Tue, 18 Feb 2020 14:46:00 +0100 Subject: [PATCH 7/8] [netinstall] Avoid duplicate operations - Since operations are added each time you leave this page, the existing operations (from a previous visit) need to be cleaned up. With the old setup of only **one** possible set of operations, this wasn't a problem. Now, merging in operations is necessary. Implement that by looking for the *source* property in an operation. FIXES #1303 --- src/modules/netinstall/NetInstallViewStep.cpp | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/src/modules/netinstall/NetInstallViewStep.cpp b/src/modules/netinstall/NetInstallViewStep.cpp index e7648cd61..b0fe53083 100644 --- a/src/modules/netinstall/NetInstallViewStep.cpp +++ b/src/modules/netinstall/NetInstallViewStep.cpp @@ -125,6 +125,19 @@ NetInstallViewStep::onLeave() QVariantList packageOperations = gs->contains( PACKAGEOP ) ? gs->value( PACKAGEOP ).toList() : QVariantList(); cDebug() << Logger::SubEntry << "Existing package operations length" << packageOperations.length(); + // Clear out existing operations for this module, going backwards: + // Sometimes we remove an item, and we don't want the index to + // fall off the end of the list. + for ( int index = packageOperations.length() - 1; 0 <= index ; index-- ) + { + const QVariantMap op = packageOperations.at(index).toMap(); + if ( op.contains( "source" ) && op.value( "source" ).toString() == moduleInstanceKey().toString() ) + { + cDebug() << Logger::SubEntry << "Removing existing operations for" << moduleInstanceKey(); + packageOperations.removeAt( index ); + } + } + // This netinstall module may add two sub-steps to the packageOperations, // one for installing and one for try-installing. QVariantList installPackages; From eb127a5e1b19fa47d493227e8d694ea9fb54b151 Mon Sep 17 00:00:00 2001 From: Adriaan de Groot Date: Tue, 18 Feb 2020 15:04:49 +0100 Subject: [PATCH 8/8] Changes: document netinstall module changes --- CHANGES | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/CHANGES b/CHANGES index 4bc35f2d5..377aa2271 100644 --- a/CHANGES +++ b/CHANGES @@ -34,6 +34,11 @@ This release contains contributions from (alphabetically by first name): - The *users* module now has knobs for setting the hostname and writing the `/etc/hosts` file. The new configuration options are documented in `users.conf`. #1140 + - Multiple *netinstall* modules can exist side-by-side, and they each + control the package installation for their part of the package list. + Previously, a netinstall module would overwrite all of the package + configuration done by other netinstall modules. + #1303 # 3.2.18 (2020-01-28) #