Utils: Approach aspect update signalling more systematicly

This was pretty much ad-hoc so far. Now do the actual value proliferation
inside the aspect first and collect information on what changed. Signal
changes in the end centrally. At that time the aspect internal state is
consistent again.

Additional design desision implemented here: setDefaultValue and
fromMap/readSetting do _not_ signal, user code with unusual needs
has to check on its own.

Change-Id: I1e46282558e014f51933d1044a72bf5c67437ec5
Reviewed-by: Qt CI Bot <qt_ci_bot@qt-project.org>
Reviewed-by: Marcus Tillmanns <marcus.tillmanns@qt.io>
Reviewed-by: <github-actions-qt-creator@cristianadam.eu>

src/libs/utils/aspects.cpp

@@ -47,9 +47,12 @@ QSettings *BaseAspect::settings()
     return theSettings;
 }
 
-namespace Internal {
+BaseAspect::Changes::Changes()
+{
+    memset(this, 0, sizeof(*this));
+}
 
-class BaseAspectPrivate
+class Internal::BaseAspectPrivate
 {
 public:
     explicit BaseAspectPrivate(AspectContainer *container) : m_container(container) {}
@@ -82,8 +85,6 @@ public:
     QList<BaseAspect::DataExtractor> m_dataExtractors;
 };
 
-} // Internal
-
 /*!
     \class Utils::BaseAspect
     \inmodule QtCreator
@@ -144,9 +145,10 @@ QVariant BaseAspect::variantValue() const
 
     Prefer the typed setValue() of derived classes.
 */
-void BaseAspect::setVariantValue(const QVariant &value)
+void BaseAspect::setVariantValue(const QVariant &value, Announcement howToAnnounce)
 {
     Q_UNUSED(value)
+    Q_UNUSED(howToAnnounce)
     QTC_CHECK(false);
 }
 
@@ -156,19 +158,6 @@ void BaseAspect::setDefaultVariantValue(const QVariant &value)
     QTC_CHECK(false);
 }
 
