Merge pull request #11475 from akallabeth/sdl-dialog-improve

[client,sdl] wrap connection dialog

client/SDL/SDL3/dialogs/CMakeLists.txt

@@ -34,6 +34,8 @@ set(SRCS
     sdl_selectlist.cpp
     sdl_connection_dialog.cpp
     sdl_connection_dialog.hpp
+    sdl_connection_dialog_wrapper.cpp
+    sdl_connection_dialog_wrapper.hpp
 )
 
 list(APPEND LIBS sdl3_client_res winpr)

client/SDL/SDL3/dialogs/sdl_connection_dialog.cpp

@@ -493,46 +493,19 @@ Uint32 SDLConnectionDialog::timeout(void* pvthis, [[maybe_unused]] SDL_TimerID t
 }
 
 SDLConnectionDialogHider::SDLConnectionDialogHider(freerdp* instance)
-    : SDLConnectionDialogHider(get(instance))
+    : SDLConnectionDialogHider(instance->context)
 {
 }
 
-SDLConnectionDialogHider::SDLConnectionDialogHider(rdpContext* context)
-    : SDLConnectionDialogHider(get(context))
+SDLConnectionDialogHider::SDLConnectionDialogHider(rdpContext* context) : _context(context)
 {
-}
-
-SDLConnectionDialogHider::SDLConnectionDialogHider(SDLConnectionDialog* dialog) : _dialog(dialog)
-{
-	if (_dialog)
-	{
-		_visible = _dialog->visible();
-		if (_visible)
-		{
-			_dialog->hide();
-		}
-	}
+	auto sdl = get_context(_context);
+	_visible = sdl->dialog.isVisible();
+	sdl->dialog.show(false);
 }
 
 SDLConnectionDialogHider::~SDLConnectionDialogHider()
 {
-	if (_dialog && _visible)
-	{
-		_dialog->show();
-	}
-}
-
-SDLConnectionDialog* SDLConnectionDialogHider::get(freerdp* instance)
-{
-	if (!instance)
-		return nullptr;
-	return get(instance->context);
-}
-
-SDLConnectionDialog* SDLConnectionDialogHider::get(rdpContext* context)
-{
-	auto sdl = get_context(context);
-	if (!sdl)
-		return nullptr;
-	return sdl->connection_dialog.get();
+	auto sdl = get_context(_context);
+	sdl->dialog.show(_visible);
 }

client/SDL/SDL3/dialogs/sdl_connection_dialog.hpp

@@ -34,6 +34,15 @@
 class SDLConnectionDialog
 {
   public:
+	enum MsgType
+	{
+		MSG_NONE,
+		MSG_INFO,
+		MSG_WARN,
+		MSG_ERROR,
+		MSG_DISCARD
+	};
+
 	explicit SDLConnectionDialog(rdpContext* context);
 	SDLConnectionDialog(const SDLConnectionDialog& other) = delete;
 	SDLConnectionDialog(const SDLConnectionDialog&& other) = delete;
@@ -58,15 +67,6 @@ class SDLConnectionDialog
 	bool handle(const SDL_Event& event);
 
   private:
-	enum MsgType
-	{
-		MSG_NONE,
-		MSG_INFO,
-		MSG_WARN,
-		MSG_ERROR,
-		MSG_DISCARD
-	};
-
 	bool createWindow();
 	void destroyWindow();
 
@@ -114,8 +114,6 @@ class SDLConnectionDialogHider
 	explicit SDLConnectionDialogHider(freerdp* instance);
 	explicit SDLConnectionDialogHider(rdpContext* context);
 
-	explicit SDLConnectionDialogHider(SDLConnectionDialog* dialog);
-
 	SDLConnectionDialogHider(const SDLConnectionDialogHider& other) = delete;
 	SDLConnectionDialogHider(SDLConnectionDialogHider&& other) = delete;
 	SDLConnectionDialogHider& operator=(const SDLConnectionDialogHider& other) = delete;
@@ -124,9 +122,6 @@ class SDLConnectionDialogHider
 	~SDLConnectionDialogHider();
 
   private:
-	SDLConnectionDialog* get(freerdp* instance);
-	static SDLConnectionDialog* get(rdpContext* context);
-
-	SDLConnectionDialog* _dialog = nullptr;
+	rdpContext* _context = nullptr;
 	bool _visible = false;
 };

client/SDL/SDL3/dialogs/sdl_connection_dialog_wrapper.cpp

@@ -0,0 +1,183 @@
+/**
+ * FreeRDP: A Remote Desktop Protocol Implementation
+ * SDL Client helper dialogs
+ *
+ * Copyright 2025 Armin Novak <armin.novak@thincast.com>
+ * Copyright 2025 Thincast Technologies GmbH
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include "sdl_connection_dialog_wrapper.hpp"
+#include "sdl_connection_dialog.hpp"
+#include "../sdl_utils.hpp"
+
+void SdlConnectionDialogWrapper::create(rdpContext* context)
+{
+	std::unique_lock lock(_mux);
+	_connection_dialog = std::make_unique<SDLConnectionDialog>(context);
+	sdl_push_user_event(SDL_EVENT_USER_UPDATE_CONNECT_DIALOG);
+}
+
+void SdlConnectionDialogWrapper::destroy()
+{
+	std::unique_lock lock(_mux);
+	_connection_dialog.reset();
+	sdl_push_user_event(SDL_EVENT_USER_UPDATE_CONNECT_DIALOG);
+}
+
+bool SdlConnectionDialogWrapper::isRunning() const
+{
+	std::unique_lock lock(_mux);
+	if (!_connection_dialog)
+		return false;
+	return _connection_dialog->running();
+}
+
+bool SdlConnectionDialogWrapper::isVisible() const
+{
+	std::unique_lock lock(_mux);
+	if (!_connection_dialog)
+		return false;
+	return _connection_dialog->visible();
+}
+
+bool SdlConnectionDialogWrapper::handleEvent(const SDL_Event& event)
+{
+	std::unique_lock lock(_mux);
+	if (!_connection_dialog)
+		return false;
+	return _connection_dialog->handle(event);
+}
+
+WINPR_ATTR_FORMAT_ARG(1, 0)
+static std::string format(WINPR_FORMAT_ARG const char* fmt, va_list ap)
+{
+	va_list ap1;
+	va_copy(ap1, ap);
+	const int size = vsnprintf(nullptr, 0, fmt, ap1);
+	va_end(ap1);
+
+	if (size < 0)
+		return "";
+
+	std::string msg;
+	msg.resize(static_cast<size_t>(size) + 1);
+
+	va_list ap2;
+	va_copy(ap2, ap);
+	(void)vsnprintf(msg.data(), msg.size(), fmt, ap2);
+	va_end(ap2);
+	return msg;
+}
+
+void SdlConnectionDialogWrapper::setTitle(const char* fmt, ...)
+{
+	va_list ap;
+	va_start(ap, fmt);
+	setTitle(format(fmt, ap));
+	va_end(ap);
+}
+
+void SdlConnectionDialogWrapper::setTitle(const std::string& title)
+{
+	std::unique_lock lock(_mux);
+	_title = title;
+	sdl_push_user_event(SDL_EVENT_USER_UPDATE_CONNECT_DIALOG);
+}
+
+void SdlConnectionDialogWrapper::showInfo(const char* fmt, ...)
+{
+	va_list ap;
+	va_start(ap, fmt);
+	showInfo(format(fmt, ap));
+	va_end(ap);
+}
+
+void SdlConnectionDialogWrapper::showInfo(const std::string& info)
+{
+	show(SDLConnectionDialog::MSG_INFO, info);
+}
+
+void SdlConnectionDialogWrapper::showWarn(const char* fmt, ...)
+{
+	va_list ap;
+	va_start(ap, fmt);
+	showWarn(format(fmt, ap));
+	va_end(ap);
+}
+
+void SdlConnectionDialogWrapper::showWarn(const std::string& info)
+{
+	show(SDLConnectionDialog::MSG_WARN, info);
+}
+
+void SdlConnectionDialogWrapper::showError(const char* fmt, ...)
+{
+	va_list ap;
+	va_start(ap, fmt);
+	showError(format(fmt, ap));
+	va_end(ap);
+}
+
+void SdlConnectionDialogWrapper::showError(const std::string& error)
+{
+	show(SDLConnectionDialog::MSG_ERROR, error);
+}
+
+void SdlConnectionDialogWrapper::show(SDLConnectionDialog::MsgType type, const std::string& msg)
+{
+	std::unique_lock lock(_mux);
+	_message = msg;
+	_type = type;
+	_visible = true;
+	sdl_push_user_event(SDL_EVENT_USER_UPDATE_CONNECT_DIALOG);
+}
+
+void SdlConnectionDialogWrapper::show(bool visible)
+{
+	std::unique_lock lock(_mux);
+	_visible = visible;
+	sdl_push_user_event(SDL_EVENT_USER_UPDATE_CONNECT_DIALOG);
+}
+
+void SdlConnectionDialogWrapper::handleShow()
+{
+	std::unique_lock lock(_mux);
+	if (!_connection_dialog)
+		return;
+
+	_connection_dialog->setTitle(_title.c_str());
+	if (!_visible)
+	{
+		_connection_dialog->hide();
+		return;
+	}
+
+	switch (_type)
+	{
+		case SDLConnectionDialog::MSG_INFO:
+			_connection_dialog->showInfo(_message.c_str());
+			break;
+		case SDLConnectionDialog::MSG_WARN:
+			_connection_dialog->showWarn(_message.c_str());
+			break;
+		case SDLConnectionDialog::MSG_ERROR:
+			_connection_dialog->showError(_message.c_str());
+			break;
+		default:
+			break;
+	}
+
+	_connection_dialog->show();
+}

client/SDL/SDL3/dialogs/sdl_connection_dialog_wrapper.hpp

@@ -0,0 +1,80 @@
+/**
+ * FreeRDP: A Remote Desktop Protocol Implementation
+ * SDL Client helper dialogs
+ *
+ * Copyright 2025 Armin Novak <armin.novak@thincast.com>
+ * Copyright 2025 Thincast Technologies GmbH
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#pragma once
+
+#include <mutex>
+#include <memory>
+#include <string>
+
+#include <winpr/platform.h>
+
+#include "sdl_connection_dialog.hpp"
+
+class SdlConnectionDialogWrapper
+{
+  public:
+	SdlConnectionDialogWrapper() = default;
+	~SdlConnectionDialogWrapper() = default;
+
+	SdlConnectionDialogWrapper(const SdlConnectionDialogWrapper& other) = delete;
+	SdlConnectionDialogWrapper(SdlConnectionDialogWrapper&& other) = delete;
+
+	SdlConnectionDialogWrapper& operator=(const SdlConnectionDialogWrapper& other) = delete;
+	SdlConnectionDialogWrapper& operator=(SdlConnectionDialogWrapper&& other) = delete;
+
+	void create(rdpContext* context);
+	void destroy();
+
+	bool isRunning() const;
+	bool isVisible() const;
+
+	bool handleEvent(const SDL_Event& event);
+
+	WINPR_ATTR_FORMAT_ARG(2, 3)
+	void setTitle(WINPR_FORMAT_ARG const char* fmt, ...);
+	void setTitle(const std::string& title);
+
+	WINPR_ATTR_FORMAT_ARG(2, 3)
+	void showInfo(WINPR_FORMAT_ARG const char* fmt, ...);
+	void showInfo(const std::string& info);
+
+	WINPR_ATTR_FORMAT_ARG(2, 3)
+	void showWarn(WINPR_FORMAT_ARG const char* fmt, ...);
+	void showWarn(const std::string& info);
+
+	WINPR_ATTR_FORMAT_ARG(2, 3)
+	void showError(WINPR_FORMAT_ARG const char* fmt, ...);
+	void showError(const std::string& error);
+
+	void show(SDLConnectionDialog::MsgType type, const std::string& msg);
+
+	void show(bool visible = true);
+
+	void handleShow();
+
+  private:
+	mutable std::mutex _mux;
+	std::string _title;
+	std::string _message;
+	bool _visible = false;
+	SDLConnectionDialog::MsgType _type = SDLConnectionDialog::MSG_NONE;
+	std::unique_ptr<SDLConnectionDialog> _connection_dialog;
+};

client/SDL/SDL3/dialogs/sdl_dialogs.cpp

@@ -212,29 +212,25 @@ SSIZE_T sdl_retry_dialog(freerdp* instance, const char* what, size_t current,
 	auto settings = instance->context->settings;
 	const BOOL enabled = freerdp_settings_get_bool(settings, FreeRDP_AutoReconnectionEnabled);
 	const size_t delay = freerdp_settings_get_uint32(settings, FreeRDP_TcpConnectTimeout);
-	std::lock_guard<CriticalSection> lock(sdl->critical);
-	if (!sdl->connection_dialog)
-		return WINPR_ASSERTING_INT_CAST(ssize_t, delay);
 
-	sdl->connection_dialog->setTitle("Retry connection to %s",
-	                                 freerdp_settings_get_server_name(instance->context->settings));
+	sdl->dialog.setTitle("Retry connection to %s",
+	                     freerdp_settings_get_server_name(instance->context->settings));
 
 	if ((strcmp(what, "arm-transport") != 0) && (strcmp(what, "connection") != 0))
 	{
-		sdl->connection_dialog->showError("Unknown module %s, aborting", what);
+		sdl->dialog.showError("Unknown module %s, aborting", what);
 		return -1;
 	}
 
 	if (current == 0)
 	{
 		if (strcmp(what, "arm-transport") == 0)
-			sdl->connection_dialog->showWarn("[%s] Starting your VM. It may take up to 5 minutes",
-			                                 what);
+			sdl->dialog.showWarn("[%s] Starting your VM. It may take up to 5 minutes", what);
 	}
 
 	if (!enabled)
 	{
-		sdl->connection_dialog->showError(
+		sdl->dialog.showError(
 		    "Automatic reconnection disabled, terminating. Try to connect again later");
 		return -1;
 	}
@@ -242,16 +238,16 @@ SSIZE_T sdl_retry_dialog(freerdp* instance, const char* what, size_t current,
 	const size_t max = freerdp_settings_get_uint32(settings, FreeRDP_AutoReconnectMaxRetries);
 	if (current >= max)
 	{
-		sdl->connection_dialog->showError(
+		sdl->dialog.showError(
 		    "[%s] retries exceeded. Your VM failed to start. Try again later or contact your "
 		    "tech support for help if this keeps happening.",
 		    what);
 		return -1;
 	}
 
-	sdl->connection_dialog->showInfo("[%s] retry %" PRIuz "/%" PRIuz ", delaying %" PRIuz
-	                                 "ms before next attempt",
-	                                 what, current, max, delay);
+	sdl->dialog.showInfo("[%s] retry %" PRIuz "/%" PRIuz ", delaying %" PRIuz
+	                     "ms before next attempt",
+	                     what, current, max, delay);
 	return WINPR_ASSERTING_INT_CAST(ssize_t, delay);
 }
 

client/SDL/SDL3/sdl_freerdp.cpp

@@ -225,14 +225,6 @@ static const struct sdl_exit_code_map_t* sdl_map_entry_by_code(int exit_code)
 	return nullptr;
 }
 
-static void sdl_hide_connection_dialog(SdlContext* sdl)
-{
-	WINPR_ASSERT(sdl);
-	std::lock_guard<CriticalSection> lock(sdl->critical);
-	if (sdl->connection_dialog)
-		sdl->connection_dialog->hide();
-}
-
 static const struct sdl_exit_code_map_t* sdl_map_entry_by_error(UINT32 error)
 {
 	for (const auto& x : sdl_exit_code_map)
@@ -288,12 +280,7 @@ static int error_info_to_error(freerdp* instance, DWORD* pcode, char** msg, size
  * It can be used to reset invalidated areas. */
 static BOOL sdl_begin_paint(rdpContext* context)
 {
-	rdpGdi* gdi = nullptr;
-	auto sdl = get_context(context);
-
-	WINPR_ASSERT(sdl);
-
-	gdi = context->gdi;
+	auto gdi = context->gdi;
 	WINPR_ASSERT(gdi);
 	WINPR_ASSERT(gdi->primary);
 	WINPR_ASSERT(gdi->primary->hdc);
@@ -554,16 +541,14 @@ static BOOL sdl_pre_connect(freerdp* instance)
 		if (!sdl_wait_for_init(sdl))
 			return FALSE;
 
-		std::lock_guard<CriticalSection> lock(sdl->critical);
 		if (!freerdp_settings_get_bool(settings, FreeRDP_UseCommonStdioCallbacks))
-			sdl->connection_dialog = std::make_unique<SDLConnectionDialog>(instance->context);
-		if (sdl->connection_dialog)
 		{
-			sdl->connection_dialog->setTitle("Connecting to '%s'",
-			                                 freerdp_settings_get_server_name(settings));
-			sdl->connection_dialog->showInfo(
-			    "The connection is being established\n\nPlease wait...");
+			sdl->dialog.create(sdl->context());
 		}
+
+		sdl->dialog.setTitle("Connecting to '%s'", freerdp_settings_get_server_name(settings));
+		sdl->dialog.showInfo("The connection is being established\n\nPlease wait...");
+
 		if (!sdl_detect_monitors(sdl, &maxWidth, &maxHeight))
 			return FALSE;
 
@@ -644,7 +629,7 @@ static void sdl_cleanup_sdl(SdlContext* sdl)
 
 	std::lock_guard<CriticalSection> lock(sdl->critical);
 	sdl->windows.clear();
-	sdl->connection_dialog.reset();
+	sdl->dialog.destroy();
 
 	sdl_destroy_primary(sdl);
 
@@ -728,11 +713,12 @@ static BOOL sdl_create_windows(SdlContext* sdl)
 
 static BOOL sdl_wait_create_windows(SdlContext* sdl)
 {
-	std::unique_lock<CriticalSection> lock(sdl->critical);
-	sdl->windows_created.clear();
-	if (!sdl_push_user_event(SDL_EVENT_USER_CREATE_WINDOWS, sdl))
-		return FALSE;
-	lock.unlock();
+	{
+		std::unique_lock<CriticalSection> lock(sdl->critical);
+		sdl->windows_created.clear();
+		if (!sdl_push_user_event(SDL_EVENT_USER_CREATE_WINDOWS, sdl))
+			return FALSE;
+	}
 
 	HANDLE handles[] = { sdl->windows_created.handle(), freerdp_abort_event(sdl->context()) };
 
@@ -753,9 +739,7 @@ static bool shall_abort(SdlContext* sdl)
 	{
 		if (sdl->rdp_thread_running)
 			return false;
-		if (!sdl->connection_dialog)
-			return true;
-		return !sdl->connection_dialog->running();
+		return !sdl->dialog.isRunning();
 	}
 	return false;
 }
@@ -804,18 +788,17 @@ static int sdl_run(SdlContext* sdl)
 			SDL_LogDebug(SDL_LOG_CATEGORY_APPLICATION, "got event %s [0x%08" PRIx32 "]",
 			             sdl_event_type_str(windowEvent.type), windowEvent.type);
 #endif
-			std::lock_guard<CriticalSection> lock(sdl->critical);
-			/* The session might have been disconnected while we were waiting for a new SDL event.
-			 * In that case ignore the SDL event and terminate. */
-			if (freerdp_shall_disconnect_context(sdl->context()))
-				continue;
-
-			if (sdl->connection_dialog)
 			{
-				if (sdl->connection_dialog->handle(windowEvent))
-				{
+				std::lock_guard<CriticalSection> lock(sdl->critical);
+				/* The session might have been disconnected while we were waiting for a new SDL
+				 * event. In that case ignore the SDL event and terminate. */
+				if (freerdp_shall_disconnect_context(sdl->context()))
 					continue;
-				}
+			}
+
+			if (sdl->dialog.handleEvent(windowEvent))
+			{
+				continue;
 			}
 
 			auto point2pix = [](Uint32 win_id, float& x, float& y)
@@ -994,6 +977,9 @@ static int sdl_run(SdlContext* sdl)
 				case SDL_EVENT_CLIPBOARD_UPDATE:
 					sdl->clip.handle_update(windowEvent.clipboard);
 					break;
+				case SDL_EVENT_USER_UPDATE_CONNECT_DIALOG:
+					sdl->dialog.handleShow();
+					break;
 				case SDL_EVENT_USER_QUIT:
 				default:
 					if ((windowEvent.type >= SDL_EVENT_DISPLAY_FIRST) &&
@@ -1095,7 +1081,7 @@ static BOOL sdl_post_connect(freerdp* instance)
 	auto sdl = get_context(context);
 
 	// Retry was successful, discard dialog
-	sdl_hide_connection_dialog(sdl);
+	sdl->dialog.show(false);
 
 	if (freerdp_settings_get_bool(context->settings, FreeRDP_AuthenticationOnly))
 	{
@@ -1197,19 +1183,14 @@ static void sdl_client_cleanup(SdlContext* sdl, int exit_code, const std::string
 				break;
 			default:
 			{
-				std::lock_guard<CriticalSection> lock(sdl->critical);
-				if (sdl->connection_dialog && !error_msg.empty())
-				{
-					sdl->connection_dialog->showError(error_msg.c_str());
-					showError = true;
-				}
+				sdl->dialog.showError(error_msg);
 			}
 			break;
 		}
 	}
 
 	if (!showError)
-		sdl_hide_connection_dialog(sdl);
+		sdl->dialog.show(false);
 
 	sdl->exit_code = exit_code;
 	sdl_push_user_event(SDL_EVENT_USER_QUIT);
@@ -1280,7 +1261,7 @@ static int sdl_client_thread_connect(SdlContext* sdl, std::string& error_msg)
 				exit_code = SDL_EXIT_CONN_FAILED;
 		}
 
-		sdl_hide_connection_dialog(sdl);
+		sdl->dialog.show(false);
 	}
 
 	return exit_code;
