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

Wayland: Fix primary clipboard handling #96540

Merged

Conversation

hunterkepley
Copy link
Contributor

@hunterkepley hunterkepley commented Sep 3, 2024

Fixes #96214

Fixes issue with middle-mouse click (even if enabled) not working with Wayland. From the issue, it works when Wayland is not in use, same system settings. But, when Wayland is in use, it breaks

@hunterkepley hunterkepley requested a review from a team as a code owner September 3, 2024 22:16
@hunterkepley
Copy link
Contributor Author

pre-commit checks passed

@fire fire changed the title fix | Allow Wayland to paste using middlemouse button Allow Wayland to paste using middlemouse button Sep 3, 2024
@hunterkepley
Copy link
Contributor Author

@fire Working on one more fix, this only fixes single line text boxes such as searching for a project

@hunterkepley
Copy link
Contributor Author

hunterkepley commented Sep 3, 2024

@fire fix for script editing merged, squashed

There is also the possibility of wrapping the clipboard_get function in the Wayland display server within the clipboard_get_primary.
It seems to not be used anywhere else besides these places, and the latter does not seemingly work with Wayland

@hunterkepley
Copy link
Contributor Author

hunterkepley commented Sep 3, 2024

Went ahead and pushed the other fix. Confirmed the function which returns an empty string from the clipboard primary is not working + only used in two places

Wrapping it like this prevents if statements + code smell

Unless changes are requested, this MR is done being edited by me

Copy link
Contributor

@Riteo Riteo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, thank you for your contribution! The primary clipboard is different form the regular clipboard, so aliasing it to the regular one is not an acceptable solution unfortunately.

Instead, the proper fix would be to fix the primary clipboard logic itself.

Calling DisplayServer.clipboard_get_primary from GDScript (with a random string in my primary clipboard) returns this in a verbose log:

Selecting media type "text/plain;charset=utf-8" from offered types.
Read chunk of 15 bytes.
Done reading 15 bytes.

So it's clearly trying to read something, although in the end it returns an empty string. The regular and the primary clipboard work in a very similar way, so perhaps there's some difference between their handling? I can see a flush instead of a roundtrip but it seems to make no difference once swapped in.

The part from the above log is printed from _read_fd in platforms/linuxbsd/wayland/wayland_thread.cpp, you could take a look there.

I hope this is a good starting point and thank you again for contributing!

@hunterkepley
Copy link
Contributor Author

thanks- will make a fix soon

@hunterkepley
Copy link
Contributor Author

@Riteo PTAL- i am embarassed at how long it took to find this issue. We were throwing away the data gotten back from primary_get_mime. Assigned it to data and all is well

@Riteo
Copy link
Contributor

Riteo commented Sep 4, 2024

@hunterkepley lol no worries I did not notice that either! Thank you for your patience!

Copy link
Contributor

@Riteo Riteo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks perfect! One small nitpick, please rename the commit and title to match the new change, prefixing it with Wayland: (e.g. Wayland: Fix primary clipboard handling)

@hunterkepley
Copy link
Contributor Author

Will do 🙂

@hunterkepley hunterkepley changed the title Allow Wayland to paste using middlemouse button Wayland: Fix primary clipboard handling Sep 4, 2024
@akien-mga akien-mga merged commit 9abf86f into godotengine:master Sep 4, 2024
20 checks passed
@akien-mga
Copy link
Member

Thanks! And congrats for your first merged Godot contribution 🎉

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.

Mouse wheel paste doesn't work on Wayland editor
4 participants