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: Only set selection when there is not already a source. #99372

Merged

Conversation

tdaven
Copy link
Contributor

@tdaven tdaven commented Nov 17, 2024

Might fix #95649.

While trying to use the editor I got really frustrated with inconsistent clipboard behavior when using wayland. Often, when text has been selected and then copied, the paste afterwards results in nothing. Trying to copy again usually succeeds.

The best way I have found to reproduce is to copy/paste once, scroll with mouse wheel, try copy/paste again.

When the source selection was reused, wayland seems to send a cancel event for the old(see _wl_data_source_on_cancelled), which causes us to destroy the source (which obviously means we didn't send anything to the clipboard now). Since the selection is now cleared, the second attempt works.

Setting the selection only when there is not already a source seems to work.

@tdaven tdaven requested a review from a team as a code owner November 17, 2024 22:27
@Riteo
Copy link
Contributor

Riteo commented Nov 17, 2024

Thank you for the PR!

Sorry for not looking into this before, I somehow missed that issue 😅

Nothing in the spec seems to explicitly report this and I don't think I've ever noticed this behavior in sway, so I think that it might be some ambiguity in how the protocol's described.

My theory is that my executing wl_data_device::set_selection each time, the compositor "replaces" the current source and sends a cancelled event, which in turn destroys itself.

I think that the proper solution might be to avoid calling it if we already have a valid source, since it will be destroyed if something else replaces it.

@Riteo Riteo added this to the 4.4 milestone Nov 17, 2024
@tdaven tdaven force-pushed the fix-inconsistent-wayland-clipboard branch from 4050131 to a46f111 Compare November 17, 2024 22:51
@tdaven tdaven changed the title Wayland: Always create a data source when setting text Wayland: Only set selection when there is not already a source. Nov 17, 2024
@Riteo
Copy link
Contributor

Riteo commented Nov 17, 2024

Code checks out! Note that you accidentally left a bunch of tabs in one line, before the TODO comment, making the CI fail:
formatted diff showing the whitespace

(Uploaded a pic because I have no idea how to highlight that through markdown)

I'll formally approve once I make a quick test with sway to make sure that it didn't bring regressions there (it shouldn't).

Co-authored-by: Riteo Siuga <riteo@posteo.net>
@tdaven tdaven force-pushed the fix-inconsistent-wayland-clipboard branch from a46f111 to 0d9a705 Compare November 17, 2024 23:00
@tdaven
Copy link
Contributor Author

tdaven commented Nov 17, 2024

Always forget to run format when doing small edits....one day I'll remember.

@Riteo
Copy link
Contributor

Riteo commented Nov 17, 2024

Always forget to run format when doing small edits....one day I'll remember.

BTW, we have pre-commit support now, which should do this automatically for each commit ;)

See https://docs.godotengine.org/en/stable/contributing/development/code_style_guidelines.html#pre-commit-hook

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.

Found no regressions on sway!

@Repiteo Repiteo merged commit 778f26e into godotengine:master Nov 18, 2024
20 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Nov 18, 2024

Thanks!

@tdaven tdaven deleted the fix-inconsistent-wayland-clipboard branch November 18, 2024 15:45
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.

Inconsistent Copy Behavior in Script Editor when Using Wayland Display Driver
3 participants