Fix capturing QDialog that included the whole desktop area
ClosedPublic

Authored by anemeth on Feb 20 2018, 12:24 AM.

Details

Summary

QDialog may add a 1x1 sized desktop window in the top left corner when capturing with Window Under Cursor.
Now it ignores that.

BUG: 376350

Test Plan

Before:

After:

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.
anemeth requested review of this revision.Feb 20 2018, 12:24 AM
anemeth created this revision.
anemeth edited the summary of this revision. (Show Details)Feb 20 2018, 12:27 AM
anemeth edited the test plan for this revision. (Show Details)
anemeth added reviewers: Spectacle, rkflx.
anemeth added a project: Spectacle.
anemeth edited the test plan for this revision. (Show Details)
anemeth edited the summary of this revision. (Show Details)
anemeth edited the summary of this revision. (Show Details)
ngraham accepted this revision.Feb 20 2018, 3:54 AM
ngraham added a subscriber: ngraham.

Works great, thanks! Can reproduce the original bug as well as the fact that this fixes it, and I didn't find any regressions.

However, as sometimes happens, while testing for regressions, I stumbled upon an un-reported pre-existing bug: https://bugs.kde.org/show_bug.cgi?id=390762

I'll wait for @rkflx to have a chance to check this patch out, too.

This revision is now accepted and ready to land.Feb 20 2018, 3:54 AM
anemeth added a comment.EditedFeb 20 2018, 12:01 PM

I stumbled upon an un-reported pre-existing bug: https://bugs.kde.org/show_bug.cgi?id=390762

"Window Under Cursor" captures the area of the window, that's why it includes things that are above it too. I don't know if it's a bug.
"Active Window" captures only the window itself, like you would expect.
It's a confusing naming.
Why are both needed?
What would we lose if we renamed "Active Window" to just "Window" and a user clicks on the window to capture itself only?

rkflx added a comment.EditedFeb 20 2018, 12:06 PM

This might be more of a usability issue, i.e. it's not clear immediately what's the difference between "Window Under Cursor" and "Active Window", in particular in terms of how overlapping (child and non-child) windows will be captured and whether child windows will be included or not. With "child window" I mean those like the configure dialog of an application. Don't forget about "Capture the current pop-up only" either.

I think this might be worth a task on Spectacle's workboard.


Active Window:

Window under Cursor (also note the incorrect shadows…):

FWIW I would be strongly in favor of collapsing the two modes into one, as long as we don't lose any functionality (e.g. under X11, is it still possible to capture transient "windows" like menus?).

rkflx added a comment.Feb 20 2018, 2:24 PM

Please open a new task for that so we can figure out what it is Spectacle currently supports, what is broken and needs to be fixed and how we can present this better to the user.

If you mean what we support under Wayland: Much less, but as this is still evolving let's not consider that case for now.

rkflx added a comment.Feb 20 2018, 2:32 PM

(e.g. under X11, is it still possible to capture transient "windows" like menus?).

The GUI supports it, but as noted in other bugs, there are problems. I now promoted this to https://bugs.kde.org/show_bug.cgi?id=390787.

Do you have any sources which say why this functionality which worked before is now unsupported? Perhaps a Qt change?

rkflx added a comment.Feb 20 2018, 2:58 PM

Now for the actual review: Code seems sensible and I could not break it so far.

@anemeth Do you know why a "QDialog without a parent" creates this 1x1px window? Is this needed, or is it a bug which should be solved elsewhere?

src/PlatformBackends/X11ImageGrabber.cpp
436

const

rkflx added a comment.Feb 20 2018, 2:59 PM

@ngraham Do you think we should land this to stable?

I too could not break it, but there is some risk, as we don't fully understand why what we're working around happens in the first place. My vote is for master this time.

anemeth added a comment.EditedFeb 20 2018, 3:07 PM

@anemeth Do you know why a "QDialog without a parent" creates this 1x1px window? Is this needed, or is it a bug which should be solved elsewhere?

Well I can only guess why this happens.
The best solution would be that apps don't use QDialog as main window parent class, but they do and we have to work around it.

Code snippet from the test case (KFloppy):

FloppyData(QWidget* parent = 0);

If KFloppy is called directly then parent won't be set and will default to 0.
Maybe this is a Qt bug?
If parent is 0 then make the desktop the parent which is for some reason a 1x1 window at the top left corner of the screen?

anemeth updated this revision to Diff 27614.Feb 20 2018, 3:12 PM

Changed variable desktopRect to const

anemeth marked an inline comment as done.Feb 20 2018, 3:13 PM
rkflx accepted this revision.EditedFeb 20 2018, 3:21 PM

If parent is 0 then make the desktop the parent which is for some reason a 1x1 window at the top left corner of the screen.

Tested this in IceWM where I could reproduce the bug, so I guess it's not something Plasma could fix. However, it would be great if you could open a bug over at https://bugreports.qt.io to determine if this is correct or not.

This revision was automatically updated to reflect the committed changes.

Please open a new task for that so we can figure out what it is Spectacle currently supports, what is broken and needs to be fixed and how we can present this better to the user.

T8033: Collapse "Active Window" and "Window under Cursor" modes

rkflx added a comment.Feb 21 2018, 7:31 PM

Looked a bit more into this, e.g. by diffing xwininfo -tree -root with and without KFloppy and comparing with xprop. Seems like the window id of the 1x1 window is set for WM_TRANSIENT_FOR(WINDOW) and WM_CLIENT_LEADER(WINDOW). Also, said window id is gone once KFloppy quit.

Let's just wait and see whether this change causes any fallout, I don't consider it likely.