From fad2e7a310aa7385a813a0edd6b3985958dd4094 Mon Sep 17 00:00:00 2001 From: Fabrice de Gans Date: Sat, 4 Feb 2023 21:59:04 -0800 Subject: [PATCH] Fix issues with the DisplayConfig dialog (#1066) The DisplayConfig dialog had a few issues: * The OK/Cancel buttons were not working as expected. The configuration options would be immediately written whenever some control values were changed. The configuration options are now properly being set on a click to OK, and ignored on a click to Cancel. * The X button works again. * There is now proper validation for the Display Scale option, validation ensures that the value is in the right range and incorrect data can no longer be input in that field. * The Direct3D option is now properly enabled on Direct3D builds. --- src/wx/config/CMakeLists.txt | 0 src/wx/config/option.cpp | 30 ++++++++ src/wx/config/option.h | 8 +++ src/wx/dialogs/display-config.cpp | 116 +++++++++++++++++------------- src/wx/dialogs/display-config.h | 10 ++- 5 files changed, 111 insertions(+), 53 deletions(-) create mode 100644 src/wx/config/CMakeLists.txt diff --git a/src/wx/config/CMakeLists.txt b/src/wx/config/CMakeLists.txt new file mode 100644 index 00000000..e69de29b diff --git a/src/wx/config/option.cpp b/src/wx/config/option.cpp index b41b7a0d..7e4e1a57 100644 --- a/src/wx/config/option.cpp +++ b/src/wx/config/option.cpp @@ -483,6 +483,36 @@ bool Option::SetGbPalette(const wxString& value) { return true; } +double Option::GetDoubleMin() const { + assert(is_double()); + return nonstd::get(min_); +} + +double Option::GetDoubleMax() const { + assert(is_double()); + return nonstd::get(max_); +} + +int32_t Option::GetIntMin() const { + assert(is_int()); + return nonstd::get(min_); +} + +int32_t Option::GetIntMax() const { + assert(is_int()); + return nonstd::get(max_); +} + +uint32_t Option::GetUnsignedMin() const { + assert(is_unsigned()); + return nonstd::get(min_); +} + +uint32_t Option::GetUnsignedMax() const { + assert(is_unsigned()); + return nonstd::get(max_); +} + void Option::NextFilter() { assert(is_filter()); const int old_value = static_cast(GetFilter()); diff --git a/src/wx/config/option.h b/src/wx/config/option.h index cb30c30a..fc8fa21e 100644 --- a/src/wx/config/option.h +++ b/src/wx/config/option.h @@ -192,6 +192,14 @@ public: bool SetEnumString(const wxString& value); bool SetGbPalette(const wxString& value); + // Min/Max accessors. + double GetDoubleMin() const; + double GetDoubleMax() const; + int32_t GetIntMin() const; + int32_t GetIntMax() const; + uint32_t GetUnsignedMin() const; + uint32_t GetUnsignedMax() const; + // Special convenience modifiers. void NextFilter(); void NextInterframe(); diff --git a/src/wx/dialogs/display-config.cpp b/src/wx/dialogs/display-config.cpp index 5163e7be..7530134c 100644 --- a/src/wx/dialogs/display-config.cpp +++ b/src/wx/dialogs/display-config.cpp @@ -11,6 +11,8 @@ #include #include #include +#include + #include #include "config/option.h" @@ -25,57 +27,60 @@ namespace dialogs { namespace { -// Validator for a wxTextCtrl with a double value. -class ScaleValidator : public widgets::OptionValidator { +// Custom validator for the kDispScale option. We rely on the existing +// wxFloatingPointValidator validator for this. +class ScaleValidator : public wxFloatingPointValidator, + public config::Option::Observer { public: - ScaleValidator() : widgets::OptionValidator(config::OptionID::kDispScale) {} + ScaleValidator() + : wxFloatingPointValidator(&value_), + config::Option::Observer(config::OptionID::kDispScale), + value_(option()->GetDouble()) { + // Let wxFloatingPointValidator do the hard work. + SetMin(option()->GetDoubleMin()); + SetMax(option()->GetDoubleMax()); + SetPrecision(1); + } ~ScaleValidator() override = default; -private: - // OptionValidator implementation. + // Returns a copy of the object. wxObject* Clone() const override { return new ScaleValidator(); } - bool IsWindowValueValid() override { - double value; - if (!wxDynamicCast(GetWindow(), wxTextCtrl) - ->GetValue() - .ToDouble(&value)) { +private: + // wxValidator implementation. + bool TransferFromWindow() final { + if (!wxFloatingPointValidator::TransferFromWindow()) { + wxLogWarning(_("Invalid value for Default magnification.")); return false; } - - return true; + return option()->SetDouble(value_); } - bool WriteToWindow() override { - wxDynamicCast(GetWindow(), wxTextCtrl) - ->SetValue(wxString::Format("%.1f", option()->GetDouble())); - return true; - } + bool TransferToWindow() final { + value_ = option()->GetDouble(); + return wxFloatingPointValidator::TransferToWindow(); + }; - bool WriteToOption() override { - double value; - if (!wxDynamicCast(GetWindow(), wxTextCtrl) - ->GetValue() - .ToDouble(&value)) { - return false; - } - - return option()->SetDouble(value); +#if WX_HAS_VALIDATOR_SET_WINDOW_OVERRIDE + void SetWindow(wxWindow* window) final { + wxFloatingPointValidator::SetWindow(window); } +#endif + + // config::Option::Observer implementation. + void OnValueChanged() final { TransferToWindow(); } + + // A local value used for configuration. + double value_; }; // Validator for a wxChoice with a Filter value. class FilterValidator : public widgets::OptionValidator { public: - FilterValidator() : OptionValidator(config::OptionID::kDispFilter) { - Bind(wxEVT_CHOICE, &FilterValidator::OnChoice, this); - } + FilterValidator() : OptionValidator(config::OptionID::kDispFilter) {} ~FilterValidator() override = default; private: - // wxChoice event handler. - void OnChoice(wxCommandEvent&) { WriteToOption(); } - // OptionValidator implementation. wxObject* Clone() const override { return new FilterValidator(); } @@ -105,15 +110,10 @@ private: // Validator for a wxChoice with an Interframe value. class InterframeValidator : public widgets::OptionValidator { public: - InterframeValidator() : OptionValidator(config::OptionID::kDispIFB) { - Bind(wxEVT_CHOICE, &InterframeValidator::OnChoice, this); - } + InterframeValidator() : OptionValidator(config::OptionID::kDispIFB) {} ~InterframeValidator() override = default; private: - // wxChoice event handler. - void OnChoice(wxCommandEvent&) { WriteToOption(); } - // OptionValidator implementation. wxObject* Clone() const override { return new InterframeValidator(); } @@ -148,14 +148,10 @@ public: : OptionValidator(config::OptionID::kDispRenderMethod), render_method_(render_method) { assert(render_method != config::RenderMethod::kLast); - Bind(wxEVT_RADIOBUTTON, &RenderValidator::OnRadioButton, this); } ~RenderValidator() override = default; private: - // wxRadioButton event handler. - void OnRadioButton(wxCommandEvent&) { WriteToOption(); } - // OptionValidator implementation. wxObject* Clone() const override { return new RenderValidator(render_method_); @@ -307,11 +303,18 @@ DisplayConfig::DisplayConfig(wxWindow* parent) ->SetValidator(RenderValidator(config::RenderMethod::kOpenGL)); #endif // NO_OGL - // Direct3D is not implemented so hide the option on every platform. +#if defined(__WXMSW__) && !defined(NO_D3D) + // Enable the Direct3D option on Windows. + GetValidatedChild(this, "OutputDirect3D") + ->SetValidator(RenderValidator(config::RenderMethod::kDirect3d)); +#else GetValidatedChild(this, "OutputDirect3D")->Hide(); +#endif filter_selector_ = GetValidatedChild(this, "Filter"); filter_selector_->SetValidator(FilterValidator()); + filter_selector_->Bind(wxEVT_CHOICE, &DisplayConfig::UpdatePlugin, this, + GetId()); // These are filled and/or hidden at dialog load time. plugin_label_ = GetValidatedChild(this, "PluginLab"); @@ -320,14 +323,21 @@ DisplayConfig::DisplayConfig(wxWindow* parent) interframe_selector_ = GetValidatedChild(this, "IFB"); interframe_selector_->SetValidator(InterframeValidator()); - Bind(wxEVT_SHOW, &DisplayConfig::OnDialogShown, this, GetId()); - Bind(wxEVT_CLOSE_WINDOW, &DisplayConfig::OnDialogClosed, this, GetId()); + Bind(wxEVT_SHOW, &DisplayConfig::OnDialogShowEvent, this, GetId()); // Finally, fit everything nicely. Fit(); } -void DisplayConfig::OnDialogShown(wxShowEvent&) { +void DisplayConfig::OnDialogShowEvent(wxShowEvent& event) { + if (event.IsShown()) { + PopulatePluginOptions(); + } else { + StopPluginHandler(); + } +} + +void DisplayConfig::PopulatePluginOptions() { // Populate the plugin values, if any. wxArrayString plugins; const wxString plugin_path = wxGetApp().GetPluginsDir(); @@ -387,17 +397,21 @@ void DisplayConfig::OnDialogShown(wxShowEvent&) { Fit(); } -void DisplayConfig::OnDialogClosed(wxCloseEvent&) { - // Reset the validator to stop handling events while this dialog is not - // shown. +void DisplayConfig::StopPluginHandler() { + // Reset the validator to stop handling events while this dialog is hidden. plugin_selector_->SetValidator(wxValidator()); } +void DisplayConfig::UpdatePlugin(wxCommandEvent& event) { + const bool is_plugin = (static_cast(event.GetSelection()) == + config::Filter::kPlugin); + plugin_label_->Enable(is_plugin); + plugin_selector_->Enable(is_plugin); +} + void DisplayConfig::OnFilterChanged(config::Option* option) { const config::Filter option_filter = option->GetFilter(); const bool is_plugin = (option_filter == config::Filter::kPlugin); - plugin_label_->Enable(is_plugin); - plugin_selector_->Enable(is_plugin); // Plugin needs to be handled separately, in case it was removed. The panel // will eventually reset if it can't actually use the plugin. diff --git a/src/wx/dialogs/display-config.h b/src/wx/dialogs/display-config.h index 4f5561c5..d8790610 100644 --- a/src/wx/dialogs/display-config.h +++ b/src/wx/dialogs/display-config.h @@ -29,11 +29,17 @@ private: // owner, `parent` is destroyed. This prevents accidental deletion. DisplayConfig(wxWindow* parent); + // Handler for the wxEVT_SHOW event. + void OnDialogShowEvent(wxShowEvent& event); + // Populates the plugin options. - void OnDialogShown(wxShowEvent&); + void PopulatePluginOptions(); // Stops handling the plugin options. - void OnDialogClosed(wxCloseEvent&); + void StopPluginHandler(); + + // Update the plugin display. + void UpdatePlugin(wxCommandEvent& event); // Displays the new filter name on the screen. void OnFilterChanged(config::Option* option);