-/*!
-    \internal
-
-    Sets \a value without emitting signals.
-
-    Prefer the typed setValueQuietly() of derived classes.
-*/
-void BaseAspect::setVariantValueQuietly(const QVariant &value)
-{
-    Q_UNUSED(value)
-    QTC_CHECK(false);
-}
-
 QVariant BaseAspect::defaultVariantValue() const
 {
     return {};
@@ -488,26 +477,18 @@ void createItem(Layouting::LayoutItem *item, const BaseAspect *aspect)
 */
 void BaseAspect::apply()
 {
-    if (isDirty()) {
+    // We assume m_buffer to reflect current gui state as invariant after signalling settled down.
-        silentApply();
+    // It's an aspect (-subclass) implementation problem if this doesn't hold. Fix it up and bark.
-        if (hasAction())
+    QTC_CHECK(!guiToBuffer());
-            emit action()->triggered(variantValue().toBool());
+
-        emit changed();
+    if (!bufferToInternal()) // Nothing to do.
-    }
+        return;
+
+    Changes changes;
+    changes.internalFromBuffer = true;
+    announceChanges(changes);
 }
 
-/*!
-    Updates this aspect's value from user-initiated changes in the widget.
-
-    \returns whether the value changed. Does not emit signals.
-*/
-
-void BaseAspect::silentApply()
-{
-    guiToBuffer();
-    bufferToInternal();
-    internalToExternal();
-}
 /*!
     Discard user changes in the widget and restore widget contents from
     aspect's value.
@@ -532,6 +513,21 @@ bool BaseAspect::hasAction() const
     return d->m_action != nullptr;
 }
 
+void BaseAspect::announceChanges(Changes changes, Announcement howToAnnounce)
+{
+    if (howToAnnounce == BeQuiet)
+        return;
+
+    if (changes.bufferFromInternal || changes.bufferFromOutside || changes.bufferFromGui)
+        emit volatileValueChanged();
+
+    if (changes.internalFromOutside || changes.internalFromBuffer) {
+        emit changed();
+        if (hasAction())
+            emit action()->triggered(variantValue().toBool());
+    }
+}
+
 bool BaseAspect::isDirty()
 {
     return false;
@@ -578,7 +574,7 @@ void BaseAspect::fromMap(const QVariantMap &map)
     if (settingsKey().isEmpty())
         return;
     const QVariant val = map.value(settingsKey(), toSettingsValue(defaultVariantValue()));
-    setVariantValue(fromSettingsValue(val));
+    setVariantValue(fromSettingsValue(val), BeQuiet);
 }
 
 /*!
@@ -596,7 +592,7 @@ void BaseAspect::readSettings()
     if (settingsKey().isEmpty())
         return;
     const QVariant val = settings()->value(settingsKey());
-    setVariantValue(val.isValid() ? fromSettingsValue(val) : defaultVariantValue());
+    setVariantValue(val.isValid() ? fromSettingsValue(val) : defaultVariantValue(), BeQuiet);
 }
 
 void BaseAspect::writeSettings() const
@@ -908,7 +904,7 @@ void StringAspect::setValueAcceptor(StringAspect::ValueAcceptor &&acceptor)
 void StringAspect::fromMap(const QVariantMap &map)
 {
     if (!settingsKey().isEmpty())
-        setValueQuietly(map.value(settingsKey(), defaultValue()).toString());
+        setValue(map.value(settingsKey(), defaultValue()).toString(), BeQuiet);
     d->m_checkerImpl.fromMap(map);
 }
 
@@ -1112,6 +1108,12 @@ void StringAspect::addToLayout(LayoutItem &parent)
     d->m_checkerImpl.addToLayoutLast(parent);
 }
 
+QString StringAspect::expandedValue() const
+{
+    // FIXME: Use macroexpander here later.
+    return m_internal;
+}
+
 bool StringAspect::guiToBuffer()
 {
     if (d->m_lineEditDisplay)
@@ -1266,9 +1268,14 @@ QString FilePathAspect::value() const
     a file path.
 */
 
-void FilePathAspect::setValue(const FilePath &filePath)
+void FilePathAspect::setValue(const FilePath &filePath, Announcement howToAnnounce)
 {
-    TypedAspect::setValue(filePath.toUserOutput());
+    TypedAspect::setValue(filePath.toUserOutput(), howToAnnounce);
+}
+
+void FilePathAspect::setValue(const QString &filePath, Announcement howToAnnounce)
+{
+    TypedAspect::setValue(filePath, howToAnnounce);
 }
 
 void FilePathAspect::setDefaultValue(const FilePath &filePath)
@@ -1402,7 +1409,7 @@ void FilePathAspect::addToLayout(Layouting::LayoutItem &parent)
 void FilePathAspect::fromMap(const QVariantMap &map)
 {
     if (!settingsKey().isEmpty())
-        setValueQuietly(map.value(settingsKey(), defaultValue()).toString());
+        setValue(map.value(settingsKey(), defaultValue()).toString(), BeQuiet);
     d->m_checkerImpl.fromMap(map);
 }
 
@@ -2545,7 +2552,7 @@ void AspectContainer::finish()
 void AspectContainer::reset()
 {
     for (BaseAspect *aspect : std::as_const(d->m_items))
-        aspect->setVariantValueQuietly(aspect->defaultVariantValue());
+        aspect->setVariantValue(aspect->defaultVariantValue());
 }
 
 void AspectContainer::setAutoApply(bool on)
@@ -2643,23 +2650,6 @@ bool BaseAspect::internalToBuffer()
     return false;
 }
 
