Merge pull request #189 from ZombieRaccoon/feature/event-listener-on-…

…attribute-change

Attach event listeners upon attribute changes

Include/RmlUi/Core/ElementUtilities.h

@@ -87,10 +87,6 @@ class RMLUICORE_API ElementUtilities
 	/// @return The string width, in pixels.
 	static int GetStringWidth(Element* element, const String& string, Character prior_character = Character::Null);
 
-	/// Bind and instance all event attributes on the given element onto the element
-	/// @param element Element to bind events on
-	static void BindEventAttributes(Element* element);
-
 	/// Generates the clipping region for an element.
 	/// @param[out] clip_origin The origin, in context coordinates, of the origin of the element's clipping window.
 	/// @param[out] clip_dimensions The size, in context coordinates, of the element's clipping window.

Source/Core/Context.cpp

@@ -270,8 +270,6 @@ ElementDocument* Context::LoadDocument(Stream* stream)
 
 	root->AppendChild(std::move(element));
 
-	ElementUtilities::BindEventAttributes(document);
-
 	// The 'load' event is fired before updating the document, because the user might
 	// need to initalize things before running an update. The drawback is that computed
 	// values and layouting are not performed yet, resulting in default values when

Source/Core/Element.cpp

@@ -73,6 +73,7 @@ static constexpr int ChildNotifyLevels = 2;
 struct ElementMeta
 {
 	ElementMeta(Element* el) : event_dispatcher(el), style(el), background_border(el), decoration(el), scroll(el) {}
+	SmallUnorderedMap<EventId, EventListener*> attribute_event_listeners;
 	EventDispatcher event_dispatcher;
 	ElementStyle style;
 	ElementBackgroundBorder background_border;
@@ -1200,14 +1201,14 @@ void Element::Click()
 }
 
 // Adds an event listener
-void Element::AddEventListener(const String& event, EventListener* listener, bool in_capture_phase)
+void Element::AddEventListener(const String& event, EventListener* listener, const bool in_capture_phase)
 {
-	EventId id = EventSpecificationInterface::GetIdOrInsert(event);
+	const EventId id = EventSpecificationInterface::GetIdOrInsert(event);
 	meta->event_dispatcher.AttachEvent(id, listener, in_capture_phase);
 }
 
 // Adds an event listener
-void Element::AddEventListener(EventId id, EventListener* listener, bool in_capture_phase)
+void Element::AddEventListener(const EventId id, EventListener* listener, const bool in_capture_phase)
 {
 	meta->event_dispatcher.AttachEvent(id, listener, in_capture_phase);
 }
