feat: use spectacle for kde on wayland#84
feat: use spectacle for kde on wayland#84davidramiro wants to merge 1 commit intodshoreman:developfrom
Conversation
davidramiro
force-pushed
the
develop
branch
3 times, most recently
from
bed11fc to
8be8433
Compare
|
|
||
| spectacle "${args[@]}" "$1" | ||
|
|
||
| if [ ! -f "$1" ]; then |
There was a problem hiding this comment.
If you're wondering about this check, if the users clicks on 'Copy' instead of pressing enter or using 'Save', no file gets created and we use the clipboard instead.
There was a problem hiding this comment.
LGTM! On first glance... Problem is that's not the only time the file won't exist.
What if you abort? Slurp throws out a non-zero status if it fails for any reason, so Nextshot aborts. Spectacle seems to always report success though, regardless of which button was used… and so it continues, blissfully unaware that what's actually in the clipboard might not be a screenshot.
Whatever it was, it just shared it. There's no special status for pressing Esc either, so we can't even abort when you cancel the selection entirely.
Is there a good way to solve this?
We could use check_clipboard in the conditional which would at least prevent accidental text leaks, but it still doesn't guarantee the clipboard image came from Spectacle…
There was a problem hiding this comment.
tl;dr: This might be much less of a problem, so I'm inclined to ignore (or create a new issue for) it so it doesn't block merge.
Long version:
Done some more testing in a vm that won't stop locking up, and apparently forgot my own code. from_checkout already enforces the image/png type; check_clipboard won't be necessary:
$ nextshot -a
Waiting for selection...
After selection (and optional edits), press [ENTER] or click on 'Save' or 'Copy'.
Clipboard content is not available as requested type "image/png"
Use "wl-paste --list-types" to view available types.
Aborted due to error
It still won't help if the clipboard does have an image, but the closest we could get to that might be listening for its dbus signal. ScreenshotFailed doesn't get fired for a quit but we could abort if Spectacle exits without a ScreenshotTaken signal. Feels like a lot of effort though…
|
Thanks! I haven't used Spectacle before so I setup a Fedora/KDE VM to test. Seems it does have a The bigger problem might be Spectacle's apparent lack of any exit status beyond |
dshoreman
left a comment
dshoreman
left a comment
There was a problem hiding this comment.
Sorry for the lengthy review! I'm mostly curious of your thoughts on solving the clipboard issue (or maybe I'm just overthinking it?)
Are you able to sign the commit(s)? If you're short for time don't worry about all the nitpicks, I can deal with those when I merge 👍
| grim "${args[@]}" "$1" | ||
| } | ||
|
|
||
| shoot_wayland_plasma() { |
There was a problem hiding this comment.
Could you rename to shoot_plasma and move it above shoot_wayland? wayland_plasma is probably redundant with X11 slowly being dropped
| } | ||
|
|
||
| shoot_wayland_plasma() { | ||
| local args |
There was a problem hiding this comment.
| local args | |
| local args=(-b -n) | |
Since every call needs the background/no-notify flags we can set the common args inline here, unless shellcheck complains… Should be ok though.
| local args | ||
| if [ "$mode" = "selection" ]; then | ||
| echo "After selection (and optional edits), press [ENTER] or click on 'Save' or 'Copy'." >&2 | ||
| args=(-brn -o) |
There was a problem hiding this comment.
| args=(-brn -o) | |
| args+=(-r) |
same for the other cases too - -o will be better in the command which leaves only the mode option
| args=(-bfn -o) | ||
| fi | ||
|
|
||
| spectacle "${args[@]}" "$1" |
There was a problem hiding this comment.
| spectacle "${args[@]}" "$1" | |
| spectacle "${args[@]}" -o "$1" |
|
|
||
| spectacle "${args[@]}" "$1" | ||
|
|
||
| if [ ! -f "$1" ]; then |
There was a problem hiding this comment.
LGTM! On first glance... Problem is that's not the only time the file won't exist.
What if you abort? Slurp throws out a non-zero status if it fails for any reason, so Nextshot aborts. Spectacle seems to always report success though, regardless of which button was used… and so it continues, blissfully unaware that what's actually in the clipboard might not be a screenshot.
Whatever it was, it just shared it. There's no special status for pressing Esc either, so we can't even abort when you cancel the selection entirely.
Is there a good way to solve this?
We could use check_clipboard in the conditional which would at least prevent accidental text leaks, but it still doesn't guarantee the clipboard image came from Spectacle…
| is_wayland() { | ||
| [ "$NEXTSHOT_ENV" = "wayland" ] | ||
| } | ||
|
|
||
| is_plasma() { | ||
| [ "$DESKTOP_SESSION" = "plasma" ] | ||
| } |
There was a problem hiding this comment.
is_plasma first so it doesn't break up is_wayland and is_wayland_detected
2 participants
First of all, thanks for the great tool. Been using it on X11 for a while now and since I am now on Wayland, at least for KDE I could provide a way to use
spectaclefor selection and capture.One upside is that spectacle allows for on-the-fly editing within the capture region. Downside is that we rely on pressing either the enter key or the Save or Copy button on the spectacle region capture.
Let me know if that works for you!