Most of #317; each Inputwidget sets "changed" finally

...but updated() is still a misnomer, and the whole update follow-up
logic is pretty immature/convoluted/unconvincing yet.

include/sfw/InputWidget.hpp

@@ -7,18 +7,59 @@ namespace sfw
 {
 
 template <class W> class InputWidget : public Widget
-/*==============================================================================
-  This "CRTP utility class" is an intermediate level in the widget class
-  hierarchy, solely to overcome some C++ limitations (i.e. to support templated
-  functions that would otherwise be unable to deduce their "actual widget type"
-  parameter (W)).
-
-  NOTE:
-    "Interactive" widgets (that can have user callbacks, support the update(...)
+/*****************************************************************************
+  Base class of widgets that are intended to receive user input (data)
+  
+  (This is implemented as a CRTP class solely to overcome a C++ limitation to
+  support templated functions that would otherwise be unable to deduce their
+  "actual widget" template parameter (W).)
+
+  Notes on the "input thing":
+
+  - Input widgets (that can have user callbacks, support the update(...)
     API etc.) should derive from this class (rather than Widget directly).
-===============================================================================*/
+
+  - Every input widget has a dedicated value of a corresponding type that
+    the widget represents (models, stores, displays etc.).
+
+    This value can be set via user input actions, or programmatically by the
+    constructor, and a dedicated set(...) method (and its overloads, if any).
+ 
+    The current value can be queried with the widget's get() method.
+    (Unfortunately C++ doesn't support overloading on return types, so for
+    outward type conversions, separate getXxx() methods may be defined.
+    However, it's better to define the widget's data type explicitly, with
+    conversions in both directions (converting ctors and "cast operators"),
+    and then just use a single set/get method.)
+
+  - Input widgets are typically configured to have callbacks to be invoked
+    when the widget's value gets updated (with the possibility of still
+    ignoring unimportant interim changes).
+******************************************************************************/
 {
 public:
+	InputWidget() :
+		m_changed(true) // Initializing counts as a change on its own right!
+		                // (So, this ensures proper update followups even if
+				// an initial set() can't detect a change, e.g. because
+				// there's no "invalid" value in the type, or because
+				// set() ism't even called by the widget ctor (which is
+				// no longer assumed/required -- exactly because of this
+				// init safeguard...))!
+	{}
+
+	// Tracking the value of the widget
+	// Derived real widgets will need to define getter/setters:
+	// - set(V value); // Will call changed() as applicable
+	// - V get();
+	bool changed() const { return m_changed; }
+	Widget* setChanged(bool newstate = true) { m_changed = newstate; return this; }
+		//!!Rename this to a) not be so ambiguous with the above (const) one, and
+		//!!b) be in line with other similar ones (like enable/disable etc.)!...
+		//!!But change() here would be awkward -- even resorting to setChanged()
+		//!!feels better, but then it's out-of-style (for the new API) again...)
+
+
 	template <typename V> W* update(V value) {
 		((W*)this)->set(value); // set() is not virtual, must downcast to find it! (Also, `W::set(...)` could be too rigid, I think.)
 		                        //!!NOTE: C++ won't find W::set() just via `set(value);` (even if no Widget::set())! :-/
@@ -43,7 +84,7 @@ template <class W> class InputWidget : public Widget
 		                              //!! a) it can be configured "orthogonally", and b) we'd have
 		                              //!! one less decision to make here.
 
-		changed(false);
+		setChanged(false);
 			//!!?? We either clear the `changed` flag here, so that it won't get stuck (note:
 			//!!?? set() isn't supposed to clear it, to allow batch updates!), and then it can't
 			//!!?? be used to track an overarching "changed" state across these update callbacks,
@@ -52,8 +93,8 @@ template <class W> class InputWidget : public Widget
 		return (W*)this;
 	}
 
-	//!! KINDA DEPRECATED...:
-	// Set callback for Update events
+	//!!SHOULD BE DEPRECATED, in favor of on(...):
+	// Set callback for the Updated events
 	// 1. Applies to interactive (input) widgets only.
 	// 2. The callback is not called for volatile interim state changes
 	//    (like typing/deleting chars to/from a text editor), but only when
@@ -80,6 +121,39 @@ template <class W> class InputWidget : public Widget
 	// {
 	//     return (SomeWidget*) Widget::setCallback(callback);
 	// }
+
+	//!!This actually belongs to InputWidget (`protected` exactly for it to be visible
+	//!!there), but not sure how a virtual would mix with that being a template!...
+	//!!Also, it's not even just a dummy callback like the others, but a dispatcher! :-/
+	//!!Needs cleanup...
+	//!! -> Actually, as a generic "callback invoker", even up in EventHandler, like
+	//!!    EventHandler::invoke_callback(which), could hit all the birds with one stone.
+protected:
+
+// Virtuals/Callbacks --------------------------------------------------------
+	virtual void onUpdated() //!! -> "EventHandler::invoke_callback(Updated)", and "updated" -> "notify(Updated)" or sg...
+	{
+		using namespace Event::internal;
+
+		auto callback = m_callbackMap[Event::Update];
+		assert( std::holds_alternative<Callback_w>(callback) || std::holds_alternative<Callback_void>(callback) );
+			//!!?? This may no longer be true after getting something from the map that may not have
+			//!!?? been there! Seems to still work, so probably true, but make it explicit whether the
+			//!!?? map would indeed return a false std::function, properly wrapped in optional<>!...
+
+		if (std::holds_alternative<Callback_w>(callback) && std::get<Callback_w>(callback))
+		{
+			return (std::get<Callback_w>(callback).value()) (this);
+		}
+		else if (std::holds_alternative<Callback_void>(callback) && std::get<Callback_void>(callback))
+		{
+			return (std::get<Callback_void>(callback).value()) ();
+		}
+	}
+
+
+private:
+	bool m_changed = false;
 };
 
 

