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

Fix primary clipboard warning #54045

Merged

Conversation

ConteZero
Copy link
Contributor

Remove warning, should fix #54037

@akien-mga
Copy link
Member

Can the getter also trigger errors on platforms where it's not supported, while using LineEdit/TextEdit normally?

@akien-mga akien-mga added this to the 4.0 milestone Oct 20, 2021
@ConteZero ConteZero force-pushed the primary_clipboard_linux_fix_warning branch from 01837fc to 09058fd Compare October 20, 2021 19:54
@ConteZero
Copy link
Contributor Author

Yes, you are right, now I have also removed the error message from the getter.
Unfortunately I can only test on Linux so I didn't notice the problem.

@akien-mga
Copy link
Member

Looking at the implementation again, I think I was a bit too fast to merge it. The code in LineEdit and TextEdit assumes that the Linux-specific primary clipboard actions are valid, and handles them regardless of that fact.

So that means that hitting middle mouse button on Windows will actually deselect, change caret position, and grab focus - when nothing else happens because it's not a valid action in this environment.

Maybe there should be a DisplayServer::get_singleton()->clipboard_has_primary() that returns whether the DisplayServer supports primary clipboards at all. If not, the events should not be handled. CC @Paulb23

@ConteZero
Copy link
Contributor Author

Initially the code in line_edit, rich_text_label and text_edit was inside
#ifdef LINUX_ENABLED
...
#endif

then @Paulb23 suggested to remove them.
I could re-add the IFDEF in the code where the getter is called, this way it will not interfere with other platforms behavior.

@Paulb23
Copy link
Member

Paulb23 commented Oct 20, 2021

Ah, good point.

DisplayServer already contains a method has_feature(Feature p_feature), should be okay to add FEATURE_CLIPBOARD_PRIMARY to that, we already do something similar for IME support.

@ConteZero ConteZero force-pushed the primary_clipboard_linux_fix_warning branch from 09058fd to 580a4ae Compare October 20, 2021 21:25
@ConteZero ConteZero requested review from a team as code owners October 20, 2021 21:25
@ConteZero
Copy link
Contributor Author

I've added FEATURE_CLIPBOARD_PRIMARY

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Aside from my last review comment, this looks good to merge.

@ConteZero ConteZero force-pushed the primary_clipboard_linux_fix_warning branch from 580a4ae to 8c48b4a Compare October 23, 2021 14:05
@akien-mga akien-mga merged commit b2ab5cb into godotengine:master Oct 23, 2021
@akien-mga
Copy link
Member

Thanks!

@ConteZero ConteZero deleted the primary_clipboard_linux_fix_warning branch March 15, 2022 08:08
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.

Primary clipboard is not supported by this display server error upon clicking on TextEdit
3 participants