Bug 308438 - When compositing is disabled, opening a window which blocks compositing *enables* it
Summary: When compositing is disabled, opening a window which blocks compositing *enab...
Status: RESOLVED FIXED
Alias: None
Product: kwin
Classification: Plasma
Component: compositing (show other bugs)
Version: 4.9.2
Platform: Compiled Sources Linux
: NOR normal
Target Milestone: 4.10
Assignee: KWin default assignee
URL: https://git.reviewboard.kde.org/r/106...
Keywords:
: 311081 (view as bug list)
Depends on:
Blocks:
 
Reported: 2012-10-15 14:01 UTC by Ralf Jung
Modified: 2013-05-20 09:58 UTC (History)
1 user (show)

See Also:
Latest Commit:
Version Fixed In: 4.9.3
mgraesslin: ReviewRequest+


Attachments
Fix handling of compositing-blocking windows when compositing is already disabled (1.71 KB, patch)
2012-10-15 14:43 UTC, Ralf Jung
Details
Fix handling of compositing-blocking windows when compositing is already disabled (v2) (2.16 KB, patch)
2012-10-15 16:06 UTC, Ralf Jung
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Ralf Jung 2012-10-15 14:01:01 UTC
When a window sets the "block compositing" property while compositing is disabled, compositing gets enabled.

Reproducible: Always

Steps to Reproduce:
1. Start with compositing enabled. Open a Konsole.
2. In the window menu, choose "More Actions - Special Window Settings..."
3. In the "Appearance" tab, enable "Block compositing", choose "Force" and "yes"
4. Close all window (at least all Konsole windows and all the others which have the "block composite" property).
5. Verify that opening a Konsole will block compositing, and closing it will re-enable compositing. Make sure it is closed.
6. Hit Ctrl+Alt+F12 to disable compositing
7. Start a Konsole.
Actual Results:  
Compositing is *enabled*.

Expected Results:  
Of course, compositing should stay disabled.
Comment 1 Ralf Jung 2012-10-15 14:15:21 UTC
Looking at the code, this is actually not surprising:
In composite.cpp, Workspace::updateCompositeBlocking *toggles* compositing when seeing the first client with the property set. The suspend call should probably be something like

QMetaObject::invokeMethod(this, "suspendCompositing", Qt::QueuedConnection, Q_ARG(bool, true));

However, if compositing was disabled, this needs to be remembered for when the last blocking window closes. This could be achieved by not setting compositingBlocked to true if it was already blocked, but the comment says

do NOT check for "compositingSuspended" or "!compositing()"

Nevertheless, a few lines down, compositingSuspended *is* checked.

This is all in the KDE/4.9 branch.
Comment 2 Ralf Jung 2012-10-15 14:43:44 UTC
Created attachment 74560 [details]
Fix handling of compositing-blocking windows when  compositing is already disabled


Attached patch fixes the problem here. Is it okay to go, should I open a review request or use a different approach?
Comment 3 Thomas Lübking 2012-10-15 14:56:28 UTC
The patch pollutes the "compositingBlocked" state if compositingSuspended && c->isBlockingCompositing()

Also since there's now a distinct suspend(bool) slot, the compositingSuspended check can be just omitted.

Notice that git master fundamentally differs for the introduction of the compositor class and as of now, composite blocking seems to not wor at all.
Comment 4 Martin Flöser 2012-10-15 15:27:09 UTC
> Notice that git master fundamentally differs for the introduction of the
> compositor class and as of now, composite blocking seems to not wor at all.
what? I'm quite sure I tested that (as I was afraid of breaking it).

