From 685f452d91a4a733af5b9f9e3aff4a5e73ee37b2 Mon Sep 17 00:00:00 2001 From: Marcus Tillmanns Date: Mon, 11 Mar 2024 11:37:43 +0100 Subject: [PATCH] SettingsDialog: Call Aspect::cancel on cancel Previously the SettingsDialog did not call Aspect::cancel(). Without this, the volatile value is not reset if the user did some changes before pressing cancel. This "should" mean that re-opening the settings dialog should present the changed values again. But since not all aspects are implemented correctly, they reinitialize from their value instead of their volatileValue. When the Settings Dialog is reopened and applied, a warning is triggered as the container thinks its dirty, but the Aspect::apply returns false. Change-Id: I8fac66fc95f9118d69c789ced22481d121769f0b Reviewed-by: Reviewed-by: Christian Stenger Reviewed-by: hjk --- src/plugins/autotest/testsettingspage.cpp | 2 ++ .../artisticstyle/artisticstyle.cpp | 3 ++- .../beautifier/clangformat/clangformat.cpp | 1 + .../beautifier/uncrustify/uncrustify.cpp | 2 ++ .../coreplugin/dialogs/ioptionspage.cpp | 25 +++++++++++++++++++ src/plugins/coreplugin/dialogs/ioptionspage.h | 3 +++ .../coreplugin/dialogs/settingsdialog.cpp | 16 +++++++----- src/plugins/git/gerrit/gerritoptionspage.cpp | 10 ++++++-- 8 files changed, 53 insertions(+), 9 deletions(-) diff --git a/src/plugins/autotest/testsettingspage.cpp b/src/plugins/autotest/testsettingspage.cpp index c11e2dfe318..4bde0e98cb4 100644 --- a/src/plugins/autotest/testsettingspage.cpp +++ b/src/plugins/autotest/testsettingspage.cpp @@ -153,6 +153,8 @@ TestSettingsWidget::TestSettingsWidget() if (!changedIds.isEmpty()) TestTreeModel::instance()->rebuild(changedIds); }); + + setOnCancel([] { Internal::testSettings().cancel(); }); } enum TestBaseInfo diff --git a/src/plugins/beautifier/artisticstyle/artisticstyle.cpp b/src/plugins/beautifier/artisticstyle/artisticstyle.cpp index 2982ac267bc..a3aa30d78e9 100644 --- a/src/plugins/beautifier/artisticstyle/artisticstyle.cpp +++ b/src/plugins/beautifier/artisticstyle/artisticstyle.cpp @@ -221,9 +221,10 @@ public: setOnApply([&s, configurations] { s.customStyle.setValue(configurations->currentConfiguration()); - settings().apply(); + s.apply(); s.save(); }); + setOnCancel([&s] { s.cancel(); }); s.read(); diff --git a/src/plugins/beautifier/clangformat/clangformat.cpp b/src/plugins/beautifier/clangformat/clangformat.cpp index 95de1965c3e..22bcea77f79 100644 --- a/src/plugins/beautifier/clangformat/clangformat.cpp +++ b/src/plugins/beautifier/clangformat/clangformat.cpp @@ -300,6 +300,7 @@ public: settings().apply(); settings().save(); }); + setOnCancel([] { settings().cancel(); }); s.read(); diff --git a/src/plugins/beautifier/uncrustify/uncrustify.cpp b/src/plugins/beautifier/uncrustify/uncrustify.cpp index 75bd5252ed9..a18371ff53b 100644 --- a/src/plugins/beautifier/uncrustify/uncrustify.cpp +++ b/src/plugins/beautifier/uncrustify/uncrustify.cpp @@ -225,6 +225,8 @@ public: settings().apply(); s.save(); }); + + setOnCancel([] { settings().cancel(); }); } }; diff --git a/src/plugins/coreplugin/dialogs/ioptionspage.cpp b/src/plugins/coreplugin/dialogs/ioptionspage.cpp index 6c2970ca45d..d4bc7276e2d 100644 --- a/src/plugins/coreplugin/dialogs/ioptionspage.cpp +++ b/src/plugins/coreplugin/dialogs/ioptionspage.cpp @@ -29,6 +29,7 @@ class IOptionsPageWidgetPrivate { public: std::function m_onApply; + std::function m_onCancel; std::function m_onFinish; }; @@ -104,6 +105,11 @@ void IOptionsPageWidget::setOnApply(const std::function &func) d->m_onApply = func; } +void IOptionsPageWidget::setOnCancel(const std::function &func) +{ + d->m_onCancel = func; +} + /*! Sets the function that is called by default on finish to \a func. */ @@ -122,6 +128,12 @@ void IOptionsPageWidget::apply() d->m_onApply(); } +void IOptionsPageWidget::cancel() +{ + if (d->m_onCancel) + d->m_onCancel(); +} + /*! Calls the finish function, if set. \sa setOnFinish @@ -260,6 +272,19 @@ void IOptionsPage::apply() } } +void IOptionsPage::cancel() +{ + if (auto widget = qobject_cast(d->m_widget)) + widget->cancel(); + + if (d->m_settingsProvider) { + AspectContainer *container = d->m_settingsProvider(); + QTC_ASSERT(container, return); + if (container->isDirty()) + container->cancel(); + } +} + /*! Called directly before the \uicontrol Options dialog closes. Here you should delete the widget that was created in widget() to free resources. diff --git a/src/plugins/coreplugin/dialogs/ioptionspage.h b/src/plugins/coreplugin/dialogs/ioptionspage.h index bb0886dd67a..618cbc80322 100644 --- a/src/plugins/coreplugin/dialogs/ioptionspage.h +++ b/src/plugins/coreplugin/dialogs/ioptionspage.h @@ -27,11 +27,13 @@ public: IOptionsPageWidget(); ~IOptionsPageWidget(); void setOnApply(const std::function &func); + void setOnCancel(const std::function &func); void setOnFinish(const std::function &func); protected: friend class IOptionsPage; virtual void apply(); + virtual void cancel(); virtual void finish(); private: @@ -59,6 +61,7 @@ public: virtual QWidget *widget(); virtual void apply(); + virtual void cancel(); virtual void finish(); virtual bool matches(const QRegularExpression ®exp) const; diff --git a/src/plugins/coreplugin/dialogs/settingsdialog.cpp b/src/plugins/coreplugin/dialogs/settingsdialog.cpp index 6dfd9106eb5..b2e4b9e969f 100644 --- a/src/plugins/coreplugin/dialogs/settingsdialog.cpp +++ b/src/plugins/coreplugin/dialogs/settingsdialog.cpp @@ -594,11 +594,13 @@ void SettingsDialog::createGui() QWidget *emptyWidget = new QWidget(this); m_stackedLayout->addWidget(emptyWidget); // no category selected, for example when filtering - QDialogButtonBox *buttonBox = new QDialogButtonBox(QDialogButtonBox::Ok | - QDialogButtonBox::Apply | - QDialogButtonBox::Cancel); - connect(buttonBox->button(QDialogButtonBox::Apply), &QAbstractButton::clicked, - this, &SettingsDialog::apply); + QDialogButtonBox *buttonBox = new QDialogButtonBox( + QDialogButtonBox::Ok | QDialogButtonBox::Apply | QDialogButtonBox::Cancel); + connect( + buttonBox->button(QDialogButtonBox::Apply), + &QAbstractButton::clicked, + this, + &SettingsDialog::apply); connect(buttonBox, &QDialogButtonBox::accepted, this, &SettingsDialog::accept); connect(buttonBox, &QDialogButtonBox::rejected, this, &SettingsDialog::reject); @@ -744,8 +746,10 @@ void SettingsDialog::reject() return; m_finished = true; disconnectTabWidgets(); - for (IOptionsPage *page : std::as_const(m_pages)) + for (IOptionsPage *page : std::as_const(m_pages)) { + page->cancel(); page->finish(); + } done(QDialog::Rejected); } diff --git a/src/plugins/git/gerrit/gerritoptionspage.cpp b/src/plugins/git/gerrit/gerritoptionspage.cpp index 8954db43aa8..392a8f81461 100644 --- a/src/plugins/git/gerrit/gerritoptionspage.cpp +++ b/src/plugins/git/gerrit/gerritoptionspage.cpp @@ -63,8 +63,14 @@ public: Git::Tr::tr("P&rotocol:"), httpsCheckBox }.attachTo(this); - setOnApply([this, hostLineEdit, userLineEdit, sshChooser, - curlChooser, portSpinBox, httpsCheckBox, onChanged] { + setOnApply([this, + hostLineEdit, + userLineEdit, + sshChooser, + curlChooser, + portSpinBox, + httpsCheckBox, + onChanged] { GerritParameters newParameters; newParameters.server = GerritServer(hostLineEdit->text().trimmed(), static_cast(portSpinBox->value()),