Merge into trunk : keyState : Code : QtMir

Reviewer	Review Type	Date Requested	Status
Gerry Boland (community)		2017-03-24	Approve on 2017-03-24
Unity8 CI Bot (community)	continuous-integration	2017-03-24	Approve on 2017-03-24
Review via email: mp+320930@code.qastaging.launchpad.net

Revision history for this message

Unity8 CI Bot (unity8-ci-bot) wrote on 2017-03-20: Posted in a previous version of this proposal

#

Click here to trigger a rebuild:
https://unity8-jenkins.ubuntu.com/job/lp-qtmir-ci/592/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Gerry Boland (gerboland) wrote on 2017-03-20: Posted in a previous version of this proposal

#

If the surface looses active focus, don't we inform Mir? Why not send the key release event then? Would be more efficient than checking on every key_up

Also, please avoid using singleton pattern, it makes testing impossible. And speaking of which, can you add test?

review: Needs Information

Revision history for this message

Daniel d'Andrada (dandrader) wrote on 2017-03-20: Posted in a previous version of this proposal

#

On 20/03/2017 14:08, Gerry Boland wrote:
> Review: Needs Information
>
> If the surface looses active focus, don't we inform Mir?

It's the other way around. Mir surface focus is what drives qml active
focus. Nowadays qml active focus by itself doesn't carry much meaning in
terms of window management.

> Why not send the key release event then? Would be more efficient than checking on every key_up

Because the key is actually still pressed. If the same window gets focus
again moments later and only then the key is lifted the client would
receive a second key_up event. Unless we keep track of what lies we have
told already to the clients or devise some other special rules.

> Also, please avoid using singleton pattern, it makes testing impossible.

Yeah, that was the easiest (laziest) way to get the two talking (event
feeder and policy). Will see if I can get something nicer. Suggestions?

But before I spend too much time on the implementation details of this
MP I would like to get feedback on the overall approach first.

> And speaking of which, can you add test?

For WindowManagementPolicy?

Revision history for this message

Gerry Boland (gerboland) wrote on 2017-03-21: Posted in a previous version of this proposal

#

> On 20/03/2017 14:08, Gerry Boland wrote:
> > Review: Needs Information
> >
> > If the surface looses active focus, don't we inform Mir?
>
> It's the other way around. Mir surface focus is what drives qml active
> focus. Nowadays qml active focus by itself doesn't carry much meaning in
> terms of window management.

There are more things in the QML scene that just MirSurfaces that can get activeFocus though. When activeFocus does switch to the search box in the AppDrawer (as example), we are indeed telling Mir somehow that the MirSurface has not got input focus (I just tested gedit, the cursor does stop flashing when the text box in the app drawer is focused). Since this bug occurs due to focus changing while modifier key is down, my initial thinking was we could use the focus change to implement a workaround.

What I don't know is if the WMpolicy if notified when the MirSurface it has selected to be focused has lost actual input focus (activeFocus). Do you know?

> > Why not send the key release event then? Would be more efficient than
> checking on every key_up
>
> Because the key is actually still pressed. If the same window gets focus
> again moments later and only then the key is lifted the client would
> receive a second key_up event. Unless we keep track of what lies we have
> told already to the clients or devise some other special rules.

This appears to be a QML subtlety we're working around, as a non-QML Qt shell would not suffer from this issue, right? So could the MirSurfaceItem keep track of the key state itself? If the MSI looses activeFocus, it send key-release to the client. Then if it gets focus again, check if key is still down (not sure how tbh, maybe QML sends the key to the Item again immediately??), and if so, re-send it to client.

I'm not 100% certain what is the best approach yet.

> > Also, please avoid using singleton pattern, it makes testing impossible.
>
> Yeah, that was the easiest (laziest) way to get the two talking (event
> feeder and policy). Will see if I can get something nicer. Suggestions?

