From 0f0dc9bcb4edb4e007f9c792e9de0cbb4965e604 Mon Sep 17 00:00:00 2001 From: ZombieRaccoon Date: Fri, 20 Aug 2021 08:32:49 +0200 Subject: [PATCH 1/2] Overcome a limitation of binding data to checkboxes --- Source/Core/DataControllerDefault.cpp | 67 ++++++---------------- Source/Core/DataControllerDefault.h | 26 +++++---- Source/Core/Elements/InputTypeCheckbox.cpp | 16 +++--- Source/Core/Elements/InputTypeRadio.cpp | 15 ++--- Source/Core/Factory.cpp | 8 +-- 5 files changed, 51 insertions(+), 81 deletions(-) diff --git a/Source/Core/DataControllerDefault.cpp b/Source/Core/DataControllerDefault.cpp index 9f863bf06..43655ed44 100644 --- a/Source/Core/DataControllerDefault.cpp +++ b/Source/Core/DataControllerDefault.cpp @@ -35,18 +35,17 @@ namespace Rml { -DataControllerValue::DataControllerValue(Element* element) : DataController(element) +DataControllerAttributeBased::DataControllerAttributeBased(Element* element, String attribute) + : DataController(element), attribute(std::move(attribute)) {} -DataControllerValue::~DataControllerValue() +DataControllerAttributeBased::~DataControllerAttributeBased() { if (Element* element = GetElement()) - { element->RemoveEventListener(EventId::Change, this); - } } -bool DataControllerValue::Initialize(DataModel& model, Element* element, const String& variable_name, const String& /*modifier*/) +bool DataControllerAttributeBased::Initialize(DataModel& model, Element* element, const String& variable_name, const String& /*modifier*/) { RMLUI_ASSERT(element); @@ -62,74 +61,40 @@ bool DataControllerValue::Initialize(DataModel& model, Element* element, const S return true; } -void DataControllerValue::ProcessEvent(Event& event) +void DataControllerAttributeBased::ProcessEvent(Event& event) { - if (Element* element = GetElement()) + if (const Element* element = GetElement()) { const auto& parameters = event.GetParameters(); - - auto it = parameters.find("value"); - if (it == parameters.end()) + const auto it = parameters.find(attribute); + if (it == parameters.cend()) { - Log::Message(Log::LT_WARNING, "A 'change' event was received, but it did not contain a value. During processing of 'data-value' in %s", element->GetAddress().c_str()); + Log::Message(Log::LT_WARNING, "A 'change' event was received, but it did not contain the attribute '%s' when processing 'data-%s' in %s", + attribute.c_str(), attribute.c_str(), element->GetAddress().c_str()); return; } + const auto affected_it = parameters.find("should_affect_data_binding"); + if (affected_it != parameters.cend() && !affected_it->second.Get()) + return; + DataModel* model = element->GetDataModel(); if (!model) return; if (DataVariable variable = model->GetVariable(address)) { - if (SetValue(it->second, variable)) + if (variable.Set(it->second)) model->DirtyVariable(address.front().name); } } } -void DataControllerValue::Release() +void DataControllerAttributeBased::Release() { delete this; } -bool DataControllerValue::SetValue(const Variant& value, DataVariable variable) -{ - return variable.Set(value); -} - - -DataControllerChecked::DataControllerChecked(Element* element) : DataControllerValue(element) -{} - - -bool DataControllerChecked::SetValue(const Variant& value, DataVariable variable) -{ - bool result = false; - Variant old_value; - - if (variable.Get(old_value)) - { - // Value will be empty if the button was just unchecked, otherwise it will take the 'value' attribute. - const String new_value = value.Get(); - - if (old_value.GetType() == Variant::BOOL) - { - // If the client variable is a boolean type, we assume the button acts like a checkbox, and set the new checked state. - result = variable.Set(Variant(!new_value.empty())); - } - else - { - // Otherwise, we assume the button acts like a radio box. Then, we do nothing if the box was unchecked, - // and instead let only the newly checked box set the new value. - if (!new_value.empty()) - result = variable.Set(value); - } - } - - return result; -} - - DataControllerEvent::DataControllerEvent(Element* element) : DataController(element) {} diff --git a/Source/Core/DataControllerDefault.h b/Source/Core/DataControllerDefault.h index 9b670a414..1d48ef4b0 100644 --- a/Source/Core/DataControllerDefault.h +++ b/Source/Core/DataControllerDefault.h @@ -42,33 +42,37 @@ class DataModel; class DataExpression; using DataExpressionPtr = UniquePtr; - -class DataControllerValue : public DataController, private EventListener { +class DataControllerAttributeBased : public DataController, private EventListener { public: - DataControllerValue(Element* element); - ~DataControllerValue(); + DataControllerAttributeBased(Element* element, String attribute); + ~DataControllerAttributeBased(); bool Initialize(DataModel& model, Element* element, const String& expression, const String& modifier) override; -protected: +private: // Responds to 'Change' events. void ProcessEvent(Event& event) override; // Delete this. void Release() override; - // Set the new value on the variable, returns true if it should be dirtied. - virtual bool SetValue(const Variant& new_value, DataVariable variable); - DataAddress address; + String attribute; }; -class DataControllerChecked final : public DataControllerValue { +class DataControllerAttributeBasedInstancer final : public DataControllerInstancer { public: - DataControllerChecked(Element* element); + DataControllerAttributeBasedInstancer(String attribute) + : attribute(std::move(attribute)) + {} + + DataControllerPtr InstanceController(Element* element) override { + return DataControllerPtr(new DataControllerAttributeBased(element, attribute)); + } - bool SetValue(const Variant& new_value, DataVariable variable) override; +private: + String attribute; }; diff --git a/Source/Core/Elements/InputTypeCheckbox.cpp b/Source/Core/Elements/InputTypeCheckbox.cpp index 89ec86946..7159e97b7 100644 --- a/Source/Core/Elements/InputTypeCheckbox.cpp +++ b/Source/Core/Elements/InputTypeCheckbox.cpp @@ -48,15 +48,16 @@ bool InputTypeCheckbox::IsSubmitted() // Checks for necessary functional changes in the control as a result of changed attributes. bool InputTypeCheckbox::OnAttributeChange(const ElementAttributes& changed_attributes) { - // Check if maxlength has been defined. - if (changed_attributes.find("checked") != changed_attributes.end()) + if (changed_attributes.count("checked")) { - bool checked = element->HasAttribute("checked"); + const bool checked = element->HasAttribute("checked"); element->SetPseudoClass("checked", checked); - Dictionary parameters; - parameters["value"] = String(checked ? GetValue() : ""); - element->DispatchEvent(EventId::Change, parameters); + const auto value = GetValue(); + element->DispatchEvent(EventId::Change, { + { "checked", Variant(checked) }, + { "value", Variant(checked ? (value.empty() ? "on" : value) : "") } + }); } return true; @@ -65,8 +66,7 @@ bool InputTypeCheckbox::OnAttributeChange(const ElementAttributes& changed_attri // Checks for necessary functional changes in the control as a result of the event. void InputTypeCheckbox::ProcessDefaultAction(Event& event) { - if (event == EventId::Click && - !element->IsDisabled()) + if (event == EventId::Click && !element->IsDisabled()) { if (element->HasAttribute("checked")) element->RemoveAttribute("checked"); diff --git a/Source/Core/Elements/InputTypeRadio.cpp b/Source/Core/Elements/InputTypeRadio.cpp index ece97513b..6c019d3d2 100644 --- a/Source/Core/Elements/InputTypeRadio.cpp +++ b/Source/Core/Elements/InputTypeRadio.cpp @@ -52,8 +52,7 @@ bool InputTypeRadio::IsSubmitted() // Checks for necessary functional changes in the control as a result of changed attributes. bool InputTypeRadio::OnAttributeChange(const ElementAttributes& changed_attributes) { - // Check if maxlength has been defined. - if (changed_attributes.find("checked") != changed_attributes.end()) + if (changed_attributes.count("checked")) { bool checked = element->HasAttribute("checked"); element->SetPseudoClass("checked", checked); @@ -61,9 +60,12 @@ bool InputTypeRadio::OnAttributeChange(const ElementAttributes& changed_attribut if (checked) PopRadioSet(); - Dictionary parameters; - parameters["value"] = String(checked ? GetValue() : ""); - element->DispatchEvent(EventId::Change, parameters); + const auto perceived_value = Variant(checked ? GetValue() : ""); + element->DispatchEvent(EventId::Change, { + { "checked", perceived_value }, + { "should_affect_data_binding", Variant(checked) }, + { "value", perceived_value } + }); } return true; @@ -79,8 +81,7 @@ void InputTypeRadio::OnChildAdd() // Checks for necessary functional changes in the control as a result of the event. void InputTypeRadio::ProcessDefaultAction(Event& event) { - if (event == EventId::Click && - !element->IsDisabled()) + if (event == EventId::Click && !element->IsDisabled()) element->SetAttribute("checked", ""); } diff --git a/Source/Core/Factory.cpp b/Source/Core/Factory.cpp index 4d3344ae4..3ba22d91c 100644 --- a/Source/Core/Factory.cpp +++ b/Source/Core/Factory.cpp @@ -186,9 +186,9 @@ struct DefaultInstancers { DataViewInstancerDefault structural_data_view_for; // Data binding controllers - DataControllerInstancerDefault data_controller_value; + DataControllerAttributeBasedInstancer data_controller_checked { "checked" }; DataControllerInstancerDefault data_controller_event; - DataControllerInstancerDefault data_controller_checked; + DataControllerAttributeBasedInstancer data_controller_value { "value" }; }; static UniquePtr default_instancers; @@ -279,9 +279,9 @@ bool Factory::Initialise() RegisterDataViewInstancer(&default_instancers->structural_data_view_for, "for", true ); // Data binding controllers - RegisterDataControllerInstancer(&default_instancers->data_controller_value, "value"); - RegisterDataControllerInstancer(&default_instancers->data_controller_event, "event"); RegisterDataControllerInstancer(&default_instancers->data_controller_checked, "checked"); + RegisterDataControllerInstancer(&default_instancers->data_controller_event, "event"); + RegisterDataControllerInstancer(&default_instancers->data_controller_value, "value"); // XML node handlers XMLParser::RegisterNodeHandler("", MakeShared()); From 7196ee7fe09077ed050b8134edcd73e943a330db Mon Sep 17 00:00:00 2001 From: ZombieRaccoon Date: Sun, 22 Aug 2021 15:12:28 +0200 Subject: [PATCH 2/2] Implement review comments --- Source/Core/DataControllerDefault.cpp | 39 ++++++++++------------ Source/Core/DataControllerDefault.h | 22 ++---------- Source/Core/Elements/InputTypeCheckbox.cpp | 12 ++++--- Source/Core/Elements/InputTypeCheckbox.h | 4 +++ Source/Core/Elements/InputTypeRadio.cpp | 9 +++-- Source/Core/Elements/InputTypeRadio.h | 4 +++ Source/Core/Factory.cpp | 5 ++- 7 files changed, 46 insertions(+), 49 deletions(-) diff --git a/Source/Core/DataControllerDefault.cpp b/Source/Core/DataControllerDefault.cpp index 43655ed44..24ad1e443 100644 --- a/Source/Core/DataControllerDefault.cpp +++ b/Source/Core/DataControllerDefault.cpp @@ -35,17 +35,17 @@ namespace Rml { -DataControllerAttributeBased::DataControllerAttributeBased(Element* element, String attribute) - : DataController(element), attribute(std::move(attribute)) +DataControllerValue::DataControllerValue(Element* element) + : DataController(element) {} -DataControllerAttributeBased::~DataControllerAttributeBased() +DataControllerValue::~DataControllerValue() { if (Element* element = GetElement()) element->RemoveEventListener(EventId::Change, this); } -bool DataControllerAttributeBased::Initialize(DataModel& model, Element* element, const String& variable_name, const String& /*modifier*/) +bool DataControllerValue::Initialize(DataModel& model, Element* element, const String& variable_name, const String& /*modifier*/) { RMLUI_ASSERT(element); @@ -61,36 +61,33 @@ bool DataControllerAttributeBased::Initialize(DataModel& model, Element* element return true; } -void DataControllerAttributeBased::ProcessEvent(Event& event) +void DataControllerValue::ProcessEvent(Event& event) { if (const Element* element = GetElement()) { + Variant value_to_set; const auto& parameters = event.GetParameters(); - const auto it = parameters.find(attribute); - if (it == parameters.cend()) - { - Log::Message(Log::LT_WARNING, "A 'change' event was received, but it did not contain the attribute '%s' when processing 'data-%s' in %s", - attribute.c_str(), attribute.c_str(), element->GetAddress().c_str()); - return; - } - - const auto affected_it = parameters.find("should_affect_data_binding"); - if (affected_it != parameters.cend() && !affected_it->second.Get()) - return; + const auto override_value_it = parameters.find("data-binding-override-value"); + const auto value_it = parameters.find("value"); + if (override_value_it != parameters.cend()) + value_to_set = override_value_it->second; + else if (value_it != parameters.cend()) + value_to_set = value_it->second; + else + Log::Message(Log::LT_WARNING, "A 'change' event was received, but it did not contain the attribute 'value' when processing a data binding in %s", + element->GetAddress().c_str()); DataModel* model = element->GetDataModel(); - if (!model) + if (value_to_set.GetType() == Variant::NONE || !model) return; if (DataVariable variable = model->GetVariable(address)) - { - if (variable.Set(it->second)) + if (variable.Set(value_to_set)) model->DirtyVariable(address.front().name); - } } } -void DataControllerAttributeBased::Release() +void DataControllerValue::Release() { delete this; } diff --git a/Source/Core/DataControllerDefault.h b/Source/Core/DataControllerDefault.h index 1d48ef4b0..caac0c084 100644 --- a/Source/Core/DataControllerDefault.h +++ b/Source/Core/DataControllerDefault.h @@ -42,10 +42,10 @@ class DataModel; class DataExpression; using DataExpressionPtr = UniquePtr; -class DataControllerAttributeBased : public DataController, private EventListener { +class DataControllerValue : public DataController, private EventListener { public: - DataControllerAttributeBased(Element* element, String attribute); - ~DataControllerAttributeBased(); + DataControllerValue(Element* element); + ~DataControllerValue(); bool Initialize(DataModel& model, Element* element, const String& expression, const String& modifier) override; @@ -57,22 +57,6 @@ class DataControllerAttributeBased : public DataController, private EventListene void Release() override; DataAddress address; - String attribute; -}; - - -class DataControllerAttributeBasedInstancer final : public DataControllerInstancer { -public: - DataControllerAttributeBasedInstancer(String attribute) - : attribute(std::move(attribute)) - {} - - DataControllerPtr InstanceController(Element* element) override { - return DataControllerPtr(new DataControllerAttributeBased(element, attribute)); - } - -private: - String attribute; }; diff --git a/Source/Core/Elements/InputTypeCheckbox.cpp b/Source/Core/Elements/InputTypeCheckbox.cpp index 7159e97b7..d5c701c20 100644 --- a/Source/Core/Elements/InputTypeCheckbox.cpp +++ b/Source/Core/Elements/InputTypeCheckbox.cpp @@ -39,6 +39,12 @@ InputTypeCheckbox::~InputTypeCheckbox() { } +String InputTypeCheckbox::GetValue() const +{ + auto value = InputType::GetValue(); + return value.empty() ? "on" : value; +} + // Returns if this value should be submitted with the form. bool InputTypeCheckbox::IsSubmitted() { @@ -52,11 +58,9 @@ bool InputTypeCheckbox::OnAttributeChange(const ElementAttributes& changed_attri { const bool checked = element->HasAttribute("checked"); element->SetPseudoClass("checked", checked); - - const auto value = GetValue(); element->DispatchEvent(EventId::Change, { - { "checked", Variant(checked) }, - { "value", Variant(checked ? (value.empty() ? "on" : value) : "") } + { "data-binding-override-value", Variant(checked) }, + { "value", Variant(checked ? GetValue() : "") } }); } diff --git a/Source/Core/Elements/InputTypeCheckbox.h b/Source/Core/Elements/InputTypeCheckbox.h index 981658600..0f72fd2cc 100644 --- a/Source/Core/Elements/InputTypeCheckbox.h +++ b/Source/Core/Elements/InputTypeCheckbox.h @@ -45,6 +45,10 @@ class InputTypeCheckbox : public InputType InputTypeCheckbox(ElementFormControlInput* element); virtual ~InputTypeCheckbox(); + /// Returns a string representation of the current value of the form control. + /// @return The value of the form control. + String GetValue() const override; + /// Returns if this value should be submitted with the form. /// @return True if the form control is to be submitted, false otherwise. bool IsSubmitted() override; diff --git a/Source/Core/Elements/InputTypeRadio.cpp b/Source/Core/Elements/InputTypeRadio.cpp index 6c019d3d2..63554cdab 100644 --- a/Source/Core/Elements/InputTypeRadio.cpp +++ b/Source/Core/Elements/InputTypeRadio.cpp @@ -43,6 +43,12 @@ InputTypeRadio::~InputTypeRadio() { } +String InputTypeRadio::GetValue() const +{ + auto value = InputType::GetValue(); + return value.empty() ? "on" : value; +} + // Returns if this value should be submitted with the form. bool InputTypeRadio::IsSubmitted() { @@ -62,8 +68,7 @@ bool InputTypeRadio::OnAttributeChange(const ElementAttributes& changed_attribut const auto perceived_value = Variant(checked ? GetValue() : ""); element->DispatchEvent(EventId::Change, { - { "checked", perceived_value }, - { "should_affect_data_binding", Variant(checked) }, + { "data-binding-override-value", checked ? Variant(perceived_value) : Variant() }, { "value", perceived_value } }); } diff --git a/Source/Core/Elements/InputTypeRadio.h b/Source/Core/Elements/InputTypeRadio.h index 956e8b752..a82b25aa6 100644 --- a/Source/Core/Elements/InputTypeRadio.h +++ b/Source/Core/Elements/InputTypeRadio.h @@ -45,6 +45,10 @@ class InputTypeRadio : public InputType InputTypeRadio(ElementFormControlInput* element); virtual ~InputTypeRadio(); + /// Returns a string representation of the current value of the form control. + /// @return The value of the form control. + String GetValue() const override; + /// Returns if this value should be submitted with the form. /// @return True if the form control is to be submitted, false otherwise. bool IsSubmitted() override; diff --git a/Source/Core/Factory.cpp b/Source/Core/Factory.cpp index 3ba22d91c..0779e758e 100644 --- a/Source/Core/Factory.cpp +++ b/Source/Core/Factory.cpp @@ -186,9 +186,8 @@ struct DefaultInstancers { DataViewInstancerDefault structural_data_view_for; // Data binding controllers - DataControllerAttributeBasedInstancer data_controller_checked { "checked" }; DataControllerInstancerDefault data_controller_event; - DataControllerAttributeBasedInstancer data_controller_value { "value" }; + DataControllerInstancerDefault data_controller_value; }; static UniquePtr default_instancers; @@ -279,7 +278,7 @@ bool Factory::Initialise() RegisterDataViewInstancer(&default_instancers->structural_data_view_for, "for", true ); // Data binding controllers - RegisterDataControllerInstancer(&default_instancers->data_controller_checked, "checked"); + RegisterDataControllerInstancer(&default_instancers->data_controller_value, "checked"); RegisterDataControllerInstancer(&default_instancers->data_controller_event, "event"); RegisterDataControllerInstancer(&default_instancers->data_controller_value, "value");