Overcome a limitation of binding data variables to checkboxes (#214)

* Overcome a limitation of binding data to checkboxes

* Implement review comments

Co-authored-by: ZombieRaccoon <[email protected]>

Source/Core/DataControllerDefault.cpp

@@ -35,15 +35,14 @@
 
 namespace Rml {
 
-DataControllerValue::DataControllerValue(Element* element) : DataController(element)
+DataControllerValue::DataControllerValue(Element* element)
+	: DataController(element)
 {}
 
 DataControllerValue::~DataControllerValue()
 {
 	if (Element* element = GetElement())
-	{
 		element->RemoveEventListener(EventId::Change, this);
-	}
 }
 
 bool DataControllerValue::Initialize(DataModel& model, Element* element, const String& variable_name, const String& /*modifier*/)
@@ -64,26 +63,27 @@ bool DataControllerValue::Initialize(DataModel& model, Element* element, const S
 
 void DataControllerValue::ProcessEvent(Event& event)
 {
-	if (Element* element = GetElement())
+	if (const Element* element = GetElement())
 	{
+		Variant value_to_set;
 		const auto& parameters = event.GetParameters();
-
-		auto it = parameters.find("value");
-		if (it == parameters.end())
-		{
-			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());
-			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 (SetValue(it->second, variable))
+			if (variable.Set(value_to_set))
 				model->DirtyVariable(address.front().name);
-		}
 	}
 }
 
@@ -92,44 +92,6 @@ void DataControllerValue::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<String>();
-
-		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)
 {}

Source/Core/DataControllerDefault.h

@@ -42,7 +42,6 @@ class DataModel;
 class DataExpression;
 using DataExpressionPtr = UniquePtr<DataExpression>;
 
-
 class DataControllerValue : public DataController, private EventListener {
 public:
     DataControllerValue(Element* element);
@@ -50,28 +49,17 @@ public:
 
     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;
 };
 
 
-class DataControllerChecked final : public DataControllerValue {
-public:
-    DataControllerChecked(Element* element);
-
-    bool SetValue(const Variant& new_value, DataVariable variable) override;
-};
-
-
 class DataControllerEvent final : public DataController, private EventListener {
 public:
     DataControllerEvent(Element* element);

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()
 {
@@ -48,15 +54,14 @@ 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);
+		element->DispatchEvent(EventId::Change, {
+			{ "data-binding-override-value", Variant(checked) },
+			{ "value", Variant(checked ? GetValue() : "") }
+		});
 	}
 
 	return true;
@@ -65,8 +70,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");

Source/Core/Elements/InputTypeCheckbox.h

@@ -45,6 +45,10 @@ public:
 	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;

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()
 {
@@ -52,8 +58,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 +66,11 @@ 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, {
+			{ "data-binding-override-value", checked ? Variant(perceived_value) : Variant() },
+			{ "value", perceived_value }
+		});
 	}
 
 	return true;
@@ -79,8 +86,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", "");
 }
 

Source/Core/Elements/InputTypeRadio.h

@@ -45,6 +45,10 @@ public:
 	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;

Source/Core/Factory.cpp

@@ -186,9 +186,8 @@ struct DefaultInstancers {
 	DataViewInstancerDefault<DataViewFor> structural_data_view_for;
 
 	// Data binding controllers
-	DataControllerInstancerDefault<DataControllerValue> data_controller_value;
 	DataControllerInstancerDefault<DataControllerEvent> data_controller_event;
-	DataControllerInstancerDefault<DataControllerChecked> data_controller_checked;
+	DataControllerInstancerDefault<DataControllerValue> data_controller_value;
 };
 
 static UniquePtr<DefaultInstancers> default_instancers;
@@ -279,9 +278,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_value, "checked");
 	RegisterDataControllerInstancer(&default_instancers->data_controller_event, "event");
-	RegisterDataControllerInstancer(&default_instancers->data_controller_checked, "checked");
+	RegisterDataControllerInstancer(&default_instancers->data_controller_value, "value");
 
 	// XML node handlers
 	XMLParser::RegisterNodeHandler("", MakeShared<XMLNodeHandlerDefault>());