From f09b61651e5f075535bfe60bfa77e22f9104085e Mon Sep 17 00:00:00 2001 From: hjk Date: Fri, 28 Jul 2023 11:08:37 +0200 Subject: [PATCH] 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 Reviewed-by: Marcus Tillmanns Reviewed-by: --- src/libs/utils/aspects.cpp | 110 +++++++--------- src/libs/utils/aspects.h | 124 ++++++++---------- .../cmakebuildconfiguration.cpp | 7 +- .../debugger/registerpostmortemaction.cpp | 2 +- src/plugins/projectexplorer/processstep.cpp | 2 +- 5 files changed, 108 insertions(+), 137 deletions(-) diff --git a/src/libs/utils/aspects.cpp b/src/libs/utils/aspects.cpp index 2ec4bfe280f..2fc74b144b4 100644 --- a/src/libs/utils/aspects.cpp +++ b/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 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()) { - silentApply(); - if (hasAction()) - emit action()->triggered(variantValue().toBool()); - emit changed(); - } + // We assume m_buffer to reflect current gui state as invariant after signalling settled down. + // It's an aspect (-subclass) implementation problem if this doesn't hold. Fix it up and bark. + QTC_CHECK(!guiToBuffer()); + + 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()) diff --git a/src/libs/utils/aspects.h b/src/libs/utils/aspects.h index 14bb3ed6f73..b4b2d4099a9 100644 --- a/src/libs/utils/aspects.h +++ b/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 setVariantValueQuietly(const QVariant &value); + virtual void setVariantValue(const QVariant &value, Announcement = DoEmit); 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 value() const { return m_external; } + ValueType operator()() const { return m_internal; } + 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()) - emit volatileValueChanged(); - bufferToGui(); - internalToExternal(); - if (changes) - emit changed(); + internalToBuffer(); // Might be more than a plain copy. } - void setValueQuietly(const ValueType &value) + void setValue(const ValueType &value, Announcement howToAnnounce = DoEmit) { - m_internal = value; - internalToBuffer(); - bufferToGui(); - internalToExternal(); + Changes changes; + changes.internalFromOutside = updateStorage(m_internal, value); + if (internalToBuffer()) { + changes.bufferFromInternal = true; + bufferToGui(); + } + announceChanges(changes, howToAnnounce); } - ValueType volatileValue() const + void setVolatileValue(const ValueType &value, Announcement howToAnnounce = DoEmit) { - return m_buffer; - } - - void setVolatileValue(const ValueType &value) - { - if (m_buffer == value) - return; - m_buffer = value; - bufferToGui(); - emit volatileValueChanged(); + Changes changes; + if (updateStorage(m_buffer, value)) { + changes.bufferFromOutside = true; + bufferToGui(); + } + if (isAutoApply() && bufferToInternal()) + changes.internalFromBuffer = true; + announceChanges(changes, howToAnnounce); } 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(m_external); + return QVariant::fromValue(m_internal); } - void setVariantValue(const QVariant &value) override + void setVariantValue(const QVariant &value, Announcement howToAnnounce = DoEmit) override { - setValue(value.value()); - } - - void setVariantValueQuietly(const QVariant &value) override - { - setValueQuietly(value.value()); + setValue(value.value(), howToAnnounce); } QVariant defaultVariantValue() const override @@ -345,11 +333,10 @@ protected: void setDefaultVariantValue(const QVariant &value) override { - m_default = value.value(); + setDefaultValue(value.value()); } 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) - return m_internalToExternal(Base::m_external, Base::m_internal); - return Base::internalToExternal(); - } - - bool externalToInternal() override - { - if (m_externalToInternal) - return m_externalToInternal(Base::m_internal, Base::m_external); - return Base::externalToInternal(); + if (!m_internalToExternal) + return Base::m_internal; + ValueType val; + m_internalToExternal(val, Base::m_internal); + return val; } Updater m_internalToBuffer; Updater m_bufferToInternal; Updater m_internalToExternal; - Updater m_externalToInternal; }; class QTCREATOR_UTILS_EXPORT BoolAspect : public TypedAspect @@ -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(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); diff --git a/src/plugins/cmakeprojectmanager/cmakebuildconfiguration.cpp b/src/plugins/cmakeprojectmanager/cmakebuildconfiguration.cpp index d2653baefeb..746c78f5491 100644 --- a/src/plugins/cmakeprojectmanager/cmakebuildconfiguration.cpp +++ b/src/plugins/cmakeprojectmanager/cmakebuildconfiguration.cpp @@ -2071,10 +2071,7 @@ QString CMakeBuildSystem::cmakeBuildType() const void CMakeBuildConfiguration::setCMakeBuildType(const QString &cmakeBuildType, bool quiet) { - if (quiet) - buildTypeAspect.setValueQuietly(cmakeBuildType); - else - buildTypeAspect.setValue(cmakeBuildType); + buildTypeAspect.setValue(cmakeBuildType, quiet ? BaseAspect::BeQuiet : BaseAspect::DoEmit); } 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) diff --git a/src/plugins/debugger/registerpostmortemaction.cpp b/src/plugins/debugger/registerpostmortemaction.cpp index a7f68083180..4cfbdc02e94 100644 --- a/src/plugins/debugger/registerpostmortemaction.cpp +++ b/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 diff --git a/src/plugins/projectexplorer/processstep.cpp b/src/plugins/projectexplorer/processstep.cpp index e2aa69dff9b..782eadf91df 100644 --- a/src/plugins/projectexplorer/processstep.cpp +++ b/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);