From a2d6c2b34a8d7c5d6fc9735529ec23865abb506e Mon Sep 17 00:00:00 2001 From: hjk Date: Wed, 17 Feb 2021 14:11:58 +0100 Subject: [PATCH] Utils: Consolidate more aspect functionality The concepts of 'value', 'defaultValue' and changed signals are present in a few aspects, with varying implementations. Time to get things sorted out. The values are stored now as QVariant in the base, with typed accessors in the derived classes, keeping the user visible interface the same. Change-Id: I4d37ef5c7a9795f46ce1bbbabc6a251222b1d54e Reviewed-by: Christian Stenger --- src/libs/utils/aspects.cpp | 345 +++++++++------------- src/libs/utils/aspects.h | 32 +- src/plugins/conan/conaninstallstep.cpp | 1 + src/plugins/ios/iosbuildconfiguration.cpp | 2 + 4 files changed, 153 insertions(+), 227 deletions(-) diff --git a/src/libs/utils/aspects.cpp b/src/libs/utils/aspects.cpp index 17015a0cf4f..3d263cb5c22 100644 --- a/src/libs/utils/aspects.cpp +++ b/src/libs/utils/aspects.cpp @@ -54,6 +54,9 @@ class BaseAspectPrivate { public: Utils::Id m_id; + QVariant m_value; + QVariant m_defaultValue; + QString m_displayName; QString m_settingsKey; // Name of data in settings. QString m_tooltip; @@ -107,6 +110,50 @@ void BaseAspect::setId(Id id) d->m_id = id; } +QVariant BaseAspect::value() const +{ + return d->m_value; +} + +/*! + Sets value. + + Emits changed() if the value changed. +*/ +void BaseAspect::setValue(const QVariant &value) +{ + if (setValueQuietly(value)) + emit changed(); +} + +/*! + Sets value without emitting changed() + + Returns whether the value changed. +*/ +bool BaseAspect::setValueQuietly(const QVariant &value) +{ + if (d->m_value == value) + return false; + d->m_value = value; + return true; +} + +QVariant BaseAspect::defaultValue() const +{ + return d->m_defaultValue; +} + +/*! + Sets a default value for this aspect. + + Default values will not be stored in settings. +*/ +void BaseAspect::setDefaultValue(const QVariant &value) +{ + d->m_defaultValue = value; +} + void BaseAspect::setDisplayName(const QString &displayName) { d->m_displayName = displayName; @@ -256,19 +303,19 @@ void BaseAspect::saveToMap(QVariantMap &data, const QVariant &value, /*! Retrieves the internal value of this BaseAspect from a \c QVariantMap. - - This base implementation does nothing. */ -void BaseAspect::fromMap(const QVariantMap &) -{} +void BaseAspect::fromMap(const QVariantMap &map) +{ + setValue(map.value(settingsKey(), defaultValue()).toBool()); +} /*! Stores the internal value of this BaseAspect into a \c QVariantMap. - - This base implementation does nothing. */ -void BaseAspect::toMap(QVariantMap &) const -{} +void BaseAspect::toMap(QVariantMap &map) const +{ + saveToMap(map, d->m_value, d->m_defaultValue); +} /*! \internal @@ -334,8 +381,6 @@ class BoolAspectPrivate { public: BoolAspect::LabelPlacement m_labelPlacement = BoolAspect::LabelPlacement::AtCheckBox; - bool m_value = false; - bool m_defaultValue = false; QString m_labelText; QPointer m_checkBox; // Owned by configuration widget QPointer m_label; // Owned by configuration widget @@ -344,8 +389,6 @@ public: class SelectionAspectPrivate { public: - int m_value = 0; - int m_defaultValue = 0; SelectionAspect::DisplayStyle m_displayStyle = SelectionAspect::DisplayStyle::RadioButtons; struct Option { QString displayName; QString tooltip; }; @@ -361,7 +404,11 @@ public: class MultiSelectionAspectPrivate { public: - QStringList m_value; + explicit MultiSelectionAspectPrivate(MultiSelectionAspect *q) : q(q) {} + + bool setValueSelectedHelper(const QString &value, bool on); + + MultiSelectionAspect *q; QStringList m_allValues; MultiSelectionAspect::DisplayStyle m_displayStyle = MultiSelectionAspect::DisplayStyle::ListView; @@ -370,9 +417,6 @@ public: // These are all owned by the configuration widget. QPointer m_listView; QPointer m_label; - - void updateListView(); - bool setValueSelectedHelper(const QString &value, bool on); }; class StringAspectPrivate @@ -387,7 +431,6 @@ public: std::function m_displayFilter; std::unique_ptr m_checker; - QString m_value; QString m_placeHolderText; QString m_historyCompleterKey; PathChooser::Kind m_expectedKind = PathChooser::File; @@ -421,8 +464,6 @@ public: class IntegerAspectPrivate { public: - qint64 m_value = 0; - qint64 m_defaultValue = 0; QVariant m_minimumValue; QVariant m_maximumValue; int m_displayIntegerBase = 10; @@ -437,7 +478,6 @@ public: class StringListAspectPrivate { public: - QStringList m_value; }; class AspectContainerPrivate @@ -526,21 +566,21 @@ void StringAspect::setValueAcceptor(StringAspect::ValueAcceptor &&acceptor) */ QString StringAspect::value() const { - return d->m_value; + return BaseAspect::value().toString(); } /*! Sets the \a value of this StringAspect from an ordinary \c QString. */ -void StringAspect::setValue(const QString &value) +void StringAspect::setValue(const QString &val) { - const bool isSame = value == d->m_value; + const bool isSame = val == value(); if (isSame) return; - QString processedValue = value; + QString processedValue = val; if (d->m_valueAcceptor) { - const Utils::optional tmp = d->m_valueAcceptor(d->m_value, value); + const Utils::optional tmp = d->m_valueAcceptor(value(), val); if (!tmp) { update(); // Make sure the original value is retained in the UI return; @@ -548,9 +588,10 @@ void StringAspect::setValue(const QString &value) processedValue = tmp.value(); } - d->m_value = processedValue; - update(); - emit changed(); + if (BaseAspect::setValueQuietly(QVariant(processedValue))) { + update(); + emit changed(); + } } /*! @@ -559,7 +600,7 @@ void StringAspect::setValue(const QString &value) void StringAspect::fromMap(const QVariantMap &map) { if (!settingsKey().isEmpty()) - d->m_value = map.value(settingsKey()).toString(); + BaseAspect::setValueQuietly(map.value(settingsKey())); if (d->m_checker) d->m_checker->fromMap(map); } @@ -569,7 +610,7 @@ void StringAspect::fromMap(const QVariantMap &map) */ void StringAspect::toMap(QVariantMap &map) const { - saveToMap(map, d->m_value, QString()); + saveToMap(map, value(), QString()); if (d->m_checker) d->m_checker->toMap(map); } @@ -583,7 +624,7 @@ void StringAspect::toMap(QVariantMap &map) const */ FilePath StringAspect::filePath() const { - return FilePath::fromUserInput(d->m_value); + return FilePath::fromUserInput(value()); } /*! @@ -839,10 +880,7 @@ void StringAspect::addToLayout(LayoutBuilder &builder) useMacroExpander(d->m_textEditDisplay); connect(d->m_textEditDisplay, &QTextEdit::textChanged, this, [this] { const QString value = d->m_textEditDisplay->document()->toPlainText(); - if (value != d->m_value) { - d->m_value = value; - emit changed(); - } + setValue(value); }); builder.addItem(d->m_textEditDisplay.data()); break; @@ -863,8 +901,7 @@ void StringAspect::addToLayout(LayoutBuilder &builder) void StringAspect::update() { - const QString displayedString = d->m_displayFilter ? d->m_displayFilter(d->m_value) - : d->m_value; + const QString displayedString = d->m_displayFilter ? d->m_displayFilter(value()) : value(); if (d->m_pathChooserDisplay) { d->m_pathChooserDisplay->setFilePath(FilePath::fromString(displayedString)); @@ -938,6 +975,7 @@ void StringAspect::makeCheckable(CheckBoxPlacement checkBoxPlacement, BoolAspect::BoolAspect(const QString &settingsKey) : d(new Internal::BoolAspectPrivate) { + setDefaultValue(false); setSettingsKey(settingsKey); } @@ -966,54 +1004,29 @@ void BoolAspect::addToLayout(LayoutBuilder &builder) builder.addItem(d->m_label.data()); break; } - d->m_checkBox->setChecked(d->m_value); + d->m_checkBox->setChecked(value()); builder.addItem(d->m_checkBox.data()); connect(d->m_checkBox.data(), &QAbstractButton::clicked, this, [this] { - d->m_value = d->m_checkBox->isChecked(); - emit changed(); + setValue(d->m_checkBox->isChecked()); }); } /*! \reimp */ -void BoolAspect::fromMap(const QVariantMap &map) -{ - d->m_value = map.value(settingsKey(), d->m_defaultValue).toBool(); -} - -/*! - \reimp -*/ -void BoolAspect::toMap(QVariantMap &data) const -{ - saveToMap(data, d->m_value, d->m_defaultValue); -} - -bool BoolAspect::defaultValue() const -{ - return d->m_defaultValue; -} - -void BoolAspect::setDefaultValue(bool defaultValue) -{ - d->m_defaultValue = defaultValue; - d->m_value = defaultValue; -} bool BoolAspect::value() const { - return d->m_value; + return BaseAspect::value().toBool(); } void BoolAspect::setValue(bool value) { - if (d->m_value == value) - return; - d->m_value = value; - if (d->m_checkBox) - d->m_checkBox->setChecked(d->m_value); - emit changed(); + if (BaseAspect::setValueQuietly(value)) { + if (d->m_checkBox) + d->m_checkBox->setChecked(value); + emit changed(); + } } void BoolAspect::setLabel(const QString &labelText, LabelPlacement labelPlacement) @@ -1058,16 +1071,13 @@ void SelectionAspect::addToLayout(LayoutBuilder &builder) for (int i = 0, n = d->m_options.size(); i < n; ++i) { const Internal::SelectionAspectPrivate::Option &option = d->m_options.at(i); auto button = new QRadioButton(option.displayName); - button->setChecked(i == d->m_value); + button->setChecked(i == value()); button->setToolTip(option.tooltip); builder.addItems({{}, button}); d->m_buttons.append(button); d->m_buttonGroup->addButton(button); connect(button, &QAbstractButton::clicked, this, [this, i] { - if (d->m_value != i) { - d->m_value = i; - emit changed(); - } + setValue(i); }); } break; @@ -1076,35 +1086,14 @@ void SelectionAspect::addToLayout(LayoutBuilder &builder) d->m_comboBox = createSubWidget(); for (int i = 0, n = d->m_options.size(); i < n; ++i) d->m_comboBox->addItem(d->m_options.at(i).displayName); - connect(d->m_comboBox.data(), QOverload::of(&QComboBox::activated), this, - [this](int index) { - if (d->m_value != index) { - d->m_value = index; - emit changed(); - } - }); - d->m_comboBox->setCurrentIndex(d->m_value); + connect(d->m_comboBox.data(), QOverload::of(&QComboBox::activated), + this, &SelectionAspect::setValue); + d->m_comboBox->setCurrentIndex(value()); builder.addItems({d->m_label.data(), d->m_comboBox.data()}); break; } } -/*! - \reimp -*/ -void SelectionAspect::fromMap(const QVariantMap &map) -{ - d->m_value = map.value(settingsKey(), d->m_defaultValue).toInt(); -} - -/*! - \reimp -*/ -void SelectionAspect::toMap(QVariantMap &data) const -{ - saveToMap(data, d->m_value, d->m_defaultValue); -} - void SelectionAspect::setVisibleDynamic(bool visible) { if (d->m_label) @@ -1115,16 +1104,6 @@ void SelectionAspect::setVisibleDynamic(bool visible) button->setVisible(visible); } -int SelectionAspect::defaultValue() const -{ - return d->m_defaultValue; -} - -void SelectionAspect::setDefaultValue(int defaultValue) -{ - d->m_defaultValue = defaultValue; -} - void SelectionAspect::setDisplayStyle(SelectionAspect::DisplayStyle style) { d->m_displayStyle = style; @@ -1132,22 +1111,23 @@ void SelectionAspect::setDisplayStyle(SelectionAspect::DisplayStyle style) int SelectionAspect::value() const { - return d->m_value; + return BaseAspect::value().toInt(); } void SelectionAspect::setValue(int value) { - d->m_value = value; - if (d->m_buttonGroup && 0 <= value && value < d->m_buttons.size()) - d->m_buttons.at(value)->setChecked(true); - else if (d->m_comboBox) { - d->m_comboBox->setCurrentIndex(value); + if (BaseAspect::setValueQuietly(value)) { + if (d->m_buttonGroup && 0 <= value && value < d->m_buttons.size()) + d->m_buttons.at(value)->setChecked(true); + else if (d->m_comboBox) + d->m_comboBox->setCurrentIndex(value); + emit changed(); } } QString SelectionAspect::stringValue() const { - return d->m_options.at(d->m_value).displayName; + return d->m_options.at(value()).displayName; } void SelectionAspect::addOption(const QString &displayName, const QString &toolTip) @@ -1167,8 +1147,10 @@ void SelectionAspect::addOption(const QString &displayName, const QString &toolT */ MultiSelectionAspect::MultiSelectionAspect() - : d(new Internal::MultiSelectionAspectPrivate) -{} + : d(new Internal::MultiSelectionAspectPrivate(this)) +{ + setDefaultValue(QStringList()); +} /*! \reimp @@ -1188,10 +1170,10 @@ void MultiSelectionAspect::addToLayout(LayoutBuilder &builder) case DisplayStyle::ListView: d->m_label = createSubWidget(d->m_labelText); d->m_listView = createSubWidget(); - for (const QString &value : qAsConst(d->m_allValues)) { - auto item = new QListWidgetItem(value, d->m_listView); + for (const QString &val : qAsConst(d->m_allValues)) { + auto item = new QListWidgetItem(val, d->m_listView); item->setFlags(item->flags() | Qt::ItemIsUserCheckable); - item->setCheckState(d->m_value.contains(item->text()) ? Qt::Checked : Qt::Unchecked); + item->setCheckState(value().contains(item->text()) ? Qt::Checked : Qt::Unchecked); } connect(d->m_listView, &QListWidget::itemChanged, this, [this](QListWidgetItem *item) { @@ -1202,14 +1184,17 @@ void MultiSelectionAspect::addToLayout(LayoutBuilder &builder) } } -bool Internal::MultiSelectionAspectPrivate::setValueSelectedHelper(const QString &value, bool on) +bool Internal::MultiSelectionAspectPrivate::setValueSelectedHelper(const QString &val, bool on) { - if (on && !m_value.contains(value)) { - m_value.append(value); + QStringList list = q->value(); + if (on && !list.contains(val)) { + list.append(val); + q->setValue(list); return true; } - if (!on && m_value.contains(value)) { - m_value.removeOne(value); + if (!on && list.contains(val)) { + list.removeOne(val); + q->setValue(list); return true; } return false; @@ -1230,34 +1215,6 @@ void MultiSelectionAspect::setLabelText(const QString &labelText) d->m_labelText = labelText; } -void Internal::MultiSelectionAspectPrivate::updateListView() -{ - if (!m_listView) - return; - const int n = m_listView->count(); - QTC_CHECK(n == m_allValues.size()); - for (int i = 0; i != n; ++i) { - auto item = m_listView->item(i); - item->setCheckState(m_value.contains(item->text()) ? Qt::Checked : Qt::Unchecked); - } -} - -/*! - \reimp -*/ -void MultiSelectionAspect::fromMap(const QVariantMap &map) -{ - d->m_value = map.value(settingsKey(), QStringList()).toStringList(); -} - -/*! - \reimp -*/ -void MultiSelectionAspect::toMap(QVariantMap &data) const -{ - saveToMap(data, d->m_value, QStringList()); -} - void MultiSelectionAspect::setVisibleDynamic(bool visible) { if (d->m_label) @@ -1273,16 +1230,23 @@ void MultiSelectionAspect::setDisplayStyle(MultiSelectionAspect::DisplayStyle st QStringList MultiSelectionAspect::value() const { - return d->m_value; + return BaseAspect::value().toStringList(); } void MultiSelectionAspect::setValue(const QStringList &value) { - if (d->m_value == value) - return; - d->m_value = value; - d->updateListView(); - emit changed(); + if (BaseAspect::setValueQuietly(value)) { + if (d->m_listView) { + const int n = d->m_listView->count(); + QTC_CHECK(n == d->m_allValues.size()); + for (int i = 0; i != n; ++i) { + auto item = d->m_listView->item(i); + item->setCheckState(value.contains(item->text()) ? Qt::Checked : Qt::Unchecked); + } + } else { + emit changed(); + } + } } @@ -1304,7 +1268,9 @@ void MultiSelectionAspect::setValue(const QStringList &value) IntegerAspect::IntegerAspect() : d(new Internal::IntegerAspectPrivate) -{} +{ + setDefaultValue(qint64(0)); +} /*! \reimp @@ -1321,7 +1287,7 @@ void IntegerAspect::addToLayout(LayoutBuilder &builder) QTC_CHECK(!d->m_spinBox); d->m_spinBox = createSubWidget(); - d->m_spinBox->setValue(int(d->m_value / d->m_displayScaleFactor)); + d->m_spinBox->setValue(int(value() / d->m_displayScaleFactor)); d->m_spinBox->setDisplayIntegerBase(d->m_displayIntegerBase); d->m_spinBox->setPrefix(d->m_prefix); d->m_spinBox->setSuffix(d->m_suffix); @@ -1332,37 +1298,22 @@ void IntegerAspect::addToLayout(LayoutBuilder &builder) builder.addItems({d->m_label.data(), d->m_spinBox.data()}); connect(d->m_spinBox.data(), QOverload::of(&QSpinBox::valueChanged), this, [this](int value) { - d->m_value = value * d->m_displayScaleFactor; - emit changed(); + setValue(value * d->m_displayScaleFactor); }); } -/*! - \reimp -*/ -void IntegerAspect::fromMap(const QVariantMap &map) -{ - d->m_value = map.value(settingsKey(), d->m_defaultValue).toLongLong(); -} - -/*! - \reimp -*/ -void IntegerAspect::toMap(QVariantMap &data) const -{ - saveToMap(data, d->m_value, d->m_defaultValue); -} - qint64 IntegerAspect::value() const { - return d->m_value; + return BaseAspect::value().toLongLong(); } void IntegerAspect::setValue(qint64 value) { - d->m_value = value; - if (d->m_spinBox) - d->m_spinBox->setValue(int(d->m_value / d->m_displayScaleFactor)); + if (setValueQuietly(value)) { + if (d->m_spinBox) + d->m_spinBox->setValue(int(value / d->m_displayScaleFactor)); + emit changed(); + } } void IntegerAspect::setRange(qint64 min, qint64 max) @@ -1400,7 +1351,7 @@ void IntegerAspect::setDisplayScaleFactor(qint64 factor) void IntegerAspect::setDefaultValue(qint64 defaultValue) { - d->m_defaultValue = defaultValue; + BaseAspect::setDefaultValue(defaultValue); } /*! @@ -1455,7 +1406,9 @@ TriState TriState::fromVariant(const QVariant &variant) StringListAspect::StringListAspect() : d(new Internal::StringListAspectPrivate) -{} +{ + setDefaultValue(QStringList()); +} /*! \reimp @@ -1471,30 +1424,14 @@ void StringListAspect::addToLayout(LayoutBuilder &builder) // TODO - when needed. } -/*! - \reimp -*/ -void StringListAspect::fromMap(const QVariantMap &map) -{ - d->m_value = map.value(settingsKey()).toStringList(); -} - -/*! - \reimp -*/ -void StringListAspect::toMap(QVariantMap &data) const -{ - saveToMap(data, d->m_value, QStringList()); -} - QStringList StringListAspect::value() const { - return d->m_value; + return BaseAspect::value().toStringList(); } void StringListAspect::setValue(const QStringList &value) { - d->m_value = value; + BaseAspect::setValue(value); } /*! diff --git a/src/libs/utils/aspects.h b/src/libs/utils/aspects.h index 26654a574ed..4018f1734a4 100644 --- a/src/libs/utils/aspects.h +++ b/src/libs/utils/aspects.h @@ -63,6 +63,13 @@ public: Utils::Id id() const; void setId(Utils::Id id); + QVariant value() const; + void setValue(const QVariant &value); + bool setValueQuietly(const QVariant &value); + + QVariant defaultValue() const; + void setDefaultValue(const QVariant &value); + QString settingsKey() const; void setSettingsKey(const QString &settingsKey); void setSettingsKey(const QString &group, const QString &key); @@ -84,8 +91,8 @@ public: void setConfigWidgetCreator(const ConfigWidgetCreator &configWidgetCreator); QWidget *createConfigWidget() const; - virtual void fromMap(const QVariantMap &); - virtual void toMap(QVariantMap &) const; + virtual void fromMap(const QVariantMap &map); + virtual void toMap(QVariantMap &map) const; virtual void acquaintSiblings(const BaseAspects &); virtual void addToLayout(LayoutBuilder &builder); @@ -170,16 +177,10 @@ public: bool value() const; void setValue(bool val); - bool defaultValue() const; - void setDefaultValue(bool defaultValue); - enum class LabelPlacement { AtCheckBox, AtCheckBoxWithoutDummyLabel, InExtraLabel }; void setLabel(const QString &labelText, LabelPlacement labelPlacement = LabelPlacement::InExtraLabel); - void fromMap(const QVariantMap &map) override; - void toMap(QVariantMap &map) const override; - private: std::unique_ptr d; }; @@ -199,17 +200,11 @@ public: QString stringValue() const; - int defaultValue() const; - void setDefaultValue(int defaultValue); - enum class DisplayStyle { RadioButtons, ComboBox }; void setDisplayStyle(DisplayStyle style); void addOption(const QString &displayName, const QString &toolTip = {}); - void fromMap(const QVariantMap &map) override; - void toMap(QVariantMap &map) const override; - protected: void setVisibleDynamic(bool visible) override; @@ -238,9 +233,6 @@ public: void setLabelText(const QString &labelText); - void fromMap(const QVariantMap &map) override; - void toMap(QVariantMap &map) const override; - protected: void setVisibleDynamic(bool visible) override; @@ -335,9 +327,6 @@ public: void setDisplayScaleFactor(qint64 factor); void setDefaultValue(qint64 defaultValue); - void fromMap(const QVariantMap &map) override; - void toMap(QVariantMap &map) const override; - private: std::unique_ptr d; }; @@ -390,9 +379,6 @@ public: QStringList value() const; void setValue(const QStringList &val); - void fromMap(const QVariantMap &map) override; - void toMap(QVariantMap &map) const override; - private: std::unique_ptr d; }; diff --git a/src/plugins/conan/conaninstallstep.cpp b/src/plugins/conan/conaninstallstep.cpp index 8d7a4adf315..cb47cf8b224 100644 --- a/src/plugins/conan/conaninstallstep.cpp +++ b/src/plugins/conan/conaninstallstep.cpp @@ -84,6 +84,7 @@ ConanInstallStep::ConanInstallStep(BuildStepList *bsl, Id id) buildMissing->setSettingsKey("ConanPackageManager.InstallStep.BuildMissing"); buildMissing->setLabel("Build missing:", BoolAspect::LabelPlacement::InExtraLabel); buildMissing->setDefaultValue(true); + buildMissing->setValue(true); setCommandLineProvider([=] { BuildConfiguration::BuildType bt = buildConfiguration()->buildType(); diff --git a/src/plugins/ios/iosbuildconfiguration.cpp b/src/plugins/ios/iosbuildconfiguration.cpp index 9cc00960e51..3c4bcfa45b5 100644 --- a/src/plugins/ios/iosbuildconfiguration.cpp +++ b/src/plugins/ios/iosbuildconfiguration.cpp @@ -408,6 +408,7 @@ IosQmakeBuildConfiguration::IosQmakeBuildConfiguration(Target *target, Utils::Id m_autoManagedSigning = addAspect(); m_autoManagedSigning->setDefaultValue(true); + m_autoManagedSigning->setValue(true); m_autoManagedSigning->setSettingsKey(autoManagedSigningKey); connect(m_signingIdentifier, @@ -506,6 +507,7 @@ IosCMakeBuildConfiguration::IosCMakeBuildConfiguration(Target *target, Id id) m_autoManagedSigning = addAspect(); m_autoManagedSigning->setDefaultValue(true); + m_autoManagedSigning->setValue(true); m_autoManagedSigning->setSettingsKey(autoManagedSigningKey); connect(m_signingIdentifier,