Fix views leaking when closing themselves

Views would invoke c->Exit() upon closing themselves, which would close them but not delete them. The controller would then be destroyed, fail to close the view because it wouldn't be the window on top, and skip destroying it in fear of something worse. This behaviour was introduced by c2f8a7df25, fixing something indeed worse: views would be destroyed while they were still in use by ui::Engine, sitting somewhere on the window stack but not at the top. In other words, the problem was (and is) that the terms of ownership of views between controllers and ui::Engine are unclear.

This commit effectively undoes that earlier commit and tries a different approach: when something is closed, close everything above it. This seems a correct thing to do and also allows controllers to unconditionally take care of their views. Prepare your popcorn though.

This fixes a bug where the stamp browser would stop rendering stamps if it was previously closed "too quickly", which turned out to mean that it was closed before all stamps had finished rendering, and thus ThumbnailRendererTasks belonging to LocalBrowserViews indirectly through SaveButtons would get stuck in a queued state, preventing other SaveButtons from starting their own ThumbnailRendererTasks.

src/gui/console/ConsoleController.cpp

@@ -72,9 +72,7 @@ ConsoleView * ConsoleController::GetView()
 ConsoleController::~ConsoleController()
 {
 	delete consoleModel;
-	if (consoleView->CloseActiveWindow())
-	{
-		delete consoleView;
-	}
+	consoleView->CloseActiveWindow();
+	delete consoleView;
 }
 

src/gui/game/GameController.cpp

@@ -150,10 +150,8 @@ GameController::~GameController()
 	}
 	commandInterface.reset();
 	delete gameModel;
-	if (gameView->CloseActiveWindow())
-	{
-		delete gameView;
-	}
+	gameView->CloseActiveWindow();
+	delete gameView;
 }
 
 bool GameController::HistoryRestore()

src/gui/interface/Engine.cpp

@@ -32,8 +32,8 @@ Engine::~Engine()
 	//Dispose of any Windows.
 	while (!windows.empty())
 	{
-		delete windows.top();
-		windows.pop();
+		delete windows.back();
+		windows.pop_back();
 	}
 }
 
@@ -79,6 +79,7 @@ void Engine::ConfirmExit()
 
 void Engine::ShowWindow(Window * window)
 {
+	CloseWindowAndEverythingAbove(window);
 	if (state_)
 		ignoreEvents = true;
 	if(window->Position.X==-1)
@@ -101,7 +102,7 @@ void Engine::ShowWindow(Window * window)
 		frozenGraphics.emplace(FrozenGraphics{0, std::make_unique<pixel []>(g->Size().X * g->Size().Y)});
 		std::copy_n(g->Data(), g->Size().X * g->Size().Y, frozenGraphics.top().screen.get());
 
-		windows.push(state_);
+		windows.push_back(state_);
 		mousePositions.push(ui::Point(mousex_, mousey_));
 	}
 	if(state_)
@@ -111,13 +112,31 @@ void Engine::ShowWindow(Window * window)
 
 }
 
