SDL: Only recreate SDL2 window when necessary

Destroying and recreating the SDL window whenever the video mode
changes in SDL2 is not necessary and causes several problems:

1. In windowed mode, the game window shifts position;
2. In fullscreen mode in macOS, every time the window is
   recreated, it causes the OS to play its switch-to-fullscreen
   animation again and emit system alert noises;
3. The window content flickers; and
4. The engine loses events from the old destroyed window.

This patch changes the SDL backend code to avoid destroying and
recreating the SDL window when using SDL2, except when switching
OpenGL modes, since there is no way to change the OpenGL feature
of a window.

There are still some outstanding issues with OpenGL where window
size ends up getting reset even though the user has resized it;
this will probably need to be addressed at some point in another
patch.

Thanks to @bgK and @criezy for their feedback which made this
patch much better.

Co-Authored-By: Bastien Bouclet <bastien.bouclet@gmail.com>

5 files changed, 89 insertions, 30 deletions

diff --git a/backends/graphics/openglsdl/openglsdl-graphics.cpp b/backends/graphics/openglsdl/openglsdl-graphics.cpp
index e41148062f..f664314862 100644
--- a/backends/graphics/openglsdl/openglsdl-graphics.cpp
+++ b/backends/graphics/openglsdl/openglsdl-graphics.cpp
@@ -463,10 +463,6 @@ bool OpenGLSdlGraphicsManager::setupMode(uint width, uint height) {
 		// Remember our choice.
 		ConfMan.setInt("last_fullscreen_mode_width", _desiredFullscreenWidth, Common::ConfigManager::kApplicationDomain);
 		ConfMan.setInt("last_fullscreen_mode_height", _desiredFullscreenHeight, Common::ConfigManager::kApplicationDomain);
-
-		// Use our choice.
-		width  = _desiredFullscreenWidth;
-		height = _desiredFullscreenHeight;
 	}
 
 	// This is pretty confusing since RGBA8888 talks about the memory
@@ -489,13 +485,23 @@ bool OpenGLSdlGraphicsManager::setupMode(uint width, uint height) {
 		_glContext = nullptr;
 	}
 
-	_window->destroyWindow();
-
-	uint32 flags = SDL_WINDOW_OPENGL;
+	uint32 flags = SDL_WINDOW_OPENGL | SDL_WINDOW_RESIZABLE;
 	if (_wantsFullScreen) {
+		// On Linux/X11, when toggling to fullscreen, the window manager saves
+		// the window size to be able to restore it when going back to windowed mode.
+		// If the user configured ScummVM to start in fullscreen mode, we first
+		// create a window and then toggle it to fullscreen to give the window manager
+		// a chance to save the window size. That way if the user switches back
+		// to windowed mode, the window manager has a window size to apply instead
+		// of leaving the window at the fullscreen resolution size.
+		if (!_window->getSDLWindow()) {
+			_window->createOrUpdateWindow(width, height, flags);
+		}
+
+		width  = _desiredFullscreenWidth;
+		height = _desiredFullscreenHeight;
+
 		flags |= SDL_WINDOW_FULLSCREEN;
-	} else {
-		flags |= SDL_WINDOW_RESIZABLE;
 	}
 
 	// Request a OpenGL (ES) context we can use.