include/sfw/Widget.hpp

@@ -32,24 +32,11 @@ class Tooltip;
 class GUI;
 
 /*****************************************************************************
-  Abstract base class for widgets
-
-  - Every (interactive) widget has a dedicated value of a corresponding
-    type that the widget represents (models, stores, displays etc.).
-
-    This value can be set via user input actions, or programmatically by the
-    constructor, and a dedicated set(...) method (and its overloads, if any).
- 
-    The current value can be queried with the widget's get() method.
-    (Unfortunately C++ doesn't support overloading on return types, so for
-    outward type conversions, separate getXxx() methods may be defined.
-    However, it's better to define the widget's data type explicitly, with
-    conversions in both directions (converting ctors and "cast operators"),
-    and then just use a single set/get method.)
-
-  - Interactive widgets are typically configured to have callbacks to be
-    invoked when the widget's value gets updated (and finalized/committed,
-    so interim changes can be ignored).
+
+    Abstract base for widgets
+
+    See InputWidget as the base for "actually useful" widgets, though. ;)
+
  *****************************************************************************/
 class Widget : public gfx::Drawable, public Event::Handler
 {
@@ -64,17 +51,6 @@ class Widget : public gfx::Drawable, public Event::Handler
 	// Check if a point belongs to the widget
 	bool contains(const sf::Vector2f& point) const;
 
-	// Tracking the value of the widget
-	// Derived real widgets will need to define getter/setters:
-	// - set(V value); // Will call changed() as applicable
-	// - V get();
-	bool changed() const { return m_changed; }
-	Widget* changed(bool newstate = true) { m_changed = newstate; return this; }
-		//!!Rename this to a) not be so ambiguous with the above (const) one, and
-		//!!b) be in line with other similar ones (like enable/disable etc.)!...
-		//!!But change() here would be awkward -- even resorting to setChanged()
-		//!!feels better, but then it's out-of-style (for the new API) again...)
-
 	// Enable/disable processing (user) events
 	// (Not just inputs, but also outputs like triggering user callbacks.)
 	Widget* enable(bool state = true);
@@ -185,23 +161,13 @@ friend class Tooltip; // just to access getMain() via Tooltip::m_owner->!
 	virtual void onResized() {}
 	virtual void onThemeChanged() {}
 
-	//!!This actually belongs to InputWidget (`protected` exactly for it to be visible
-	//!!there), but not sure how a virtual would mix with that being a template!...
-	//!!Also, it's not even just a dummy callback like the others, but a dispatcher! :-/
-	//!!Needs cleanup...
-	//!! -> Actually, as a generic "callback invoker", even up in EventHandler, like
-	//!!    EventHandler::invoke_callback(which), could hit all the birds with one stone.
-	protected:
-	virtual void onUpdated(); //!! -> "EventHandler::invoke_callback(Updated)", and "updated" -> "notify(Updated)" or sg...
-
 private:
 	WidgetContainer* m_parent = nullptr;
 	Widget* m_previous = nullptr;
 	Widget* m_next = nullptr;
 
 	bool m_focusable;
 	WidgetState m_state;
-	bool m_changed;
 
 	sf::Vector2f m_position;
 	sf::Vector2f m_size;

include/sfw/Widgets/OptionsBox.inl

@@ -62,6 +62,7 @@ template <class T> auto OptionsBox<T>::set(size_t index)
 {
 	if (index != m_currentIndex)
 	{
+		this->setChanged(); //! this-> required here due to C++ cringe...
 		update_selection(index);
 	}
 	return this;

include/sfw/Widgets/Slider.hpp

@@ -68,7 +68,7 @@ class Slider: public InputWidget<Slider>
 	// Current value
 	Slider* set(float value);
 	float   get() const;
-	// Unfortunately, these are also needed to seamlessly support integers, too, without warnings:
+	// Unfortunately, these are also needed to seamlessly support integers, too (without warnings):
 	Slider* set(int value)      { return set((float)value); }
 	Slider* set(unsigned value) { return set((float)value); }
 

src/lib/Widget.cpp

@@ -45,8 +45,7 @@ Widget* Widget::getWidget(const std::string& name) const
 //----------------------------------------------------------------------------
 Widget::Widget() :
 	m_focusable(true),
-	m_state(WidgetState::Default),
-	m_changed(false)
+	m_state(WidgetState::Default)
 {
 }
 
@@ -57,7 +56,6 @@ Widget::Widget(Widget&& tmp) :
 	m_next(tmp.m_next),
 	m_focusable(tmp.m_focusable),
 	m_state(tmp.m_state),
-	m_changed(tmp.m_changed),
 	m_position(tmp.m_position),
 	m_size(tmp.m_size),
 	m_transform(tmp.m_transform)
@@ -78,7 +76,6 @@ Widget::Widget(const Widget& other) :
 	m_next(other.m_next),
 	m_focusable(other.m_focusable),
 	m_state(other.m_state),
-	m_changed(other.m_changed),
 	m_position(other.m_position),
 	m_size(other.m_size),
 	m_transform(other.m_transform)
@@ -341,29 +338,6 @@ const sf::Transform& Widget::getTransform() const
 }
 
 
-// Virtuals/Callbacks --------------------------------------------------------
-
-void Widget::onUpdated()
-{
-	using namespace Event::internal;
-
-	auto callback = m_callbackMap[Event::Update];
-	assert( std::holds_alternative<Callback_w>(callback) || std::holds_alternative<Callback_void>(callback) );
-		//!!?? This may no longer be true after getting something from the map that may not have
-		//!!?? been there! Seems to still work, so probably true, but make it explicit whether the
-		//!!?? map would indeed return a false std::function, properly wrapped in optional<>!...
-
-	if (std::holds_alternative<Callback_w>(callback) && std::get<Callback_w>(callback))
-	{
-		return (std::get<Callback_w>(callback).value()) (this);
-	}
-	else if (std::holds_alternative<Callback_void>(callback) && std::get<Callback_void>(callback))
-	{
-		return (std::get<Callback_void>(callback).value()) ();
-	}
-}
-
-
 // Diagnostics ---------------------------------------------------------------
 
 #ifdef DEBUG

src/lib/Widgets/Button.cpp

@@ -44,7 +44,7 @@ Button* Button::click()
 {
 	//! This weird kludge won't do antything to the visuals. (That would require
 	//! way more hocus pocus, for just a little gimmick, so... perhaps later.)
-	changed(true);
+	setChanged();
 	updated();
 	return this;
 }

src/lib/Widgets/CheckBox.cpp

@@ -28,7 +28,7 @@ CheckBox::CheckBox(std::function<void(CheckBox*)> callback, bool checked):
 CheckBox* CheckBox::set(bool checked)
 {
 	m_checked = checked;
-	changed(true);
+	setChanged();
 	return this;
 }
 

src/lib/Widgets/Slider.cpp

@@ -120,15 +120,24 @@ Slider* Slider::setRange(float min, float max, float step)
 Slider* Slider::set(float value)
 // Snaps to the closest multiple of step. (Remember: step may be non-integer!)
 {
-	//!! Disabling this until clarifying the "enigma" in #317! :) 
+	//!!----------------------------------------------------------------------------
+	//!! THIS IS NOW DONE, BUT LEAVING IT STILL HERE AS A MEMENTO...
+	//!!----------------------------------------------------------------------------
+	//!!
+	//!! Disabling this if(...) until clarifying the "enigma" in #317! :)
 	//!! ...And until finding a clean way to get this back, while also ensuring
 	//!! the an initial set() would count as a real change() (no matter what
 	//!! a C++ default-inited value would be! -> Universally, for each widget!
 	//!! The point here is actually the side-effects, though, so perhaps those
 	//!! should be taken care of during init (construction) independently from
 	//!! set()? But that'd be yet another explicit widget init chore then... :-/
 	//!!
-	//!!if (value != m_value) // Really changing?
+	//!!----------------------------------------------------------------------------
+	//!! (The -- trivial, in hindsight... also, kinda ingenious :) -- solution was
+	//!! just to init m_changed with true, instead of false in InputWidget!)
+	//!!----------------------------------------------------------------------------
+	if (value != m_value) // Really changing?
+	                      //! NOTE: No early return, as we may have things to do even if this is false!
 		//!!FIX: But if never actually changed (like ctor-init to 0...), then updateView, setTooltip etc. will never be called?!
 	{
 //cerr << "set("<< value <<")... [" << range().min << ".." << range().max << "], step: "<< step() <<endl;
@@ -149,17 +158,22 @@ Slider* Slider::set(float value)
 		if (value != m_value) // Still looks like a change? :) (Note: floating-point errors make this hit-and-miss though!)
 		{
 			m_value = value;
-//cerr << " - set(): m_value: "<< m_value <<endl;
-			updateView(); //! This is equivalent to `if (changed()) updateView()`,
-			              //! and also should be in onUpdated(), but for most widgets,
-			              //! the view can change even if the value doesn't (e.g. in
-				      //! onThemeChanged, or a widget may be resizable etc. etc.)
-				      //!! So, this should be an entire additional dimension to
-				      //!! widget change control (apart from value, i.e. the "model";
-				      //!! the "view" lives its own life, only _mostly_ depending on
-				      //!! the value!
+			setChanged();
 		}
 
+	}
+
+//cerr << " - set(): m_value: "<< m_value <<endl;
+	if (changed()) //! Note: this will be true initially even if the values are the same,
+	               //! because InputWidgets are constructed with changed = true!...
+	{
+		updateView();	//! This could be in onUpdated() -- but for most widgets,
+				//! the view can change even if the value doesn't (e.g. in
+				//! onThemeChanged, or a widget may be resizable etc. etc.)
+				//!! So, this should be an entire additional dimension to
+				//!! widget change control (apart from value, i.e. the "model";
+				//!! the "view" lives its own life, only _mostly_ depending on
+				//!! the value!
 		setTooltip(to_string(m_value));
 	}
 	return this;

src/lib/Widgets/TextBox.cpp

@@ -60,7 +60,7 @@ TextBox* TextBox::set(const std::string& content)
 	m_text.set(sfw::utf8_substr(content, 0, m_maxLength)); // Limit the length
 	setCursorPos(length()); // End(), but it's unclear if it'd be too high-level here...
 
-	changed(); //! Will become more sophisticated later (-> Undo/Redo)
+	setChanged(); //!! Will become more sophisticated with Undo/Redo etc.
 	return this;
 }
 
@@ -506,6 +506,8 @@ void TextBox::onKeyPressed(const sf::Event::KeyEvent& key)
 
 	// "Apply"
 	case sf::Keyboard::Enter:
+		setChanged(); //!! None of the inidividual editing actions update this yet,
+		              //!! so we just force it here for the time being...
 		updated(); //!!TBD: Should it be forced even if !changed()?
 		break;