+void Engine::CloseWindowAndEverythingAbove(Window *window)
+{
+	if (window == state_)
+	{
+		CloseWindow();
+		return;
+	}
+	auto it = std::find(windows.begin(), windows.end(), window);
+	if (it != windows.end())
+	{
+		auto toPop = int(windows.end() - it) + 1; // including state_
+		for (int i = 0; i < toPop; ++i)
+		{
+			CloseWindow();
+		}
+	}
+}
+
 int Engine::CloseWindow()
 {
 	if(!windows.empty())
 	{
 		frozenGraphics.pop();
-		state_ = windows.top();
-		windows.pop();
+		state_ = windows.back();
+		windows.pop_back();
 
 		if(state_)
 			state_->DoFocus();

src/gui/interface/Engine.h

@@ -27,6 +27,7 @@ namespace ui
 
 		void ShowWindow(Window * window);
 		int CloseWindow();
+		void CloseWindowAndEverythingAbove(Window *window);
 
 		void initialMouse(int x, int y);
 		void onMouseMove(int x, int y);
@@ -90,7 +91,7 @@ namespace ui
 
 		float dt;
 		float fps;
-		std::stack<Window*> windows;
+		std::deque<Window*> windows;
 		std::stack<Point> mousePositions;
 		//Window* statequeued_;
 		Window* state_;

src/gui/interface/Window.cpp

@@ -141,14 +141,9 @@ void Window::MakeActiveWindow()
 		Engine::Ref().ShowWindow(this);
 }
 
-bool Window::CloseActiveWindow()
+void Window::CloseActiveWindow()
 {
-	if (Engine::Ref().GetWindow() == this)
-	{
-		Engine::Ref().CloseWindow();
-		return true;
-	}
-	return false;
+	Engine::Ref().CloseWindowAndEverythingAbove(this);
 }
 
 Graphics * Window::GetGraphics()

src/gui/interface/Window.h

@@ -82,7 +82,7 @@ namespace ui
 		enum ExitMethod { MouseOutside, Escape, ExitButton };
 
 		void MakeActiveWindow();
-		bool CloseActiveWindow();
+		void CloseActiveWindow();
 		Graphics * GetGraphics();
 
 	protected:

src/gui/localbrowser/LocalBrowserController.cpp

@@ -161,9 +161,7 @@ void LocalBrowserController::Exit()
 LocalBrowserController::~LocalBrowserController()
 {
 	delete browserModel;
-	if (browserView->CloseActiveWindow())
-	{
-		delete browserView;
-	}
+	browserView->CloseActiveWindow();
+	delete browserView;
 }
 

src/gui/login/LoginController.cpp

@@ -44,9 +44,7 @@ void LoginController::Exit()
 LoginController::~LoginController()
 {
 	delete loginModel;
-	if (loginView->CloseActiveWindow())
-	{
-		delete loginView;
-	}
+	loginView->CloseActiveWindow();
+	delete loginView;
 }
 

src/gui/options/OptionsController.cpp

@@ -170,9 +170,7 @@ void OptionsController::Exit()
 OptionsController::~OptionsController()
 {
 	delete model;
-	if (view->CloseActiveWindow())
-	{
-		delete view;
-	}
+	view->CloseActiveWindow();
+	delete view;
 }
 

src/gui/preview/PreviewController.cpp

@@ -150,8 +150,6 @@ PreviewController::~PreviewController()
 {
 	Client::Ref().RemoveListener(this);
 	delete previewModel;
-	if (previewView->CloseActiveWindow())
-	{
-		delete previewView;
-	}
+	previewView->CloseActiveWindow();
+	delete previewView;
 }

src/gui/render/RenderController.cpp

@@ -67,9 +67,7 @@ void RenderController::Exit()
 RenderController::~RenderController()
 {
 	delete renderModel;
-	if (renderView->CloseActiveWindow())
-	{
-		delete renderView;
-	}
+	renderView->CloseActiveWindow();
+	delete renderView;
 }
 

src/gui/search/SearchController.cpp

@@ -93,10 +93,8 @@ SearchController::~SearchController()
 {
 	delete activePreview;
 	delete searchModel;
-	if (searchView->CloseActiveWindow())
-	{
-		delete searchView;
-	}
+	searchView->CloseActiveWindow();
+	delete searchView;
 }
 
 void SearchController::DoSearch(String query, bool now)

src/gui/tags/TagsController.cpp

@@ -52,9 +52,7 @@ void TagsController::Exit()
 TagsController::~TagsController()
 {
 	delete tagsModel;
-	if (tagsView->CloseActiveWindow())
-	{
-		delete tagsView;
-	}
+	tagsView->CloseActiveWindow();
+	delete tagsView;
 }
 

src/tasks/TaskWindow.cpp

@@ -62,10 +62,8 @@ void TaskWindow::NotifyDone(Task * task)
 
 void TaskWindow::Exit()
 {
-	if (CloseActiveWindow())
-	{
-		SelfDestruct();
-	}
+	CloseActiveWindow();
+	SelfDestruct();
 }
 
 void TaskWindow::NotifyProgress(Task * task)