-/*
-    Applies common postprocessing like macro expansion.
-*/
-
-bool BaseAspect::internalToExternal()
-{
-    return false;
-}
-
-/*
-    Mirrors externally visible value to internal volatile value.
-*/
-bool BaseAspect::externalToInternal()
-{
-    return false;
-}
-
 void BaseAspect::handleGuiChanged()
 {
     if (guiToBuffer())

src/libs/utils/aspects.h

@@ -53,9 +53,10 @@ public:
     Id id() const;
     void setId(Id id);
 
+    enum Announcement { DoEmit, BeQuiet };
+
     virtual QVariant variantValue() const;
-    virtual void setVariantValue(const QVariant &value);
+    virtual void setVariantValue(const QVariant &value, Announcement = DoEmit);
-    virtual void setVariantValueQuietly(const QVariant &value);
 
     virtual QVariant defaultVariantValue() const;
     virtual void setDefaultVariantValue(const QVariant &value);
@@ -114,12 +115,24 @@ public:
     QVariant fromSettingsValue(const QVariant &val) const;
 
     virtual void apply();
-    virtual void silentApply();
     virtual void cancel();
     virtual void finish();
     virtual bool isDirty();
     bool hasAction() const;
 
+    struct QTCREATOR_UTILS_EXPORT Changes
+    {
+        Changes();
+
+        unsigned internalFromOutside : 1;
+        unsigned internalFromBuffer : 1;
+        unsigned bufferFromOutside : 1;
+        unsigned bufferFromInternal : 1;
+        unsigned bufferFromGui : 1;
+    };
+
+    void announceChanges(Changes changes, Announcement howToAnnounce = DoEmit);
+
     class QTCREATOR_UTILS_EXPORT Data
     {
     public:
@@ -172,7 +185,6 @@ public:
 signals:
     void changed(); // "internal"
     void volatileValueChanged();
-    void externalValueChanged();
     void labelLinkActivated(const QString &link);
     void checkedChanged();
 
@@ -181,8 +193,6 @@ protected:
     virtual bool bufferToInternal();
     virtual void bufferToGui();
     virtual bool guiToBuffer();
-    virtual bool internalToExternal();
-    virtual bool externalToInternal();
 
     virtual void handleGuiChanged();
 
@@ -253,48 +263,41 @@ public:
         ValueType value;
     };
 
-    ValueType operator()() const { return m_external; }
+    ValueType operator()() const { return m_internal; }
-    ValueType value() const { return m_external; }
+    ValueType value() const { return m_internal; }
     ValueType defaultValue() const { return m_default; }
+    ValueType volatileValue() const { return m_buffer; }
 
+    // We assume that this is only used in the ctor and no signalling is needed.
+    // If it is used elsewhere changes have to be detected and signalled externally.
     void setDefaultValue(const ValueType &value)
     {
         m_default = value;
-        setValue(value);
-    }
-
-    void setValue(const ValueType &value)
-    {
-        const bool changes = m_internal != value;
         m_internal = value;
-        if (internalToBuffer())
+        internalToBuffer(); // Might be more than a plain copy.
-            emit volatileValueChanged();
-        bufferToGui();
-        internalToExternal();
-        if (changes)
-            emit changed();
     }
 
-    void setValueQuietly(const ValueType &value)
+    void setValue(const ValueType &value, Announcement howToAnnounce = DoEmit)
     {
-        m_internal = value;
+        Changes changes;
-        internalToBuffer();
+        changes.internalFromOutside = updateStorage(m_internal, value);
-        bufferToGui();
+        if (internalToBuffer()) {
-        internalToExternal();
+            changes.bufferFromInternal = true;
+            bufferToGui();
+        }
+        announceChanges(changes, howToAnnounce);
     }
 
-    ValueType volatileValue() const
+    void setVolatileValue(const ValueType &value, Announcement howToAnnounce = DoEmit)
     {
-        return m_buffer;
+        Changes changes;
-    }
+        if (updateStorage(m_buffer, value)) {
-
+            changes.bufferFromOutside = true;
-    void setVolatileValue(const ValueType &value)
+            bufferToGui();
-    {
+        }
-        if (m_buffer == value)
+        if (isAutoApply() && bufferToInternal())
-            return;
+            changes.internalFromBuffer = true;
-        m_buffer = value;
+        announceChanges(changes, howToAnnounce);
-        bufferToGui();
-        emit volatileValueChanged();
     }
 
 protected:
@@ -313,29 +316,14 @@ protected:
         return updateStorage(m_internal, m_buffer);
     }
 
-    bool internalToExternal() override
-    {
-        return updateStorage(m_external, m_internal);
-    }
-
-    bool externalToInternal() override
-    {
-        return updateStorage(m_internal, m_external);
-    }
-
     QVariant variantValue() const override
     {
-        return QVariant::fromValue<ValueType>(m_external);
+        return QVariant::fromValue<ValueType>(m_internal);
     }
 
-    void setVariantValue(const QVariant &value) override
+    void setVariantValue(const QVariant &value, Announcement howToAnnounce = DoEmit) override
     {
-        setValue(value.value<ValueType>());
+        setValue(value.value<ValueType>(), howToAnnounce);
-    }
-
-    void setVariantValueQuietly(const QVariant &value) override
-    {
-        setValueQuietly(value.value<ValueType>());
     }
 
     QVariant defaultVariantValue() const override
@@ -345,11 +333,10 @@ protected:
 
     void setDefaultVariantValue(const QVariant &value) override
     {
-        m_default = value.value<ValueType>();
+        setDefaultValue(value.value<ValueType>());
     }
 
     ValueType m_default{};
-    ValueType m_external{};
     ValueType m_internal{};
     ValueType m_buffer{};
 };