@@ -1659,48 +1660,66 @@ void Element::OnStyleSheetChange()
 // Called when attributes on the element are changed.
 void Element::OnAttributeChange(const ElementAttributes& changed_attributes)
 {
-	auto it = changed_attributes.find("id");
-	if (it != changed_attributes.end())
-	{
-		id = it->second.Get<String>();
-		meta->style.DirtyDefinition();
-	}
-
-	it = changed_attributes.find("class");
-	if (it != changed_attributes.end())
-	{
-		meta->style.SetClassNames(it->second.Get<String>());
-	}
-
-	if (changed_attributes.count("colspan") || changed_attributes.count("rowspan"))
+	for (const auto& element_attribute : changed_attributes)
 	{
-		if (meta->computed_values.display == Style::Display::TableCell)
-			DirtyLayout();
-	}
-
-	if (changed_attributes.count("span"))
-	{
-		if (meta->computed_values.display == Style::Display::TableColumn || meta->computed_values.display == Style::Display::TableColumnGroup)
+		const auto& attribute = element_attribute.first;
+		const auto& value = element_attribute.second;
+		if (attribute == "id")
+		{
+			id = value.Get<String>();
+			meta->style.DirtyDefinition();
+		}
+		else if (attribute == "class")
+		{
+			meta->style.SetClassNames(value.Get<String>());
+		}
+		else if (((attribute == "colspan" || attribute == "rowspan") && meta->computed_values.display == Style::Display::TableCell)
+			|| (attribute == "span" && (meta->computed_values.display == Style::Display::TableColumn || meta->computed_values.display == Style::Display::TableColumnGroup)))
+		{
 			DirtyLayout();
-	}
-
-	it = changed_attributes.find("style");
-	if (it != changed_attributes.end())
-	{
-		if (it->second.GetType() == Variant::STRING)
+		}
+		else if (attribute.size() > 2 && attribute[0] == 'o' && attribute[1] == 'n')
 		{
-			PropertyDictionary properties;
-			StyleSheetParser parser;
-			parser.ParseProperties(properties, it->second.GetReference<String>());
+			static constexpr bool IN_CAPTURE_PHASE = false;
+
+			auto& attribute_event_listeners = meta->attribute_event_listeners;
+			auto& event_dispatcher = meta->event_dispatcher;
+			const auto event_id = EventSpecificationInterface::GetIdOrInsert(attribute.substr(2));
+			const auto remove_event_listener_if_exists = [&attribute_event_listeners, &event_dispatcher, event_id]()
+			{
+				const auto listener_it = attribute_event_listeners.find(event_id);
+				if (listener_it != attribute_event_listeners.cend())
+				{
+					event_dispatcher.DetachEvent(event_id, listener_it->second, IN_CAPTURE_PHASE);
+					attribute_event_listeners.erase(listener_it);
+				}
+			};
 
-			for (const auto& name_value : properties.GetProperties())
+			if (value.GetType() == Variant::Type::STRING)
 			{
-				meta->style.SetProperty(name_value.first, name_value.second);
+				remove_event_listener_if_exists();
+
+				const auto value_as_string = value.Get<String>();
+				auto insertion_result = attribute_event_listeners.emplace(event_id, Factory::InstanceEventListener(value_as_string, this));
+				if (auto* listener = insertion_result.first->second)
+					event_dispatcher.AttachEvent(event_id, listener, IN_CAPTURE_PHASE);
 			}
+			else if (value.GetType() == Variant::Type::NONE)
+				remove_event_listener_if_exists();
 		}
-		else if (it->second.GetType() != Variant::NONE)
+		else if (attribute == "style")
 		{
-			Log::Message(Log::LT_WARNING, "Invalid 'style' attribute, string type required. In element: %s", GetAddress().c_str());
+			if (value.GetType() == Variant::STRING)
+			{
+				PropertyDictionary properties;
+				StyleSheetParser parser;
+				parser.ParseProperties(properties, value.GetReference<String>());
+
+				for (const auto& name_value : properties.GetProperties())
+					meta->style.SetProperty(name_value.first, name_value.second);
+			}
+			else if (value.GetType() != Variant::NONE)
+				Log::Message(Log::LT_WARNING, "Invalid 'style' attribute, string type required. In element: %s", GetAddress().c_str());
 		}
 	}
 }

Source/Core/ElementUtilities.cpp

@@ -163,20 +163,6 @@ int ElementUtilities::GetStringWidth(Element* element, const String& string, Cha
 	return GetFontEngineInterface()->GetStringWidth(font_face_handle, string, prior_character);
 }
 