@@ -503,11 +509,11 @@ bool OpenGLSdlGraphicsManager::setupMode(uint width, uint height) {
 	SDL_GL_SetAttribute(SDL_GL_CONTEXT_MAJOR_VERSION, _glContextMajor);
 	SDL_GL_SetAttribute(SDL_GL_CONTEXT_MINOR_VERSION, _glContextMinor);
 
-	if (!_window->createWindow(width, height, flags)) {
+	if (!_window->createOrUpdateWindow(width, height, flags)) {
 		// We treat fullscreen requests as a "hint" for now. This means in
 		// case it is not available we simply ignore it.
 		if (_wantsFullScreen) {
-			_window->createWindow(width, height, SDL_WINDOW_OPENGL | SDL_WINDOW_RESIZABLE);
+			_window->createOrUpdateWindow(width, height, SDL_WINDOW_OPENGL | SDL_WINDOW_RESIZABLE);
 		}
 
 		if (!_window->getSDLWindow()) {
@@ -541,6 +547,8 @@ bool OpenGLSdlGraphicsManager::setupMode(uint width, uint height) {
 
 	uint32 flags = SDL_OPENGL;
 	if (_wantsFullScreen) {
+		width  = _desiredFullscreenWidth;
+		height = _desiredFullscreenHeight;
 		flags |= SDL_FULLSCREEN;
 	} else {
 		flags |= SDL_RESIZABLE;
diff --git a/backends/graphics/surfacesdl/surfacesdl-graphics.cpp b/backends/graphics/surfacesdl/surfacesdl-graphics.cpp
index 9594587d88..2e658bca3b 100644
--- a/backends/graphics/surfacesdl/surfacesdl-graphics.cpp
+++ b/backends/graphics/surfacesdl/surfacesdl-graphics.cpp
@@ -214,6 +214,10 @@ SurfaceSdlGraphicsManager::SurfaceSdlGraphicsManager(SdlEventSource *sdlEventSou
 
 SurfaceSdlGraphicsManager::~SurfaceSdlGraphicsManager() {
 	unloadGFXMode();
+#if SDL_VERSION_ATLEAST(2, 0, 0)
+	if (_window)
+		_window->destroyWindow();
+#endif
 	if (_mouseSurface)
 		SDL_FreeSurface(_mouseSurface);
 	_mouseSurface = 0;
@@ -2644,13 +2648,11 @@ void SurfaceSdlGraphicsManager::notifyVideoExpose() {
 	_forceFull = true;
 }
 
-#ifdef USE_SDL_RESIZABLE_WINDOW
 void SurfaceSdlGraphicsManager::notifyResize(const uint width, const uint height) {
 #if SDL_VERSION_ATLEAST(2, 0, 0)
 	setWindowResolution(width, height);
 #endif
 }
-#endif
 
 void SurfaceSdlGraphicsManager::transformMouseCoordinates(Common::Point &point) {
 #if SDL_VERSION_ATLEAST(2, 0, 0)
@@ -2683,9 +2685,6 @@ void SurfaceSdlGraphicsManager::deinitializeRenderer() {
 
 	SDL_DestroyRenderer(_renderer);
 	_renderer = nullptr;
-
-	if (_window)
-		_window->destroyWindow();
 }
 
 void SurfaceSdlGraphicsManager::setWindowResolution(int width, int height) {
@@ -2746,7 +2745,7 @@ SDL_Surface *SurfaceSdlGraphicsManager::SDL_SetVideoMode(int width, int height,
 		createWindowFlags |= SDL_WINDOW_FULLSCREEN_DESKTOP;
 	}
 
-	if (!_window->createWindow(width, height, createWindowFlags)) {
+	if (!_window->createOrUpdateWindow(width, height, createWindowFlags)) {
 		return nullptr;
 	}
 
diff --git a/backends/graphics/surfacesdl/surfacesdl-graphics.h b/backends/graphics/surfacesdl/surfacesdl-graphics.h
index fa130cd9a5..532399ed66 100644
--- a/backends/graphics/surfacesdl/surfacesdl-graphics.h
+++ b/backends/graphics/surfacesdl/surfacesdl-graphics.h
@@ -156,9 +156,7 @@ public:
 
 	// SdlGraphicsManager interface
 	virtual void notifyVideoExpose();
-#ifdef USE_SDL_RESIZABLE_WINDOW
 	virtual void notifyResize(const uint width, const uint height);
-#endif
 	virtual void transformMouseCoordinates(Common::Point &point);
 	virtual void notifyMousePos(Common::Point mouse);
 
diff --git a/backends/platform/sdl/sdl-window.cpp b/backends/platform/sdl/sdl-window.cpp
index 609186a061..543320f3be 100644
--- a/backends/platform/sdl/sdl-window.cpp
+++ b/backends/platform/sdl/sdl-window.cpp
@@ -28,9 +28,12 @@
 
 #include "icons/scummvm.xpm"
 
+static const uint32 fullscreenMask = SDL_WINDOW_FULLSCREEN_DESKTOP | SDL_WINDOW_FULLSCREEN;
+
 SdlWindow::SdlWindow()
 #if SDL_VERSION_ATLEAST(2, 0, 0)
-	: _window(nullptr), _inputGrabState(false), _windowCaption("ScummVM")
+	: _window(nullptr), _inputGrabState(false), _windowCaption("ScummVM"),
+	_lastFlags(0), _lastX(SDL_WINDOWPOS_UNDEFINED), _lastY(SDL_WINDOWPOS_UNDEFINED)
 #endif
 	{
 }
@@ -199,25 +202,67 @@ SDL_Surface *copySDLSurface(SDL_Surface *src) {
 	return res;
 }
 
-bool SdlWindow::createWindow(int width, int height, uint32 flags) {
-	destroyWindow();
-
+bool SdlWindow::createOrUpdateWindow(int width, int height, uint32 flags) {
 	if (_inputGrabState) {
 		flags |= SDL_WINDOW_INPUT_GRABBED;
 	}
 
-	_window = SDL_CreateWindow(_windowCaption.c_str(), SDL_WINDOWPOS_UNDEFINED,
-	                           SDL_WINDOWPOS_UNDEFINED, width, height, flags);
+	// SDL_WINDOW_RESIZABLE can also be updated without recreating the window
+	// starting with SDL 2.0.5, but it is not treated as updateable here
+	// because:
+	// 1. It is currently only changed in conjunction with the SDL_WINDOW_OPENGL
+	//    flag, so the window will always be recreated anyway when changing
+	//    resizability; and
+	// 2. Users (particularly on Windows) will sometimes swap older SDL DLLs
+	//    to avoid bugs, which would be impossible if the feature was enabled
+	//    at compile time using SDL_VERSION_ATLEAST.
+	const uint32 updateableFlagsMask = fullscreenMask | SDL_WINDOW_INPUT_GRABBED;
+
+	const uint32 oldNonUpdateableFlags = _lastFlags & ~updateableFlagsMask;
+	const uint32 newNonUpdateableFlags = flags & ~updateableFlagsMask;
+
+	if (!_window || oldNonUpdateableFlags != newNonUpdateableFlags) {
+		destroyWindow();
+		_window = SDL_CreateWindow(_windowCaption.c_str(), _lastX,
+								   _lastY, width, height, flags);
+		if (_window) {
+			setupIcon();
+		}
+	} else {
+		const uint32 fullscreenFlags = flags & fullscreenMask;
+
+		if (fullscreenFlags) {
+			SDL_DisplayMode fullscreenMode;
+			fullscreenMode.w = width;
+			fullscreenMode.h = height;
+			fullscreenMode.driverdata = nullptr;
+			fullscreenMode.format = 0;
+			fullscreenMode.refresh_rate = 0;
+			SDL_SetWindowDisplayMode(_window, &fullscreenMode);
+		} else {
+			SDL_SetWindowSize(_window, width, height);
+		}
+
+		SDL_SetWindowFullscreen(_window, fullscreenFlags);
+		SDL_SetWindowGrab(_window, (flags & SDL_WINDOW_INPUT_GRABBED) ? SDL_TRUE : SDL_FALSE);
+	}
+
 	if (!_window) {
 		return false;
 	}
-	setupIcon();
+
+	_lastFlags = flags;
 
 	return true;
 }
 
 void SdlWindow::destroyWindow() {
-	SDL_DestroyWindow(_window);
-	_window = nullptr;
+	if (_window) {
+		if (!(_lastFlags & fullscreenMask)) {
+			SDL_GetWindowPosition(_window, &_lastX, &_lastY);
+		}
+		SDL_DestroyWindow(_window);
+		_window = nullptr;
+	}
 }
 #endif
diff --git a/backends/platform/sdl/sdl-window.h b/backends/platform/sdl/sdl-window.h
index b62860960d..d75e811f56 100644
--- a/backends/platform/sdl/sdl-window.h
+++ b/backends/platform/sdl/sdl-window.h
@@ -81,14 +81,14 @@ public:
 	SDL_Window *getSDLWindow() const { return _window; }
 
 	/**
-	 * Creates a new SDL window (and destroys the old one).
+	 * Creates or updates the SDL window.
 	 *
 	 * @param width   Width of the window.
 	 * @param height  Height of the window.
 	 * @param flags   SDL flags passed to SDL_CreateWindow
 	 * @return true on success, false otherwise
 	 */
-	bool createWindow(int width, int height, uint32 flags);
+	bool createOrUpdateWindow(int width, int height, uint32 flags);
 
 	/**
 	 * Destroys the current SDL window.
@@ -99,6 +99,15 @@ protected:
 	SDL_Window *_window;
 
 private:
+	uint32 _lastFlags;
+
+	/**
+	 * Switching between software and OpenGL modes requires the window to be
+	 * destroyed and recreated. These properties store the position of the last
+	 * window so the new window will be created in the same place.
+	 */
+	int _lastX, _lastY;
+
 	bool _inputGrabState;
 	Common::String _windowCaption;
 #endif