-
Notifications
You must be signed in to change notification settings - Fork 97
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
Support GNOME Wayland sessions #75
base: master
Are you sure you want to change the base?
Support GNOME Wayland sessions #75
Conversation
There does not appear to be a universal screenshot tool for Wayland (grim is not supported on recent Ubuntu releases), but gnome-screenshot works okay if it is available
if sys.platform == 'linux': | ||
try: | ||
# ref: https://unix.stackexchange.com/questions/202891/how-to-know-whether-wayland-or-x11-is-being-used#comment1133584_371164 | ||
session_string = subprocess.check_output("loginctl show-session $(loginctl show-user $(whoami) -p Display --value) -p Type --value", shell=True, stderr=subprocess.STDOUT).decode().strip() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might want to leave a comment mentioning that this relies on the host being systemd based.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair point; that probably belongs in the commit message. How would you revise the commit message to reflect that nuance?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something along the lines of "This currently relies on processing information from loginctl provided by systemd in order to validate whether we are inside of a wayland session, which should work in most setups today."
else: | ||
# force loading before unlinking, Image.open() is lazy | ||
im.load() | ||
raise Exception("pyscreeze only supports Wayland session when gnome-screenshot is available") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO it would be better to treat this failure here way above right when checking whether gnome-screenshot is available or not
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seemed appropriate to minimize this patch by mimicking flow of logic used for scrot
. Handling the error condition inside a function also generates a more readable stack trace and affords us more flexibility when adding support for environments other than Gnome.
That said, I'm fine with throwing an exception earlier if @asweigart prefers that approach.
@ccallawa-intel This now has conflicts |
There does not appear to be a universal screenshot tool for Wayland
(grim is not supported on recent Ubuntu releases), but gnome-screenshot
works okay if it is available