From: Mikko Rasa <tdb@tdb.fi>
Date: Tue, 22 Aug 2023 20:07:49 +0000 (+0300)
Subject: Rework how widget ownership works in Container
X-Git-Url: http://git.tdb.fi/?a=commitdiff_plain;p=libs%2Fgltk.git

Rework how widget ownership works in Container

The class now supports both owned and non-owned children.  Owned children
are passed in as unique_ptr or can be created by the container with the
add_new() function template.  This is likely to introduce memory leaks in
existing code.
---

diff --git a/examples/widgetdemo/buttondemo.cpp b/examples/widgetdemo/buttondemo.cpp
index 170ce85..871244a 100644
--- a/examples/widgetdemo/buttondemo.cpp
+++ b/examples/widgetdemo/buttondemo.cpp
@@ -10,17 +10,15 @@ ButtonDemo::ButtonDemo()
 {
 	get_or_create_layout();
 
-	GLtk::Label *lbl_message = new GLtk::Label;
-	add(*lbl_message);
+	GLtk::Label *lbl_message = &add_new<GLtk::Label>();
 
 	vector<GLtk::Button *> buttons;
 	for(unsigned i=0; i<5; ++i)
 	{
 		string text = format("Button %d", i+1);
-		GLtk::Button *btn = new GLtk::Button(text);
+		GLtk::Button *btn = &add_new<GLtk::Button>(text);
 		btn->signal_clicked.connect(sigc::bind(sigc::mem_fun(lbl_message, &GLtk::Label::set_text), text+" was clicked"));
 		buttons.push_back(btn);
-		add(*btn);
 		if(i>0)
 		{
 			layout->add_constraint(*btn, GLtk::Layout::COPY_WIDTH, *buttons[0]);
@@ -41,10 +39,9 @@ ButtonDemo::ButtonDemo()
 
 	layout->add_constraint(*buttons[0], GLtk::Layout::BELOW, *lbl_message);
 
-	GLtk::Button *btn = new GLtk::Button("A big one");
+	GLtk::Button *btn = &add_new<GLtk::Button>("A big one");
 	btn->signal_clicked.connect(sigc::bind(sigc::mem_fun(lbl_message, &GLtk::Label::set_text), "The big button was clicked"));
 	buttons.push_back(btn);
-	add(*btn);
 	layout->add_constraint(*btn, GLtk::Layout::ALIGN_TOP, *buttons[0]);
 	layout->add_constraint(*btn, GLtk::Layout::ALIGN_BOTTOM, *buttons[1]);
 	layout->add_constraint(*btn, GLtk::Layout::ALIGN_LEFT, *buttons[3]);
diff --git a/examples/widgetdemo/demoselector.cpp b/examples/widgetdemo/demoselector.cpp
index 52ba458..507815c 100644
--- a/examples/widgetdemo/demoselector.cpp
+++ b/examples/widgetdemo/demoselector.cpp
@@ -14,8 +14,7 @@ DemoSelector::DemoSelector():
 
 	GLtk::Column col(*layout);
 
-	GLtk::Label *lbl = new GLtk::Label("Select a widget:");
-	add(*lbl);
+	add_new<GLtk::Label>("Select a widget:");
 
 	list.set_data(demos);
 	list.signal_item_selected.connect(sigc::mem_fun(this, &DemoSelector::item_selected));
diff --git a/examples/widgetdemo/dialogdemo.cpp b/examples/widgetdemo/dialogdemo.cpp
index 63fb68e..db8ba8d 100644
--- a/examples/widgetdemo/dialogdemo.cpp
+++ b/examples/widgetdemo/dialogdemo.cpp
@@ -27,8 +27,7 @@ DialogDemo::DialogDemo()
 {
 	get_or_create_layout();
 
-	GLtk::Button *btn_prompt = new GLtk::Button("Enter some text");
-	add(*btn_prompt);
+	GLtk::Button *btn_prompt = &add_new<GLtk::Button>("Enter some text");
 	btn_prompt->signal_clicked.connect(sigc::mem_fun(this, &DialogDemo::button_clicked));
 
 	add(lbl_text);
@@ -59,8 +58,7 @@ PromptDialog::PromptDialog()
 	add(ent_text);
 	layout->set_expand(ent_text, true, false);
 
-	GLtk::Button *btn_ok = new GLtk::Button("OK");
-	add_button(*btn_ok, 1);
+	GLtk::Button *btn_ok = &add_button("OK", 1);
 	layout->add_constraint(*btn_ok, GLtk::Layout::FAR_BELOW, ent_text);
 	layout->set_gravity(*btn_ok, 1, -1);
 }
diff --git a/examples/widgetdemo/dropdowndemo.cpp b/examples/widgetdemo/dropdowndemo.cpp
index 5ab8782..8d6d276 100644
--- a/examples/widgetdemo/dropdowndemo.cpp
+++ b/examples/widgetdemo/dropdowndemo.cpp
@@ -9,9 +9,8 @@ DropdownDemo::DropdownDemo()
 {
 	get_or_create_layout();
 
-	GLtk::Dropdown *drp = new GLtk::Dropdown(categories);
+	GLtk::Dropdown *drp = &add_new<GLtk::Dropdown>(categories);
 	drp->signal_item_selected.connect(sigc::mem_fun(this, &DropdownDemo::category_selected));
-	add(*drp);
 
 	categories.append("Arabic numerals");
 	categories.append("Roman numerals");
@@ -19,8 +18,7 @@ DropdownDemo::DropdownDemo()
 	categories.append("Uppercase letters");
 
 	GLtk::Widget *prev = drp;
-	drp = new GLtk::Dropdown(values);
-	add(*drp);
+	drp = &add_new<GLtk::Dropdown>(values);
 
 	layout->add_constraint(*drp, GLtk::Layout::BELOW, *prev);
 	layout->add_constraint(*drp, GLtk::Layout::ALIGN_LEFT, *prev);
diff --git a/examples/widgetdemo/entrydemo.cpp b/examples/widgetdemo/entrydemo.cpp
index 062ce8b..ad94010 100644
--- a/examples/widgetdemo/entrydemo.cpp
+++ b/examples/widgetdemo/entrydemo.cpp
@@ -8,27 +8,23 @@ EntryDemo::EntryDemo()
 {
 	get_or_create_layout();
 
-	GLtk::Label *lbl = new GLtk::Label("Single-line entries:");
-	add(*lbl);
+	GLtk::Label *lbl = &add_new<GLtk::Label>("Single-line entries:");
 
 	GLtk::Widget *prev = lbl;
 	for(unsigned i=0; i<2; ++i)
 	{
-		GLtk::Entry *ent = new GLtk::Entry;
+		GLtk::Entry *ent = &add_new<GLtk::Entry>();
 		ent->set_edit_size(20+i*40, 1);
-		add(*ent);
 		layout->add_constraint(*ent, GLtk::Layout::BELOW, *prev);
 		prev = ent;
 	}
 
-	lbl = new GLtk::Label("Multi-line entry:");
-	add(*lbl);
+	lbl = &add_new<GLtk::Label>("Multi-line entry:");
 	layout->add_constraint(*lbl, GLtk::Layout::BELOW, *prev);
 	prev = lbl;
 
-	GLtk::Entry *ent = new GLtk::Entry;
+	GLtk::Entry *ent = &add_new<GLtk::Entry>();
 	ent->set_multiline(true);
 	ent->set_edit_size(60, 5);
-	add(*ent);
 	layout->add_constraint(*ent, GLtk::Layout::BELOW, *prev);
 }
diff --git a/examples/widgetdemo/toggledemo.cpp b/examples/widgetdemo/toggledemo.cpp
index 91849e6..2cb6df5 100644
--- a/examples/widgetdemo/toggledemo.cpp
+++ b/examples/widgetdemo/toggledemo.cpp
@@ -9,14 +9,12 @@ ToggleDemo::ToggleDemo()
 {
 	get_or_create_layout();
 
-	GLtk::Label *lbl = new GLtk::Label("Standalone toggles:");
-	add(*lbl);
+	GLtk::Label *lbl = &add_new<GLtk::Label>("Standalone toggles:");
 
 	GLtk::Widget *prev = lbl;
 	for(unsigned i=0; i<4; ++i)
 	{
-		GLtk::Toggle *tgl = new GLtk::Toggle(format("Toggle %d", i+1));
-		add(*tgl);
+		GLtk::Toggle *tgl = &add_new<GLtk::Toggle>(format("Toggle %d", i+1));
 
 		layout->add_constraint(*tgl, GLtk::Layout::BELOW, *prev);
 		layout->add_constraint(*tgl, GLtk::Layout::ALIGN_LEFT, *prev);
@@ -27,23 +25,20 @@ ToggleDemo::ToggleDemo()
 
 	for(unsigned i=0; i<2; ++i)
 	{
-		GLtk::Panel *group = new GLtk::Panel;
-		add(*group);
+		GLtk::Panel *group = &add_new<GLtk::Panel>();
 		layout->add_constraint(*group, (i==0 ? GLtk::Layout::BELOW : GLtk::Layout::RIGHT_OF), *prev);
 		if(i>0)
 			layout->add_constraint(*group, GLtk::Layout::ALIGN_TOP, *prev);
 		GLtk::Layout &group_layout = group->get_or_create_layout();
 
-		lbl = new GLtk::Label(format("Group %d:", i+1));
-		group->add(*lbl);
+		lbl = &group->add_new<GLtk::Label>(format("Group %d:", i+1));
 
 		prev = lbl;
 		for(unsigned j=0; j<4; ++j)
 		{
-			GLtk::Toggle *tgl = new GLtk::Toggle(format("Option %d", j+1));
+			GLtk::Toggle *tgl = &group->add_new<GLtk::Toggle>(format("Option %d", j+1));
 			tgl->set_style("option");
 			tgl->set_exclusive(true);
-			group->add(*tgl);
 
 			group_layout.add_constraint(*tgl, GLtk::Layout::BELOW, *prev);
 			group_layout.add_constraint(*tgl, GLtk::Layout::ALIGN_LEFT, *prev);
diff --git a/examples/widgetdemo/widgetdemo.cpp b/examples/widgetdemo/widgetdemo.cpp
index b1482e7..1332ee1 100644
--- a/examples/widgetdemo/widgetdemo.cpp
+++ b/examples/widgetdemo/widgetdemo.cpp
@@ -25,11 +25,11 @@ WidgetDemo::WidgetDemo(int, char **):
 	root.add(selector);
 	root_layout.set_expand(selector, false, true);
 
-	add_demo("Button", new ButtonDemo);
-	add_demo("Dialog", new DialogDemo);
-	add_demo("Dropdown", new DropdownDemo);
-	add_demo("Entry", new EntryDemo);
-	add_demo("Toggle", new ToggleDemo);
+	add_demo<ButtonDemo>("Button");
+	add_demo<DialogDemo>("Dialog");
+	add_demo<DropdownDemo>("Dropdown");
+	add_demo<EntryDemo>("Entry");
+	add_demo<ToggleDemo>("Toggle");
 
 	view.set_content(&root);
 }
@@ -46,14 +46,15 @@ void WidgetDemo::tick()
 	view.render();
 }
 
-void WidgetDemo::add_demo(const string &title, GLtk::Panel *demo)
+template<typename T>
+void WidgetDemo::add_demo(const string &title)
 {
 	GLtk::Layout &root_layout = root.get_or_create_layout();
 
-	demo->set_visible(false);
-	root.add(*demo);
-	root_layout.add_constraint(*demo, GLtk::Layout::RIGHT_OF, selector);
-	root_layout.set_expand(*demo, true, true);
+	T &demo = root.add_new<T>();
+	demo.set_visible(false);
+	root_layout.add_constraint(demo, GLtk::Layout::RIGHT_OF, selector);
+	root_layout.set_expand(demo, true, true);
 
-	selector.add_demo(title, demo);
+	selector.add_demo(title, &demo);
 }
diff --git a/examples/widgetdemo/widgetdemo.h b/examples/widgetdemo/widgetdemo.h
index cab80b7..f6ef7f6 100644
--- a/examples/widgetdemo/widgetdemo.h
+++ b/examples/widgetdemo/widgetdemo.h
@@ -26,7 +26,8 @@ public:
 private:
 	virtual void tick();
 
-	void add_demo(const std::string &, Msp::GLtk::Panel *);
+	template<typename T>
+	void add_demo(const std::string &);
 };
 
 #endif
diff --git a/source/container.cpp b/source/container.cpp
index 88e5952..f1f1934 100644
--- a/source/container.cpp
+++ b/source/container.cpp
@@ -14,19 +14,21 @@ hierarchy_error::hierarchy_error(const string &w):
 
 Container::~Container()
 {
+	// Clear children here while members are still valid
 	while(!children.empty())
-		delete children.front()->widget;
+	{
+		if(children.back()->own_widget)
+			/* Avoid destroying the unique_ptr for the widget from within its own
+			reset() function */
+			unique_ptr<Widget> w = move(children.back()->own_widget);
+		else
+			children.pop_back();
+	}
 }
 
-void Container::add(Widget &wdg)
+void Container::add(unique_ptr<Widget> wdg)
 {
-	wdg.set_parent(this);
-	children.push_back(make_unique<Child>(*this, &wdg));
-	if(wdg.get_animation_interval())
-		check_animation_interval();
-	children_rebuild_needed = true;
-	signal_rebuild_needed.emit();
-	on_child_added(wdg);
+	add_child(*wdg).own_widget = move(wdg);
 }
 
 void Container::remove(Widget &wdg)
@@ -35,6 +37,7 @@ void Container::remove(Widget &wdg)
 	if(i==children.end())
 		throw hierarchy_error("widget not in container");
 
+	unique_ptr<Widget> owned = move((*i)->own_widget);
 	if(&wdg==saved_input_focus)
 		saved_input_focus = nullptr;
 	wdg.set_parent(nullptr);
@@ -44,6 +47,18 @@ void Container::remove(Widget &wdg)
 	on_child_removed(wdg);
 }
 
+Container::Child &Container::add_child(Widget &wdg)
+{
+	wdg.set_parent(this);
+	children.push_back(make_unique<Child>(*this, &wdg));
+	if(wdg.get_animation_interval())
+		check_animation_interval();
+	children_rebuild_needed = true;
+	signal_rebuild_needed.emit();
+	on_child_added(wdg);
+	return *children.back();
+}
+
 Geometry Container::determine_child_geometry(const Widget &child, const Part &part) const
 {
 	Geometry pgeom = part.get_geometry();
@@ -381,6 +396,12 @@ Container::Child::Child(Container &c, Widget *w):
 	widget->signal_rebuild_needed.connect(sigc::mem_fun(this, &Child::rebuild_needed));
 }
 
+Container::Child::Child(Container &c, unique_ptr<Widget> w):
+	Child(c, w.get())
+{
+	own_widget = move(w);
+}
+
 Container::Child::~Child()
 {
 	visibility_changed(false);
diff --git a/source/container.h b/source/container.h
index 6311a41..d100dac 100644
--- a/source/container.h
+++ b/source/container.h
@@ -24,10 +24,12 @@ protected:
 	struct Child: public sigc::trackable
 	{
 		Container &container;
+		std::unique_ptr<Widget> own_widget;
 		Widget *widget = nullptr;
 		Time::TimeDelta time_since_animate;
 
 		Child(Container &, Widget *);
+		Child(Container &, std::unique_ptr<Widget>);
 		virtual ~Child();
 
 		void visibility_changed(bool);
@@ -52,9 +54,15 @@ protected:
 public:
 	virtual ~Container();
 
-	void add(Widget &);
+	void add(Widget &w) { add_child(w); }
+	void add(std::unique_ptr<Widget>);
+
+	template<typename T, typename... Args>
+	T &add_new(Args &&...);
+
 	void remove(Widget &);
 protected:
+	Child &add_child(Widget &);
 	Geometry determine_child_geometry(const Widget &, const Part &) const;
 	void autosize_child(const Widget &, const Part &, Geometry &) const;
 	void reposition_child(Widget &, const Part &) const;
@@ -102,6 +110,16 @@ protected:
 	virtual void on_input_focus_changed(Widget *);
 };
 
+
+template<typename T, typename... Args>
+T &Container::add_new(Args &&... args)
+{
+	std::unique_ptr<T> wdg = std::make_unique<T>(std::forward<Args>(args)...);
+	T *ptr = wdg.get();
+	add(move(wdg));
+	return *ptr;
+}
+
 } // namespace GLtk
 } // namespace Msp
 
diff --git a/source/dialog.cpp b/source/dialog.cpp
index 5e5e390..fed9c85 100644
--- a/source/dialog.cpp
+++ b/source/dialog.cpp
@@ -9,7 +9,21 @@ namespace GLtk {
 void Dialog::add_button(Button &button, int code)
 {
 	add(button);
-	button.signal_clicked.connect(sigc::bind(sigc::mem_fun(this, &Dialog::response), code));
+	connect_button(button, code);
+}
+
+void Dialog::add_button(unique_ptr<Button> button, int code)
+{
+	Button &b = *button;
+	add(move(button));
+	connect_button(b, code);
+}
+
+Button &Dialog::add_button(const string &text, int code)
+{
+	Button &b = add_new<Button>(text);
+	connect_button(b, code);
+	return b;
 }
 
 void Dialog::set_modal(bool m)
@@ -24,6 +38,11 @@ void Dialog::set_modal(bool m)
 		signal_ungrab_pointer.emit();
 }
 
+void Dialog::connect_button(Button &button, int code)
+{
+	button.signal_clicked.connect(sigc::bind(sigc::mem_fun(this, &Dialog::response), code));
+}
+
 void Dialog::response(int code)
 {
 	on_response(code);
@@ -42,8 +61,9 @@ void Dialog::Loader::action_button(const string &n, int c)
 {
 	unique_ptr<Button> btn = make_unique<Button>();
 	load_sub(*btn);
-	obj.add_button(*btn.get(), c);
-	last_widget = wdg_map[n] = btn.release();
+	Widget *wdg = btn.get();
+	obj.add_button(move(btn), c);
+	last_widget = wdg_map[n] = wdg;
 }
 
 } // namespace GLtk
diff --git a/source/dialog.h b/source/dialog.h
index cc5ed41..6209ab2 100644
--- a/source/dialog.h
+++ b/source/dialog.h
@@ -33,11 +33,16 @@ public:
 	response handlers and delete the dialog. */
 	void add_button(Button &, int);
 
+	void add_button(std::unique_ptr<Button>, int);
+
+	Button &add_button(const std::string &, int);
+
 	/** Sets the modality of the dialog.  When modal, the user can't navigate
 	away from the dialog. */
 	void set_modal(bool);
 
 protected:
+	void connect_button(Button &, int);
 	void response(int);
 
 	/** Called when an action button is pressed. */
diff --git a/source/entry.cpp b/source/entry.cpp
index b212f5f..b2c7800 100644
--- a/source/entry.cpp
+++ b/source/entry.cpp
@@ -118,8 +118,9 @@ void Entry::set_multiline(bool m)
 	{
 		if(!slider)
 		{
-			slider = new VSlider;
-			add(*slider);
+			unique_ptr<VSlider> s = make_unique<VSlider>();
+			slider = s.get();
+			add(move(s));
 			slider->set_step(1);
 			slider->signal_value_changed.connect(sigc::mem_fun(this, &Entry::slider_value_changed));
 			mark_rebuild();
diff --git a/source/panel.cpp b/source/panel.cpp
index d4e737b..ba66c68 100644
--- a/source/panel.cpp
+++ b/source/panel.cpp
@@ -295,8 +295,9 @@ void Panel::Loader::unnamed_child<Panel>()
 {
 	unique_ptr<Panel> pnl = make_unique<Panel>();
 	load_sub(*pnl, wdg_map);
-	obj.add(*pnl.get());
-	last_widget = pnl.release();
+	Widget *wdg = pnl.get();
+	obj.add(move(pnl));
+	last_widget = wdg;
 }
 
 
diff --git a/source/panel.h b/source/panel.h
index 3abfee1..3061032 100644
--- a/source/panel.h
+++ b/source/panel.h
@@ -117,8 +117,9 @@ void Panel::Loader::unnamed_child()
 {
 	std::unique_ptr<T> chl = std::make_unique<T>();
 	load_sub(*chl);
-	obj.add(*chl.get());
-	last_widget = chl.release();
+	Widget *wdg = chl.get();
+	obj.add(move(chl));
+	last_widget = wdg;
 }
 
 
diff --git a/source/root.cpp b/source/root.cpp
index 8d32c46..16ed5bd 100644
--- a/source/root.cpp
+++ b/source/root.cpp
@@ -90,8 +90,7 @@ void Root::tick()
 		{
 			if(!lbl_tooltip)
 			{
-				lbl_tooltip = new Label;
-				add(*lbl_tooltip);
+				lbl_tooltip = &add_new<Label>();
 				lbl_tooltip->set_style("tooltip");
 			}