@@ -366,7 +353,6 @@ public:
     void setInternalToBuffer(const Updater &updater) { m_internalToBuffer = updater; }
     void setBufferToInternal(const Updater &updater) { m_bufferToInternal = updater; }
     void setInternalToExternal(const Updater &updater) { m_internalToExternal = updater; }
-    void setExternalToInternal(const Updater &updater) { m_externalToInternal = updater; }
 
 protected:
     bool internalToBuffer() override
@@ -383,24 +369,18 @@ protected:
         return Base::bufferToInternal();
     }
 
-    bool internalToExternal() override
+    ValueType expandedValue()
     {
-        if (m_internalToExternal)
+        if (!m_internalToExternal)
-            return m_internalToExternal(Base::m_external, Base::m_internal);
+            return Base::m_internal;
-        return Base::internalToExternal();
+        ValueType val;
-    }
+        m_internalToExternal(val, Base::m_internal);
-
+        return val;
-    bool externalToInternal() override
-    {
-        if (m_externalToInternal)
-            return m_externalToInternal(Base::m_internal, Base::m_external);
-        return Base::externalToInternal();
     }
 
     Updater m_internalToBuffer;
     Updater m_bufferToInternal;
     Updater m_internalToExternal;
-    Updater m_externalToInternal;
 };
 
 class QTCREATOR_UTILS_EXPORT BoolAspect : public TypedAspect<bool>
@@ -536,6 +516,9 @@ public:
 
     void addToLayout(Layouting::LayoutItem &parent) override;
 
+    QString operator()() const { return expandedValue(); }
+    QString expandedValue() const;
+
     // Hook between UI and StringAspect:
     using ValueAcceptor = std::function<std::optional<QString>(const QString &, const QString &)>;
     void setValueAcceptor(ValueAcceptor &&acceptor);
@@ -599,7 +582,8 @@ public:
     FilePath operator()() const;
     FilePath expandedValue() const;
     QString value() const;
-    void setValue(const FilePath &filePath);
+    void setValue(const FilePath &filePath, Announcement howToAnnounce = DoEmit);
+    void setValue(const QString &filePath, Announcement howToAnnounce = DoEmit);
     void setDefaultValue(const FilePath &filePath);
 
     void setPromptDialogFilter(const QString &filter);

src/plugins/cmakeprojectmanager/cmakebuildconfiguration.cpp

@@ -2071,10 +2071,7 @@ QString CMakeBuildSystem::cmakeBuildType() const
 
 void CMakeBuildConfiguration::setCMakeBuildType(const QString &cmakeBuildType, bool quiet)
 {
-    if (quiet)
+    buildTypeAspect.setValue(cmakeBuildType, quiet ? BaseAspect::BeQuiet : BaseAspect::DoEmit);
-        buildTypeAspect.setValueQuietly(cmakeBuildType);
-    else
-        buildTypeAspect.setValue(cmakeBuildType);
 }
 
 namespace Internal {
@@ -2134,7 +2131,7 @@ void InitialCMakeArgumentsAspect::setAllValues(const QString &values, QStringLis
 
     // Display the unknown arguments in "Additional CMake Options"
     const QString additionalOptionsValue = ProcessArgs::joinArgs(additionalOptions);
-    setValueQuietly(additionalOptionsValue);
+    setValue(additionalOptionsValue, BeQuiet);
 }
 
 void InitialCMakeArgumentsAspect::setCMakeConfiguration(const CMakeConfig &config)

src/plugins/debugger/registerpostmortemaction.cpp

@@ -60,7 +60,7 @@ void RegisterPostMortemAction::readSettings()
         registered = isRegistered(handle, debuggerCall(), &errorMessage);
     if (handle)
         RegCloseKey(handle);
-    setValueQuietly(registered);
+    setValue(registered, BaseAspect::BeQuiet);
 }
 
 } // namespace Internal

src/plugins/projectexplorer/processstep.cpp

@@ -37,7 +37,7 @@ public:
         arguments.setLabelText(Tr::tr("Arguments:"));
 
         workingDirectory.setSettingsKey(PROCESS_WORKINGDIRECTORY_KEY);
-        workingDirectory.setValue(Constants::DEFAULT_WORKING_DIR);
+        workingDirectory.setValue(QString(Constants::DEFAULT_WORKING_DIR));
         workingDirectory.setLabelText(Tr::tr("Working directory:"));
         workingDirectory.setExpectedKind(PathChooser::Directory);