@@ -1333,7 +1314,7 @@ static int sdl_client_thread_run(SdlContext* sdl, std::string& error_msg)
 			if (client_auto_reconnect(instance))
 			{
 				// Retry was successful, discard dialog
-				sdl_hide_connection_dialog(sdl);
+				sdl->dialog.show(false);
 				continue;
 			}
 			else

client/SDL/SDL3/sdl_freerdp.hpp

@@ -24,6 +24,7 @@
 #include <map>
 #include <atomic>
 #include <queue>
+#include <mutex>
 
 #include <freerdp/freerdp.h>
 #include <freerdp/client/rdpei.h>
@@ -40,7 +41,7 @@
 #include "sdl_clip.hpp"
 #include "sdl_utils.hpp"
 #include "sdl_window.hpp"
-#include "dialogs/sdl_connection_dialog.hpp"
+#include "dialogs/sdl_connection_dialog_wrapper.hpp"
 
 using SDLSurfacePtr = std::unique_ptr<SDL_Surface, decltype(&SDL_DestroySurface)>;
 
@@ -97,9 +98,8 @@ class SdlContext
 
 	SDL_PixelFormat sdl_pixel_format = SDL_PIXELFORMAT_UNKNOWN;
 
-	std::unique_ptr<SDLConnectionDialog> connection_dialog;
-
 	std::atomic<bool> rdp_thread_running;
 
 	std::queue<std::vector<SDL_Rect>> _queue;
+	SdlConnectionDialogWrapper dialog;
 };

client/SDL/SDL3/sdl_utils.hpp

@@ -53,7 +53,9 @@ enum
 	SDL_EVENT_USER_CERT_RESULT,
 	SDL_EVENT_USER_SHOW_RESULT,
 	SDL_EVENT_USER_AUTH_RESULT,
-	SDL_EVENT_USER_SCARD_RESULT
+	SDL_EVENT_USER_SCARD_RESULT,
+
+	SDL_EVENT_USER_UPDATE_CONNECT_DIALOG
 };
 
 typedef struct