From 02b679aec37f59e3059e67a2fab3e6f121ed6318 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tam=C3=A1s=20B=C3=A1lint=20Misius?= Date: Fri, 25 Oct 2024 22:02:53 +0200 Subject: [PATCH] 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 c2f8a7df25f3, 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 | 6 ++-- src/gui/game/GameController.cpp | 6 ++-- src/gui/interface/Engine.cpp | 29 +++++++++++++++---- src/gui/interface/Engine.h | 3 +- src/gui/interface/Window.cpp | 9 ++---- src/gui/interface/Window.h | 2 +- .../localbrowser/LocalBrowserController.cpp | 6 ++-- src/gui/login/LoginController.cpp | 6 ++-- src/gui/options/OptionsController.cpp | 6 ++-- src/gui/preview/PreviewController.cpp | 6 ++-- src/gui/render/RenderController.cpp | 6 ++-- src/gui/search/SearchController.cpp | 6 ++-- src/gui/tags/TagsController.cpp | 6 ++-- src/tasks/TaskWindow.cpp | 6 ++-- 14 files changed, 49 insertions(+), 54 deletions(-) diff --git a/src/gui/console/ConsoleController.cpp b/src/gui/console/ConsoleController.cpp index 47a4d0327..1f22e848f 100644 --- a/src/gui/console/ConsoleController.cpp +++ b/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; } diff --git a/src/gui/game/GameController.cpp b/src/gui/game/GameController.cpp index a18b4ba4f..f9639e407 100644 --- a/src/gui/game/GameController.cpp +++ b/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() diff --git a/src/gui/interface/Engine.cpp b/src/gui/interface/Engine.cpp index b0a3b0aa3..6d8e72caa 100644 --- a/src/gui/interface/Engine.cpp +++ b/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(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(); diff --git a/src/gui/interface/Engine.h b/src/gui/interface/Engine.h index 88367f6a7..7738c9e47 100644 --- a/src/gui/interface/Engine.h +++ b/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 windows; + std::deque windows; std::stack mousePositions; //Window* statequeued_; Window* state_; diff --git a/src/gui/interface/Window.cpp b/src/gui/interface/Window.cpp index 8de1a0e24..b70435f4b 100644 --- a/src/gui/interface/Window.cpp +++ b/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() diff --git a/src/gui/interface/Window.h b/src/gui/interface/Window.h index 66d84427f..11d228a31 100644 --- a/src/gui/interface/Window.h +++ b/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: diff --git a/src/gui/localbrowser/LocalBrowserController.cpp b/src/gui/localbrowser/LocalBrowserController.cpp index a2684a656..2b2f383a2 100644 --- a/src/gui/localbrowser/LocalBrowserController.cpp +++ b/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; } diff --git a/src/gui/login/LoginController.cpp b/src/gui/login/LoginController.cpp index 2d53c7cd5..cabc0569e 100644 --- a/src/gui/login/LoginController.cpp +++ b/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; } diff --git a/src/gui/options/OptionsController.cpp b/src/gui/options/OptionsController.cpp index 56dfb8200..8c4881338 100644 --- a/src/gui/options/OptionsController.cpp +++ b/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; } diff --git a/src/gui/preview/PreviewController.cpp b/src/gui/preview/PreviewController.cpp index 4de00c936..1636acedd 100644 --- a/src/gui/preview/PreviewController.cpp +++ b/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; } diff --git a/src/gui/render/RenderController.cpp b/src/gui/render/RenderController.cpp index 0ff5168f8..218fe90cc 100644 --- a/src/gui/render/RenderController.cpp +++ b/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; } diff --git a/src/gui/search/SearchController.cpp b/src/gui/search/SearchController.cpp index 7432cde76..ecb3ed10a 100644 --- a/src/gui/search/SearchController.cpp +++ b/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) diff --git a/src/gui/tags/TagsController.cpp b/src/gui/tags/TagsController.cpp index df9ec12b3..ed2e0cbbe 100644 --- a/src/gui/tags/TagsController.cpp +++ b/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; } diff --git a/src/tasks/TaskWindow.cpp b/src/tasks/TaskWindow.cpp index 4c9e9c97f..bf7e49c35 100644 --- a/src/tasks/TaskWindow.cpp +++ b/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)