Nothing better than the usual piping an object around, so everyone who needs it gets a pointer to it. Sorry :(

> But before I spend too much time on the implementation details of this
> MP I would like to get feedback on the overall approach first.

Ah ok, I hope I've supplied that at least

>
> > And speaking of which, can you add test?
>
> For WindowManagementPolicy?

More the logic you're adding. But please ignore me until you settle on the approach you want.

> On 20/03/2017 14:08, Gerry Boland wrote:
> > Review: Needs Information
> >
> > If the surface looses active focus, don't we inform Mir?
> 
> It's the other way around. Mir surface focus is what drives qml active
> focus. Nowadays qml active focus by itself doesn't carry much meaning in
> terms of window management.

There are more things in the QML scene that just MirSurfaces that can get activeFocus though. When activeFocus does switch to the search box in the AppDrawer (as example), we are indeed telling Mir somehow that the MirSurface has not got input focus (I just tested gedit, the cursor does stop flashing when the text box in the app drawer is focused). Since this bug occurs due to focus changing while modifier key is down, my initial thinking was we could use the focus change to implement a workaround.

What I don't know is if the WMpolicy if notified when the MirSurface it has selected to be focused has lost actual input focus (activeFocus). Do you know?

> >   Why not send the key release event then? Would be more efficient than
> checking on every key_up
> 
> Because the key is actually still pressed. If the same window gets focus
> again moments later and only then the key is lifted the client would
> receive a second key_up event. Unless we keep track of what lies we have
> told already to the clients or devise some other special rules.

This appears to be a QML subtlety we're working around, as a non-QML Qt shell would not suffer from this issue, right? So could the MirSurfaceItem keep track of the key state itself? If the MSI looses activeFocus, it send key-release to the client. Then if it gets focus again, check if key is still down (not sure how tbh, maybe QML sends the key to the Item again immediately??), and if so, re-send it to client.

I'm not 100% certain what is the best approach yet.

> > Also, please avoid using singleton pattern, it makes testing impossible.
> 
> Yeah, that was the easiest (laziest) way to get the two talking (event
> feeder and policy). Will see if I can get something nicer. Suggestions?

Nothing better than the usual piping an object around, so everyone who needs it gets a pointer to it. Sorry :(

> But before I spend too much time on the implementation details of this
> MP I would like to get feedback on the overall approach first.

Ah ok, I hope I've supplied that at least

> 
> > And speaking of which, can you add test?
> 
> For WindowManagementPolicy?

More the logic you're adding. But please ignore me until you settle on the approach you want.

Revision history for this message

Daniel d'Andrada (dandrader) wrote on 2017-03-21: Posted in a previous version of this proposal

#

On 21/03/2017 09:37, Gerry Boland wrote:
> This appears to be a QML subtlety we're working around, as a non-QML Qt shell would not suffer from this issue, right?

Yes. As I said, QQuickWindow input dispatching not perfectly suited for
shells.

> So could the MirSurfaceItem keep track of the key state itself?
Sure.

> If the MSI looses activeFocus, it send key-release to the client. Then if it gets focus again, check if key is still down (not sure how tbh, maybe QML sends the key to the Item again immediately??), and if so, re-send it to client.

It can't check the key state, but it can ignore a key-up for a key that
it didn't tell its surface it was down (because lied or whatever).

> I'm not 100% certain what is the best approach yet.

Working around QQuickWindow input dispatching will never be beautiful. So, should I go for the MirSurfaceItem-based key filtering/manipulation?

I'm fine either way. I put in the policy to avoid sending fake input to clients and as a general rule-of-thumb of "putting things in the miral policy"/"keep the policy in the loop".

Revision history for this message

Gerry Boland (gerboland) wrote on 2017-03-22: Posted in a previous version of this proposal

#

I think I'd prefer using MirSurfaceItem-based key filtering. Since it's working around the QQuickWindow/QML problem, it makes sense to me to confine the workaround in code only used in code exposed to QQuickWindow/QML.

Revision history for this message

Unity8 CI Bot (unity8-ci-bot) wrote on 2017-03-22: Posted in a previous version of this proposal

#

Click here to trigger a rebuild:
https://unity8-jenkins.ubuntu.com/job/lp-qtmir-ci/613/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Gerry Boland (gerboland) wrote on 2017-03-23: Posted in a previous version of this proposal

#

+++ src/modules/Unity/Application/mirsurface.h
Could you move the implementation of the PressedKey constructor into the cpp file please, it is big enough a header file.

+++ src/modules/Unity/Application/mirsurface.cpp
You're using QElapsedTimer to get the current value of the monotonic clock, yes? If so, we really only need one QElapsedTimer everywhere, so could make it static singeton and add a static function that calls msecsSinceReference on it and returns the val. wdyt?

Code makes sense, I just need to test. It does need a big comment explaining why we're doing this though. Do the QtWayland guys have the same issue, wonder if they do a similar fix or not...

Revision history for this message

Daniel d'Andrada (dandrader) wrote on 2017-03-23: Posted in a previous version of this proposal

#

> +++ src/modules/Unity/Application/mirsurface.h
> Could you move the implementation of the PressedKey constructor into the cpp
> file please, it is big enough a header file.

Done.

>
> +++ src/modules/Unity/Application/mirsurface.cpp
> You're using QElapsedTimer to get the current value of the monotonic clock,
> yes? If so, we really only need one QElapsedTimer everywhere, so could make it
> static singeton and add a static function that calls msecsSinceReference on it
> and returns the val. wdyt?

Right. Done.

> Code makes sense, I just need to test. It does need a big comment explaining
> why we're doing this though. Do the QtWayland guys have the same issue, wonder
> if they do a similar fix or not...

Added a big fat comment.

Revision history for this message

Unity8 CI Bot (unity8-ci-bot) wrote on 2017-03-23: Posted in a previous version of this proposal

#

Click here to trigger a rebuild:
https://unity8-jenkins.ubuntu.com/job/lp-qtmir-ci/616/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Unity8 CI Bot (unity8-ci-bot) wrote on 2017-03-24: Posted in a previous version of this proposal

#

Click here to trigger a rebuild:
https://unity8-jenkins.ubuntu.com/job/lp-qtmir-ci/619/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Unity8 CI Bot (unity8-ci-bot) wrote on 2017-03-24: Posted in a previous version of this proposal

#

Click here to trigger a rebuild:
https://unity8-jenkins.ubuntu.com/job/lp-qtmir-ci/627/rebuild

review: Approve (continuous-integration)

Revision history for this message

Unity8 CI Bot (unity8-ci-bot) wrote on 2017-03-24:

#

Click here to trigger a rebuild:
https://unity8-jenkins.ubuntu.com/job/lp-qtmir-ci/629/rebuild

review: Approve (continuous-integration)

Revision history for this message

Gerry Boland (gerboland) wrote on 2017-03-24:

#

How about using the anonymous namespace we have? I can stick this in there

static qint64 msecsSinceReference()
{
    static QElapsedTimer elapsedTimer;
    elapsedTimer.start();
    return elapsedTimer.msecsSinceReference();
}

and it works ok. After all it is just a function to return a timestamp, which is a purely internal implementation thing that doesn't need to be a class member IMO.

+ if (m_focused) {
releaseAllPressedKeys();
Why not on unfocus?

lp://qastaging/~dandrader/qtmir/keyState updated on 2017-03-24

628. By Daniel d'Andrada on 2017-03-24: Some refactoring as per review feedback

Revision history for this message

Daniel d'Andrada (dandrader) wrote on 2017-03-24:

#

On 24/03/2017 15:37, Gerry Boland wrote:
> How about using the anonymous namespace we have? I can stick this in there
>
> static qint64 msecsSinceReference()
> {
> static QElapsedTimer elapsedTimer;
> elapsedTimer.start();
> return elapsedTimer.msecsSinceReference();
> }
>
> and it works ok. After all it is just a function to return a timestamp, which is a purely internal implementation thing that doesn't need to be a class member IMO.

Yes, makes for less code. Done.

> + if (m_focused) {
> releaseAllPressedKeys();
> Why not on unfocus?

Because an unfocused surface, by definition, shouldn't be receiving
input events.

Revision history for this message

Unity8 CI Bot (unity8-ci-bot) wrote on 2017-03-24:

#

Click here to trigger a rebuild:
https://unity8-jenkins.ubuntu.com/job/lp-qtmir-ci/633/rebuild

review: Approve (continuous-integration)

Revision history for this message

Gerry Boland (gerboland) wrote on 2017-03-24:

#

> Because an unfocused surface, by definition, shouldn't be receiving
> input events.
Yeah. Not ideal, but as we have no reliable way to send those events just before a surface is send the unfocus event, this is the best that can be done.

Tested and works ok

review: Approve

QtMir

Merge lp://qastaging/~dandrader/qtmir/keyState into lp://qastaging/qtmir

Commit message

Description of the change

Preview Diff

Subscribers