But given the changes between 4.9 and master in that area we will probably 
need two patches.
Comment 5 Ralf Jung 2012-10-15 15:35:55 UTC
(In reply to comment #3)
> The patch pollutes the "compositingBlocked" state if compositingSuspended &&
> c->isBlockingCompositing()
How that? If the window is blocking compositing and compositing was not yet blocked (this merges the formerly two ifs), it will get blocked unless it's manually suspended.
I am available in IRC (#kde-devel).

(In reply to comment #4)
> > Notice that git master fundamentally differs for the introduction of the
> > compositor class and as of now, composite blocking seems to not wor at all.
> what? I'm quite sure I tested that (as I was afraid of breaking it).
Yes, it seems to ignore the blocking here as well - probably a different bug?
Comment 6 Martin Flöser 2012-10-15 15:41:29 UTC
(In reply to comment #5)
> (In reply to comment #4)
> > > Notice that git master fundamentally differs for the introduction of the
> > > compositor class and as of now, composite blocking seems to not wor at all.
> > what? I'm quite sure I tested that (as I was afraid of breaking it).
> Yes, it seems to ignore the blocking here as well - probably a different bug?
probably, given the rather large refactoring.
Comment 7 Thomas Lübking 2012-10-15 15:59:57 UTC
(In reply to comment #4)

> what? I'm quite sure I tested that (as I was afraid of breaking it).
the function is not called, only updateCompositeBlocking(NULL) is.(

In reply to comment #5)
> How that?
As far as i can say from the plaintext patch, "compositingBlocked = true;" is not called if "compositingSuspended == true", despite "c->isBlockingCompositing()" is, thus the state becomes invalid.
Comment 8 Ralf Jung 2012-10-15 16:01:56 UTC
(In reply to comment #7)
> In reply to comment #5)
> > How that?
> As far as i can say from the plaintext patch, "compositingBlocked = true;"
> is not called if "compositingSuspended == true", despite
> "c->isBlockingCompositing()" is, thus the state becomes invalid.
That's true, but it is necessary: If compositingBlocked were set to true, then compositing would be resumed after the last compositing-blocking window is closed - even if compositing was manually suspended previously.
Comment 9 Ralf Jung 2012-10-15 16:06:51 UTC
Created attachment 74564 [details]
Fix handling of compositing-blocking windows when compositing is already disabled (v2)

Updated patch
Comment 10 Thomas Lübking 2012-10-15 16:10:15 UTC
(In reply to comment #7)

> > what? I'm quite sure I tested that (as I was afraid of breaking it).
> the function is not called, only updateCompositeBlocking(NULL) is.

workspace.cpp / ::createClient binds 
connect(c, SIGNAL(blockingCompositingChanged(KWin::Client*)), m_compositor, SLOT(updateCompositeBlocking(KWin::Client*)));

after conditional invocation in "!c->manage(w, is_mapped)"

The connects could and likely should happen after the construction and before the management attempt.
Comment 11 Thomas Lübking 2012-10-15 16:25:58 UTC
(In reply to comment #8)

> That's true, but it is necessary: If compositingBlocked were set to true,
> then compositing would be resumed after the last compositing-blocking window
> is closed - even if compositing was manually suspended previously.

This will require a slight more complex check that store resumeCompositing alongside setting the blocked state and alters it with external susped/resume requests, shifting the variable meaning won't resolve this ;-P
Comment 12 Ralf Jung 2012-10-15 16:28:57 UTC
I can't think of a situation in which the behaviour this patch implements is wrong. No matter whether the initial compositing state is enabled or disabled, it will be restored after the last window is closed.
If you could tell me under which circumstances the behaviour should be different, I can try to fix that.
Comment 13 Thomas Lübking 2012-10-15 17:09:36 UTC
suspend compositing (alt+shift+f12)
launch blocking client // m_blocked = false
resume compositing (alt+shift+f12)
launch blocking client // m_blocked = true
suspend compositing (alt+shift+f12)
close blocking clients // (m_blocked == true)
-> compositing gets resumed
Comment 14 Ralf Jung 2012-10-15 17:45:03 UTC
I see. I will open a review request as this has more side-effects than I anticipated.
Comment 15 Ralf Jung 2012-10-15 18:04:37 UTC
(In reply to comment #13)
> suspend compositing (alt+shift+f12)
> launch blocking client // m_blocked = false
> resume compositing (alt+shift+f12)
> launch blocking client // m_blocked = true
> suspend compositing (alt+shift+f12)
Actually this does not work - when the blocking client is launched, compositing is automatically suspended. Of course, it will be resumed later. If however I hit alt+shift+F12 two times, then the new patch shows the desired behaviour of not resuming composite.
Comment 16 Thomas Lübking 2012-10-15 19:06:43 UTC
Sorry, forgot a resume, but just tested and this:

suspend compositing (alt+shift+f12)
launch blocking client // m_blocked = false
resume compositing (alt+shift+f12)
launch blocking client // m_blocked = true
resume compositing (alt+shift+f12)
suspend compositing (alt+shift+f12)
close blocking clients // (m_blocked == true)

resumes the compositor.

That's no wonder, because the system currently does not store the compositing state request from user (shortcut, dbus) and system (blocked for window) independently.

If that's demanded (given the initial bug, it seems so) the m_suspended state either needs to be a flag (ByUser = 1, ByBlockingClient = 2) and the resume after a block may only happen
m_suspended &= ~ByBlockingClient;
if (!m_suspended)
   ::invoke("resume");

or there need to be an additional boolean about the user driven compositing state.
Comment 17 Ralf Jung 2012-10-15 20:48:12 UTC
The current patch is at https://git.reviewboard.kde.org/r/106900/ and I will reply over there to avois splitting the thread too much.
(Should've opened a review request from the start, sorry for that)
Comment 18 Martin Flöser 2012-10-15 20:56:29 UTC
(In reply to comment #17)
> (Should've opened a review request from the start, sorry for that)
no problem at all, the systems are connected (well at least kind of)
Comment 19 Ralf Jung 2012-11-01 13:04:43 UTC
Git commit d61ee825eceac212c4aa4d011c47a82ec92ade9a by Ralf Jung.
Committed on 26/10/2012 at 16:18.
Pushed by ralfjung into branch 'KDE/4.9'.

do not resume compositing when it's suspended and a client blocks it

REVIEW: 106900

M  +6    -3    kwin/composite.cpp

http://commits.kde.org/kde-workspace/d61ee825eceac212c4aa4d011c47a82ec92ade9a
Comment 20 Ralf Jung 2012-11-01 21:41:46 UTC
The bug is not actually fixed, behaviour got just a bit better.
Comment 21 Martin Flöser 2012-11-02 06:45:59 UTC
(In reply to comment #20)
> The bug is not actually fixed, behaviour got just a bit better.
but it is fixed in master, isn't it?
Comment 22 Ralf Jung 2012-11-02 09:42:05 UTC
(In reply to comment #21)
> but it is fixed in master, isn't it?
Last time I checked, no: If compositing is disabled, opening a blocking window will (correctly) do nothing. However, closing that window again will start compositing.
To prevent this, the original compositing state needs to be remembered when a blocking window is opened, and properly restored. However, since the user can manually toggle compositing even when a blocking window is present, it's not entirely clear what the expected behaviour is.
I can propose a patch against master which implements the behaviour I'd expect, and open a review request. As far as I remember, Thomas expects a different behaviour.
Comment 23 Thomas Lübking 2012-12-03 14:07:03 UTC
*** Bug 311081 has been marked as a duplicate of this bug. ***
Comment 24 Martin Flöser 2013-01-09 07:33:53 UTC
@Ralf: what's the current state in 4.10 branch? If it's not fixed we should try to get it fixed before RC 3
Comment 25 Ralf Jung 2013-01-09 11:21:21 UTC
The current state is - as far as I know (didn't test in a while) - still as described above: If compositing is disabled, opening a blocking window will (correctly) do nothing. However, closing that window again will start compositing.
We could not agree on the correct behaviour to fix this. I proposed the following behaviour in my patch:
KWin maintains a flag whether compositing was automatically suspended by a window. This flag is reset whenever the user manually toggles compositing.
When a window with composite-blocking is opened, if compositing is already disabled or the flag is set, nothing happens. If compositing is enabled, the flag is set and compositing is disabled.
When the last blocking window is closed, if the flag is set, compositing is enabled and the flag reset.
Comment 26 Thomas Lübking 2013-01-23 21:12:24 UTC
Git commit 7df368b35d27b27c3cb7da4802daa541a89dc773 by Thomas Lübking.
Committed on 09/01/2013 at 16:03.
Pushed by luebking into branch 'master'.

32 bit compositing suspension

REVIEW: 108304

M  +25   -29   kwin/composite.cpp
M  +15   -8    kwin/composite.h

http://commits.kde.org/kde-workspace/7df368b35d27b27c3cb7da4802daa541a89dc773
Comment 27 Ralf Jung 2013-05-20 09:53:55 UTC
I see this buggy behaviour in KDE 4.10.3 again - no idea when it came back.
Comment 28 Ralf Jung 2013-05-20 09:58:52 UTC
Oops, I'm sorry. This is just the "compositing is re-enabled when the window is closed" bug, which I thought was fixed in KDE 4.10, but it is not. Sorry for the noise.