It failed to capture the window area when the window was outside of the screen and showed a null image to the user when compositor was not enabled.
Now it only captures the area that is on the screen.
BUG: 390652
Details
Details
- Reviewers
rkflx ngraham broulik - Group Reviewers
Spectacle - Commits
- R166:5d2e20ce7e4c: Set screen bounds for active window grab when compositor is disabled
Diff Detail
Diff Detail
- Repository
- R166 Spectacle
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
Comment Actions
Thanks for the patch ;) I'll try to test as soon as a I find the time. Do you plan to submit more patches to KDE in the coming months?
Comment Actions
Thanks for the patch! Is this really the right fix, though? If a window is half off the screen, I'm not sure that only capturing the visible part when in Active Window mode is what's expected. It's Active Window, not Visible part of Active Window.
macOS's screenshot tool captures the whole window in this case, for example.
Comment Actions
It would be a shame if not, because you are talented! Let me know if I can do anything to help you in that regard…
@ngraham Only part of the visible screen can be captured when the compositor is disabled.
I don't know how possible is adding that feature. Maybe @rkflx knows.
I guess you know more about this than I do ;), but it makes sense this way. When the compositor is enabled, though, it should probably work? IIRC I even tested this (not sure ATM). Does this break now?
Comment Actions
It's interesting because it works if the window is off screen when "Active Window" mode is selected, but not when "Window Under Cursor" is selected.
Comment Actions
Oh right, this only affects the non-composited use case. Apparently my VM uses compositing, and I can now reproduce the original issue. This patch does what @anemeth says it does and doesn't break the composited use case, or any other use cases I tested.
Comment Actions
Thanks for this patch! It does not take into account device pixel ratio, ie. screen scaling. Here the screenshots turn into half (a quarter) the size they should have since I'm on a 2x scaled screen.
| src/PlatformBackends/X11ImageGrabber.cpp | ||
|---|---|---|
| 323–332 | you are setting rect twice | |
Comment Actions
For me there's still a weird case left where I can trigger the bug: Moving a window (I used Kate) under the bottom panel (or even lower, e.g only half of the height visible), Spectacle captures a null image. This happens after setting 1.5x scaling in kcmshell kcm_kscreen and restarting the session.
I don't get it for 1x scaling, though, and with compositing enabled it works for both scaling settings. Letting the window extend to the side is also fine (without the patch this used to break).
Comment Actions
Works for me:
Looks like you used Window Under Cursor, but here we are all about Active Window. Could you try again, please?
For me, with 1.5x scaling I still get a null image as soon as Kate touches the bottom edge of the screen (if it only touches the bottom panel it's fine, touching left/top/right is good too).
Comment Actions
Works for me:
Looks like you used Window Under Cursor, but here we are all about Active Window. Could you try again, please?
For me, with 1.5x scaling I still get a null image as soon as Kate touches the bottom edge of the screen (if it only touches the bottom panel it's fine, touching left/top/right is good too).
Oops, my bad. I tried again with 1.5x scaling, Active Window, and the compositor off and I can reproduce. @anemeth, here are the exact conditions under which you should be able to reproduce:
- Whole session uses 1.5x scaling (have to reboot afterwards)
- Kate window goes below the panel
- Compositor off (don't forgot to turn it off again after rebooting)
- Using Active Window mode
Comment Actions
Could this be an off-by-one error, perhaps some coordinate system mixup wrt. QRegion?
Comment Actions
- Whole session uses 1.5x scaling (have to reboot afterwards)
- Kate window goes below the panel
- Compositor off (don't forgot to turn it off again after rebooting)
- Using Active Window mode
I still can't reproduce the problem.
I even added the default bottom panel too to see if that is somehow influencing it, but no.
What am I doing wrong?
Comment Actions
I still can't reproduce the problem.
What am I doing wrong?
Perhaps a config/version thing? I cleared my config, now it's working. Let me bisect that tonight, so we know whether it'll affect users or not.
Comment Actions
@broulik I guess we have solved the HiDPI problem you reported. Any additional objections? Patch LGTM.
@anemeth Thanks, all issues fixed and I could not break it in any other ways. If you'd like to solve a similar bug, there is Bug 390787.
@ngraham As this is strictly a bug fix and in terms of risk for regressions this only adds an additional else, I tend to land this on stable. Is this okay with you?
After resetting the config a second time, suddenly the bug was there again! IOW, config related bug was a red herring.
However, I found a simpler way to reproduce. For me, resizing the (VM) display was enough to either fix or trigger the bug.
Yes, it can ;)
In getToplevelPixmap, adding debug output like this
QPixmap nativePixmap = getPixmapFromDrawable(rootWindow, rect); qDebug() << rect << endl << nativePixmap;
and decreasing the screen height results in this (the null images result from xcb_image_get failure, convertFromNative is not even reached):
QRect(150,268 712x706) QPixmap(null) QRect(150,268 712x691) QPixmap(QSize(712, 691),...) QRect(150,268 712x680) QPixmap(QSize(712, 680),...) QRect(150,268 712x674) QPixmap(QSize(712, 674),...) QRect(150,268 712x664) QPixmap(null)
Only the height is changing, sometimes resulting in failure, sometimes in success.
If y'all don't mind (as it took me some time to find the culprit), I'll make this a follow-up patch. Follow along in D10930: Capture reliably with compositing off regardless of screen size.
Comment Actions
@broulik You are blocking arc land.
Let me know if there are still concerns, otherwise I'll land the Diff anyway tonight.