-void ElementUtilities::BindEventAttributes(Element* element)
-{
-	// Check for and instance the on* events
-	for (const auto& pair: element->GetAttributes())
-	{
-		if (pair.first.size() > 2 && pair.first[0] == 'o' && pair.first[1] == 'n')
-		{
-			EventListener* listener = Factory::InstanceEventListener(pair.second.Get<String>(), element);
-			if (listener)
-				element->AddEventListener(pair.first.substr(2), listener, false);
-		}
-	}
-}
-
 // Generates the clipping region for an element.
 bool ElementUtilities::GetClippingRegion(Vector2i& clip_origin, Vector2i& clip_dimensions, Element* element)
 {

Source/Core/EventDispatcher.cpp

@@ -33,7 +33,7 @@
 #include "../../Include/RmlUi/Core/Factory.h"
 #include "EventSpecification.h"
 #include <algorithm>
-#include <limits.h>
+#include <limits>
 
 namespace Rml {
 
@@ -50,8 +50,8 @@ struct CompareIdPhase {
 
 
 EventDispatcher::EventDispatcher(Element* _element)
+	: element(_element)
 {
-	element = _element;
 }
 
 EventDispatcher::~EventDispatcher()
@@ -61,36 +61,27 @@ EventDispatcher::~EventDispatcher()
 		event.listener->OnDetach(element);
 }
 
-void EventDispatcher::AttachEvent(EventId id, EventListener* listener, bool in_capture_phase)
+void EventDispatcher::AttachEvent(const EventId id, EventListener* listener, const bool in_capture_phase)
 {
-	EventListenerEntry entry(id, listener, in_capture_phase);
+	const EventListenerEntry entry(id, listener, in_capture_phase);
 
 	// The entries are sorted by (id,phase). Find the bounds of this sort, then find the entry.
-	auto range = std::equal_range(listeners.begin(), listeners.end(), entry, CompareIdPhase());
-	auto it = std::find(range.first, range.second, entry);
-
-	if(it == range.second)
+	const auto range = std::equal_range(listeners.cbegin(), listeners.cend(), entry, CompareIdPhase());
+	const auto matching_entry_it = std::find(range.first, range.second, entry);
+	if (matching_entry_it == range.second)
 	{
-		// No existing entry found, add it to the end of the (id, phase) range
-		listeners.emplace(it, entry);
+		listeners.emplace(range.second, entry);
 		listener->OnAttach(element);
 	}
 }
 
 
-void EventDispatcher::DetachEvent(EventId id, EventListener* listener, bool in_capture_phase)
+void EventDispatcher::DetachEvent(const EventId id, EventListener* listener, const bool in_capture_phase)
 {
-	EventListenerEntry entry(id, listener, in_capture_phase);
-
-	// The entries are sorted by (id,phase). Find the bounds of this sort, then find the entry.
-	// We could also just do a linear search over all the entries, which might be faster for low number of entries.
-	auto range = std::equal_range(listeners.begin(), listeners.end(), entry, CompareIdPhase());
-	auto it = std::find(range.first, range.second, entry);
-
-	if (it != range.second)
+	const auto listenerIt = std::find(listeners.cbegin(), listeners.cend(), EventListenerEntry(id, listener, in_capture_phase));
+	if (listenerIt != listeners.cend())
 	{
-		// We found our listener, remove it
-		listeners.erase(it);
+		listeners.erase(listenerIt);
 		listener->OnDetach(element);
 	}
 }
@@ -178,7 +169,7 @@ bool EventDispatcher::DispatchEvent(Element* target_element, const EventId id, c
 	if (!event)
 		return false;
 
-	int previous_sort_value = INT_MAX;
+	auto previous_sort_value = std::numeric_limits<int>::max();
 
 	// Process the event in each listener.
 	for (const auto& listener_desc : listeners)

Source/Core/EventDispatcher.h

@@ -39,7 +39,8 @@ class EventListener;
 struct CollectedListener;
 
 struct EventListenerEntry {
-	EventListenerEntry(EventId id, EventListener* listener, bool in_capture_phase) : id(id), in_capture_phase(in_capture_phase), listener(listener) {}
+	EventListenerEntry(const EventId id, EventListener* listener, const bool in_capture_phase) : id(id), in_capture_phase(in_capture_phase), listener(listener) {}
+
 	EventId id;
 	bool in_capture_phase;
 	EventListener* listener;
@@ -63,15 +64,15 @@ class EventDispatcher
 	/// Destructor
 	~EventDispatcher();
 
-	/// Attaches a new listener to the specified event name
-	/// @param[in] type Type of the event to attach to
-	/// @param[in] event_listener The event listener to be notified when the event fires
-	/// @param[in] in_capture_phase Should the listener be notified in the capture phase
+	/// Attaches a new listener to the specified event name.
+	/// @param[in] type Type of the event to attach to.
+	/// @param[in] event_listener The event listener to be notified when the event fires.
+	/// @param[in] in_capture_phase Should the listener be notified in the capture phase.
 	void AttachEvent(EventId id, EventListener* event_listener, bool in_capture_phase);
 
 	/// Detaches a listener from the specified event name
 	/// @param[in] type Type of the event to attach to
-	/// @para[in]m event_listener The event listener to be notified when the event fires
+	/// @param[in] event_listener The event listener to be notified when the event fires
 	/// @param[in] in_capture_phase Should the listener be notified in the capture phase
 	void DetachEvent(EventId id, EventListener* listener, bool in_capture_phase);
 

Source/Core/Factory.cpp

@@ -359,23 +359,16 @@ ElementInstancer* Factory::GetElementInstancer(const String& tag)
 // Instances a single element.
 ElementPtr Factory::InstanceElement(Element* parent, const String& instancer_name, const String& tag, const XMLAttributes& attributes)
 {
-	ElementInstancer* instancer = GetElementInstancer(instancer_name);
-
-	if (instancer)
+	if (ElementInstancer* instancer = GetElementInstancer(instancer_name))
 	{
-		ElementPtr element = instancer->InstanceElement(parent, tag, attributes);		
-
-		// Process the generic attributes and bind any events
-		if (element)
+		if (ElementPtr element = instancer->InstanceElement(parent, tag, attributes))
 		{
 			element->SetInstancer(instancer);
 			element->SetAttributes(attributes);
-			ElementUtilities::BindEventAttributes(element.get());
 
 			PluginRegistry::NotifyElementCreate(element.get());
+			return element;
 		}
-
-		return element;
 	}
 
 	return nullptr;

Tests/CMakeLists.txt

@@ -7,20 +7,21 @@ target_compile_definitions(RmlCore PUBLIC RMLUI_TESTS_ENABLED)
 #===================================
 # Include dependencies =============
 #===================================
-set(DOCTEST_INCLUDE_DIR ${CMAKE_CURRENT_SOURCE_DIR}/Dependencies/doctest)
-add_library(doctest::doctest IMPORTED INTERFACE)
-set_property(TARGET doctest::doctest PROPERTY INTERFACE_INCLUDE_DIRECTORIES "${DOCTEST_INCLUDE_DIR}")
+function(include_dependency NAME)
+	set(DEPENDENCY_INCLUDE_DIR ${CMAKE_CURRENT_SOURCE_DIR}/Dependencies/${NAME})
+	set(DEPENDENCY_TARGET ${NAME}::${NAME})
 
-# Include doctest's discovery module
-include(${DOCTEST_INCLUDE_DIR}/cmake/doctest.cmake)
+	add_library(${DEPENDENCY_TARGET} IMPORTED INTERFACE)
+	set_property(TARGET ${DEPENDENCY_TARGET} PROPERTY INTERFACE_INCLUDE_DIRECTORIES "${DEPENDENCY_INCLUDE_DIR}")
+endfunction()
 
-set(NANOBENCH_INCLUDE_DIR ${CMAKE_CURRENT_SOURCE_DIR}/Dependencies/nanobench)
-add_library(nanobench::nanobench IMPORTED INTERFACE)
-set_property(TARGET nanobench::nanobench PROPERTY INTERFACE_INCLUDE_DIRECTORIES "${NANOBENCH_INCLUDE_DIR}")
+include_dependency("doctest")
+# Include doctest's discovery module
+include(${CMAKE_CURRENT_SOURCE_DIR}/Dependencies/doctest/cmake/doctest.cmake)
 
-set(LODEPNG_INCLUDE_DIR ${CMAKE_CURRENT_SOURCE_DIR}/Dependencies/lodepng)
-add_library(lodepng::lodepng IMPORTED INTERFACE)
-set_property(TARGET lodepng::lodepng PROPERTY INTERFACE_INCLUDE_DIRECTORIES "${LODEPNG_INCLUDE_DIR}")
+include_dependency("nanobench")
+include_dependency("lodepng")
+include_dependency("trompeloeil")
 
 #===================================
 # Common source files ==============
@@ -37,7 +38,7 @@ file(GLOB UnitTests_HDR_FILES ${CMAKE_CURRENT_SOURCE_DIR}/Source/UnitTests/*.h )
 file(GLOB UnitTests_SRC_FILES ${CMAKE_CURRENT_SOURCE_DIR}/Source/UnitTests/*.cpp )
 
 add_executable(UnitTests ${UnitTests_HDR_FILES} ${UnitTests_SRC_FILES})
-target_link_libraries(UnitTests RmlCore doctest::doctest ${sample_LIBRARIES})
+target_link_libraries(UnitTests RmlCore doctest::doctest trompeloeil::trompeloeil ${sample_LIBRARIES})
 add_common_target_options(UnitTests)
 
 if(MSVC)

Tests/Dependencies/LICENSE.txt

@@ -74,3 +74,32 @@ AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
 LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
 OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
 SOFTWARE.
+
+-------------------------------------------------------------------------------
+Trompeloeil (BSL-1.0 License)
+https://github.com/rollbear/trompeloeil
+-------------------------------------------------------------------------------
+
+Boost Software License - Version 1.0 - August 17th, 2003
+
+Permission is hereby granted, free of charge, to any person or organization
+obtaining a copy of the software and accompanying documentation covered by
+this license (the "Software") to use, reproduce, display, distribute,
+execute, and transmit the Software, and to prepare derivative works of the
+Software, and to permit third-parties to whom the Software is furnished to
+do so, all subject to the following:
+
+The copyright notices in the Software and this entire statement, including
+the above license grant, this restriction and the following disclaimer,
+must be included in all copies of the Software, in whole or in part, and
+all derivative works of the Software, unless such copies or derivative
+works are solely in the form of machine-executable object code generated by
+a source language processor.
+
+THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+FITNESS FOR A PARTICULAR PURPOSE, TITLE AND NON-INFRINGEMENT. IN NO EVENT
+SHALL THE COPYRIGHT HOLDERS OR ANYONE DISTRIBUTING THE SOFTWARE BE LIABLE
+FOR ANY DAMAGES OR OTHER LIABILITY, WHETHER IN CONTRACT, TORT OR OTHERWISE,
+ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
+DEALINGS IN THE SOFTWARE.