Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[3.x] Added primary clipboard for Linux #54026

Merged
merged 1 commit into from
Mar 14, 2022

Conversation

ConteZero
Copy link
Contributor

This commit add support for primary clipboard on Linux to handle text paste with middle mouse button.
This is a backport of #53702 to 3.x branch.

@akien-mga
Copy link
Member

This should also include changes from follow up PRs (at least #54045, I didn't check if there was any other), otherwise it will exhibit the same issues.

@ConteZero
Copy link
Contributor Author

3.x branch does not have DisplayServer has_feature method so I've used some ifdef to solve #54045
I think it's the more safe and less invasive way to solve the problem.

@akien-mga akien-mga force-pushed the 3.x branch 2 times, most recently from 71cb8d3 to c58391c Compare January 6, 2022 22:40
@ConteZero ConteZero force-pushed the primary_clipboard_linux_3.x branch from 499d9a5 to 528a07e Compare March 14, 2022 09:16
@akien-mga
Copy link
Member

akien-mga commented Mar 14, 2022

3.x branch does not have DisplayServer has_feature method so I've used some ifdef to solve #54045 I think it's the more safe and less invasive way to solve the problem.

There is OS::has_feature which I think would be better for this than yet another define.

The command line is pretty long already :)

g++ -o platform/x11/os_x11.x11.tools.64.o -c -std=gnu++14 -Wctor-dtor-privacy -Wnon-virtual-dtor -Wnoexcept -Wplacement-new=1 -g3 -ggdb -Wno-error=return-type -pipe -fpie -Wall -Wextra -Wwrite-strings -Wno-unused-parameter -Wno-misleading-indentation -Wshadow-local -Walloc-zero -Wduplicated-branches -Wduplicated-cond -Wstringop-overflow=4 -Wlogical-op -Wattribute-alias=2 -Werror -DDEBUG_ENABLED -DDEV_ENABLED -DNO_EDITOR_SPLASH -DTOUCH_ENABLED -DALSA_ENABLED -DALSAMIDI_ENABLED -DPULSEAUDIO_ENABLED -D_REENTRANT -DJOYDEV_ENABLED -DUDEV_ENABLED -DX11_ENABLED -DUNIX_ENABLED -DOPENGL_ENABLED -DGLES_ENABLED -D_FILE_OFFSET_BITS=64 -DPTRCALL_ENABLED -DTOOLS_ENABLED -DMINIZIP_ENABLED -DZSTD_STATIC_LINKING_ONLY -DGLAD_ENABLED -DGLES_OVER_GL -DHAVE_MNTENT -Ithirdparty/freetype/include -Ithirdparty/libpng -Ithirdparty/glad -Ithirdparty/zstd -Ithirdparty/zlib -Iplatform/x11 -I. platform/x11/os_x11.cpp

Unlike DisplayServer::has_feature, it relies on strings, so I think you'd have to add logic to OS_X11::_check_internal_feature_support like this:

diff --git a/platform/x11/os_x11.cpp b/platform/x11/os_x11.cpp
index 7127447f14..4af06351a2 100644
--- a/platform/x11/os_x11.cpp
+++ b/platform/x11/os_x11.cpp
@@ -3357,7 +3357,17 @@ Error OS_X11::shell_open(String p_uri) {
 }
 
 bool OS_X11::_check_internal_feature_support(const String &p_feature) {
-	return p_feature == "pc";
+	if (p_feature == "pc") {
+		return true;
+	}
+
+#ifdef __linux__
+	if (p_feature == "primary_clipboard") {
+		return true;
+	}
+#endif
+
+	return false;
 }
 
 String OS_X11::get_config_path() const {

That's assuming that primary clipboard is a Linux-only feature and not something provided by *BSD systems too. Otherwise, you could then indeed just rely on the already existing X11_ENABLED define.

@ConteZero ConteZero force-pushed the primary_clipboard_linux_3.x branch from 528a07e to b4b8f62 Compare March 14, 2022 12:46
@ConteZero
Copy link
Contributor Author

I've removed the DEFINE

@akien-mga
Copy link
Member

akien-mga commented Mar 14, 2022

I'm still not sure why it's Linux specific though, as opposed to X11 specific. The implementation for primary clipboard doesn't make a distinction between GNU/Linux and FreeBSD/OpenBSD/NetBSD.

To clarify, the platform is called x11 on 3.x and linuxbsd on master because it's not only a GNU/Linux platform port. The same code (modulo a handful of defines) can compile on other Unix + Freedesktop platforms that support X11 such as several *BSD distributions.

@ConteZero
Copy link
Contributor Author

I'm still not sure why it's Linux specific though, as opposed to X11 specific. The implementation for primary clipboard doesn't make a distinction between GNU/Linux and FreeBSD/OpenBSD/NetBSD.

To clarify, the platform is called x11 on 3.x and linuxbsd on master because it's not only a GNU/Linux platform port. The same code (modulo a handful of defines) can compile on other Unix + Freedesktop platforms that support X11 such as several *BSD distributions.

I can only test on Linux so I really don't know if the primary clipboard code works on *BSD.
We can remove the
#ifdef __linux__
if some *BSD user can confirm that it works.

@akien-mga
Copy link
Member

Searching "OpenBSD X11 primary clipboard" gives a number of results which seem to infer that it does work. If it doesn't work, the X11 API would return an error and hopefully we already handle it gracefully anyway.

@ConteZero ConteZero force-pushed the primary_clipboard_linux_3.x branch from b4b8f62 to 2ff0735 Compare March 14, 2022 14:29
@ConteZero
Copy link
Contributor Author

Searching "OpenBSD X11 primary clipboard" gives a number of results which seem to infer that it does work. If it doesn't work, the X11 API would return an error and hopefully we already handle it gracefully anyway.

OK, I've removed the #ifdef

@akien-mga akien-mga merged commit ee818e1 into godotengine:3.x Mar 14, 2022
@akien-mga
Copy link
Member

Thanks!

@ConteZero ConteZero deleted the primary_clipboard_linux_3.x branch March 15, 2022 08:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants