Compiz Core

Merge lp://qastaging/~vanvugt/compiz-core/fix-925979 into lp://qastaging/compiz-core

  1. fix-925979
  2. Merge into 0.9.7
Proposed by Daniel van Vugt
Status: Merged
Approved by: Sam Spilsbury
Approved revision: 2980
Merged at revision: 2983
Proposed branch: lp://qastaging/~vanvugt/compiz-core/fix-925979
Merge into: lp://qastaging/compiz-core
Diff against target: 14 lines (+1/-2)
1 file modified
src/event.cpp (+1/-2)
To merge this branch: bzr merge lp://qastaging/~vanvugt/compiz-core/fix-925979
Reviewer Review Type Date Requested Status
Sam Spilsbury Approve
Robert Carr Pending
Daniel van Vugt Pending
Alan Griffiths Pending
Review via email: mp+91615@code.qastaging.launchpad.net

This proposal supersedes a proposal from 2012-02-03.

Commit message

Fix regression: Compiz failed to pass key events through to other apps/windows in some cases (LP: #925979)

Revert back to how the code looked in the previous release (and oneiric).

Description of the change

Fortunately, this bug has not made it into any Compiz or Ubuntu release yet.

To post a comment you must log in.
Revision history for this message
Alan Griffiths (alan-griffiths) wrote : Posted in a previous version of this proposal

OK

review: Approve
Revision history for this message
Sam Spilsbury (smspillaz) wrote : Posted in a previous version of this proposal

I'm pretty sure this was added for a good reason, let me check with racarr on that one.

Revision history for this message
Sam Spilsbury (smspillaz) wrote : Posted in a previous version of this proposal

------------------------------------------------------------
revno: 2814.1.1
git commit: 6b8ecb998ce409462cfd72343696f4925aa33fdb
committer: Robert Carr <email address hidden>
timestamp: Thu 2011-09-15 11:28:55 -0400
message:
  event.cpp: Only ungrab the keyboard if the keystate is non zero.

Revision history for this message
Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal

Well it caused a regression. So whatever it was an attempt to fix should and probably can be fixed differently. In fact, I would be surprised if there wasn't a solution where we could remove XUngrabKeyboard and replace it with something else. But I don't know what else will work.

Whatever the alternative is, we should at least use the test case described in bug 925979.

Revision history for this message
Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal

Robert, could you please explain what you were fixing with this commit:
http://git.compiz.org/compiz/core/commit/?id=6b8ecb998ce409462cfd72343696f4925aa33fdb

Revision history for this message
Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal

Or rather: http://bazaar.launchpad.net/~compiz-team/compiz-core/0.9.5/revision/2815

Revision history for this message
Sam Spilsbury (smspillaz) wrote : Posted in a previous version of this proposal

On Sat, 4 Feb 2012, Daniel van Vugt wrote:

> Well it caused a regression. So whatever it was an attempt to fix should and probably can be fixed differently. In fact, I would be surprised if there wasn't a solution where we could remove XUngrabKeyboard and replace it with something else. But I don't know what else will work.
>
> Whatever the alternative is, we should at least use the test case described in bug 925979.

To be honest, I think this is the right approach, although I remember
racarr told me there was a good reason to only allow this on keys that
have a modifier (eg, not single keys). Perhaps that reason isn't relevant
anymore, I'll need to check.

I'll ask him when he gets in next week.

> --
> https://code.launchpad.net/~vanvugt/compiz-core/fix-925979/+merge/91412
> Your team Compiz Maintainers is subscribed to branch lp:compiz-core.
>

Revision history for this message
Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal

We could accommodate both. The issue with tapping a modifier is that it creates regular key events without any modifier state set. So maybe it needs:

 if (event->xkey.state || (keycode is a modifier))

But I'm reluctant to keep any "if" statement if Robert can't remember the justification for introducing it.

Revision history for this message
Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal

Needs resubmit for target branch lp:compiz-core (0.9.7)

review: Needs Resubmitting
Revision history for this message
Sam Spilsbury (smspillaz) wrote :

I had a quick chat with racarr today about this and the reason is most likely because of the super-key shortcuts in unity (eg super-1 super-2 etc). I think in this case, unity wasn't making the necessary key grabs for those keys. I'm going to test to make sure if it isn't and propose the relevant fix there so that we can get this one merged.

Other than that +1

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches