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

Update wl-paste handling and return None for some errors in grabclipboard() on Linux #7745

Merged
merged 13 commits into from
Feb 5, 2024

Conversation

nik012003
Copy link
Contributor

@nik012003 nik012003 commented Jan 22, 2024

Simpified logic and made it more robust against edge cases ( see the allowed_errors list ).
Doing error checking this way, makes the behaviour of this function for x11 and wayland platforms more similar to darwin and windows systems.

src/PIL/ImageGrab.py Outdated Show resolved Hide resolved
src/PIL/ImageGrab.py Outdated Show resolved Hide resolved
"Nothing is copied", # wl-paste, when the clipboard is empty
"not available", # wl-paste, when an image isn't available
"cannot convert", # xclip, when an image isn't available
"There is no owner", # xclip, when the clipboard isn't initialized
Copy link
Member

@radarhere radarhere Jan 23, 2024

Choose a reason for hiding this comment

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

Could you describe how I can manually reproduce this "xclip, when the clipboard isn't initialized" error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can reproduce that by logging oit of gnome, and logging back in. Then run the xclip command. I can confirm this happens on fedora 39.

]
if any(e in err for e in allowed_errors):
return None
msg = f"{args[0]} error: {err.strip() if err else 'Unknown error'}"
Copy link
Member

Choose a reason for hiding this comment

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

Did you actually find a scenario where an error wasn't returned, meaning that you needed "Unknown error"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not really, but I would like to be as possible, since we are calling userspace programs which could change their behaviour anytime

@radarhere
Copy link
Member

makes the behaviour of this function for x11 and wayland platforms more similar to darwin and windows systems.

You mean because grabclipboard() now returns None under some circumstances on Linux.

@radarhere radarhere changed the title Refactor grabclipboard() for x11 and wayland Refactor grabclipboard() for Linux Jan 23, 2024
@nik012003
Copy link
Contributor Author

nik012003 commented Jan 23, 2024

makes the behaviour of this function for x11 and wayland platforms more similar to darwin and windows systems.

You mean because grabclipboard() now returns None under some circumstances on Linux.

Yes, that behaviour is not consitant with the macos/windows implementation of the function, so if it does break if the developer isn't careful.(i.e. doesn't put a try-except block all around it). With this PR the majority of x11 and wayland "non-errors" (i.e. clipboard empty, or not containg images) will return None like mac/win instead of raising some exceptions.

Comment on lines 164 to 165
"not available", # wl-paste, when an image isn't available
"cannot convert", # xclip, when an image isn't available
Copy link
Member

Choose a reason for hiding this comment

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

When I have text in the clipboard, I get a "not available" message.

So that seems like the "not available" comment should mention that it also applies to xclip?
Could you explain how to replicate "cannot convert"?

xclip.mov

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mhh, I get this result. I tried copying from multiple applications, gnome terminal, firefox, kitty, and they all respond with the "cannot convert" error...
Screencast from 2024-01-27 09-06-37.webm

I will look into the xclip code to be sure

Copy link
Contributor Author

@nik012003 nik012003 Jan 27, 2024

Choose a reason for hiding this comment

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

After further inspection, it seems that the version debian uses (and thus ubuntu) is different from the upstream one, and does indeed error out with fprintf(stderr, "Error: target %s not available\n", atom_name); instead of fprintf(stderr, " cannot convert %s selection to target '%s'\n", selection_name, atom_name);.

I will update the comment accordingly and take the opportunty to squash those typos commits :p

Copy link
Member

Choose a reason for hiding this comment

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

Simpified logic and made it more robust against edge cases ( see the `allowed_errors` list ). Doing error checking this way, makes the behaviour of this function for x11 and wayland platforms more silimar to darwin and windows systems.

fix typo src/PIL/ImageGrab.py

Co-authored-by: Ondrej Baranovič <nulano@nulano.eu>

fix typo src/PIL/ImageGrab.py

Co-authored-by: Andrew Murray <3112309+radarhere@users.noreply.github.com>

ImageGrab: \added debian edge case to comment
src/PIL/ImageGrab.py Outdated Show resolved Hide resolved
Co-authored-by: Andrew Murray <3112309+radarhere@users.noreply.github.com>
@radarhere radarhere changed the title Refactor grabclipboard() for Linux Update wl-paste handling and return None for some errors in grabclipboard() on Linux Jan 27, 2024
@radarhere
Copy link
Member

I've created nik012003#3 to test two failing wl-paste scenarios.

@radarhere radarhere merged commit 3bd58a8 into python-pillow:main Feb 5, 2024
55 of 56 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants