Unity 8

Merge lp:~unity-team/unity8/physical-key-mapper into lp:unity8

physical-key-mapper
Merge into trunk

Proposed by Michał Sawicz on 2015-02-16

Status:

Merged

Approved by:

Albert Astals Cid on 2015-02-17

Approved revision:

1519

Merged at revision:

1644

Proposed branch:

lp:~unity-team/unity8/physical-key-mapper

Merge into:

lp:unity8

Diff against target:

437 lines (+248/-125)

6 files modified

qml/Components/Dialogs.qml (+4/-24)
qml/Components/PhysicalKeysMapper.qml (+113/-0)
qml/Components/VolumeKeyFilter.qml (+0/-63)
qml/Shell.qml (+11/-38)
tests/qmltests/CMakeLists.txt (+1/-0)
tests/qmltests/Components/tst_PhysicalKeysMapper.qml (+119/-0)

To merge this branch:

bzr merge lp:~unity-team/unity8/physical-key-mapper

High

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
PS Jenkins bot	continuous-integration	2015-02-16	Needs Fixing on 2015-02-17
Albert Astals Cid (community)		2015-02-16	Approve on 2015-02-17
Daniel d'Andrada	codewise	2015-02-16	Pending
kevin gunn	testing	2015-02-16	Pending
Michał Sawicz	ux		Pending
Review via email: mp+249890@code.launchpad.net

This proposal supersedes a proposal from 2014-12-10.

Commit Message

Create a PhysicalKeyMapper to handle all physical/hardware keys

Description of the Change

* Are there any related MPs required for this MP to build/function as expected? Please list.
No

* Did you perform an exploratory manual test run of your code change and any related functionality?

Yes

* Did you make sure that your branch does not contain spurious tags?

Yes

* If you changed the packaging (debian), did you subscribe the ubuntu-unity team to this MP?

N/A

* If you changed the UI, has there been a design review?

No, should there be?

Create a PhysicalKeyFilter to handle all physical/hardware keys on a mobile device. In addition, change the key combination for taking a screenshot from (Volume Up + Volume Down) to (Power + Volume Down).

--fixes lp:1390393

Albert Astals Cid (aacid) wrote on 2014-12-10: Posted in a previous version of this proposal

The else ifs need to be written in our style

} else if (volumeDownKeyPressed) {

instead of

}
else if (volumeDownKeyPressed) {

review: Needs Fixing

Albert Astals Cid (aacid) wrote on 2014-12-10: Posted in a previous version of this proposal

Something is wrong with the power button, after i press it to bring the phone back from sleep i always end up getting the shutdown dialog.

review: Needs Fixing

Daniel d'Andrada (dandrader) wrote on 2014-12-10: Posted in a previous version of this proposal

8 + * Authors:
9 + * Josh Arenson <email address hidden>

Better leave that out as this info gets quickly outdated overtime, as the code gets modified. bzr/git history does a better job at that.

Daniel d'Andrada (dandrader) wrote on 2014-12-10: Posted in a previous version of this proposal

Please follow the MP submission checklist regarding the commit message:
https://wiki.ubuntu.com/Process/Merges/Checklists/Unity8

Daniel d'Andrada (dandrader) wrote on 2014-12-10: Posted in a previous version of this proposal

> Please follow the MP submission checklist regarding the commit message:
> https://wiki.ubuntu.com/Process/Merges/Checklists/Unity8

Also the "--fixes lp:1390393" line is not necessary as CI will automatically add it when merging the branch.

Daniel d'Andrada (dandrader) wrote on 2014-12-10: Posted in a previous version of this proposal

If you're really going to make PhysicalKeysFilter also handle higher level things such as key mappings as you're doing for the screenshot, you should also absorb the code from Dialogs.qml that handles long power key press detection.

review: Needs Fixing

Daniel d'Andrada (dandrader) wrote on 2014-12-10: Posted in a previous version of this proposal

> If you're really going to make PhysicalKeysFilter also handle higher level
> things such as key mappings as you're doing for the screenshot, you should
> also absorb the code from Dialogs.qml that handles long power key press
> detection.

In that tone, I might be better to call it a key mapper instead of a key filter.

Josh Arenson (josharenson) wrote on 2014-12-10: Posted in a previous version of this proposal

> Something is wrong with the power button, after i press it to bring the phone
> back from sleep i always end up getting the shutdown dialog.

I will look into this, but it sounds like lp:1383277

PS Jenkins bot (ps-jenkins) wrote on 2014-12-11: Posted in a previous version of this proposal

FAILED: Continuous integration, rev:1492
http://jenkins.qa.ubuntu.com/job/unity8-ci/5020/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-vivid-touch/522
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-vivid-amd64-ci/185
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-vivid-i386-ci/185
    None: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-runner-vivid-mako/454/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/520
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/520/artifact/work/output/*zip*/output.zip
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/16505

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity8-ci/5020/rebuild

review: Needs Fixing (continuous-integration)

Daniel d'Andrada (dandrader) wrote on 2014-12-11: Posted in a previous version of this proposal

In qml/Shell.qml

"""
    Dialogs {
        ...
        Connections {
            target: physicalKeysMapper
            onPowerKeyLongPress: dialogs.showPowerDialog();
        }
        ...
    }
"""

Why not put the onPowerKeyLongPress line directly into PhysicalKeysMapper, like it's done for all other connections?

-----------------

In qml/Components/Dialogs.qml

"""
property bool dialogShown: false
"""

That should still be inside a QtObject { id: d } wrapper as it's still a private property (ie, not meant to be seem or used outside Dialogs.qml)

--------------------

In qml/Components/PhysicalKeysMapper.qml

"""
property bool eventAccepted: false

...

    function onKeyPressed(key) {
        ...
            eventAccepted = true;
        ...
            eventAccepted = true;
        ...
            eventAccepted = true;
        ...
        return eventAccepted;
    }

    function onKeyReleased(key) {
        ...
            eventAccepted = true;
        ...
            eventAccepted = true;
        ...
            eventAccepted = true;
        ...
        return inadvertently;
    }

"""

You init eventAccepted to false but once it's been set to true it will never come back to false again.
Meaning that you're inadvertently telling WindowKeysFilter to filter out *all* key events, so no application will ever get a physical key event again. This is a bug.

eventAccepted should not be a property but a internal variable of those funtioncs, like this:

"""
    function onKeyPressed(key) {
        var eventAccepted = false;
        ...
            eventAccepted = true;
        ...
            eventAccepted = true;
        ...
            eventAccepted = true;
        ...
        return eventAccepted;
    }
"""

--------------------

In qml/Components/PhysicalKeysMapper.qml

"""
        } else if (key == Qt.Key_VolumeDown) {
            eventAccepted = true;
            volumeDownKeyPressed = true;
        } else if (key == Qt.Key_VolumeUp) {
            eventAccepted = true;
            volumeUpKeyPressed = true;
        }
"""

Volume keys should *not* be accepted. Check the original code in Shell.qml.
If you accept them you're telling WindowKeysFilter to filter them out, so they won't get dispatched to the focused item in the qml scene (eg: the MirSurfaceItem of the focused app), meaning that apps won't get volume key events anymore. Check the documentation in plugins/Utils/windowkeysfilter.h

Volume keys are special in which we want both unity8 and the focused app to process them, unlike the power key, which should be processed only by unity8.

--------------------

In qml/Components/PhysicalKeysMapper.qml

"""
property bool aPowerKeyWasReleased: true
property bool powerKeyPressed: false

    property bool aVolumeKeyWasReleased: true
    property bool volumeDownKeyPressed: false
    property bool volumeUpKeyPressed: false
"""

Could you please wrap those properties in a "property QtObject d: QtObject { id: d }" as they're private?

review: Needs Fixing

Albert Astals Cid (aacid) wrote on 2014-12-11: Posted in a previous version of this proposal

Daniel's handling the review

review: Abstain

Daniel d'Andrada (dandrader) wrote on 2014-12-11: Posted in a previous version of this proposal

In qml/Components/PhysicalKeysMapper.qml

"""
signal bothVolumeKeysPressed();
"""

This signal is dead code as it's not being used in Shell.qml (since the screeshot is now power+volumeDown as has its own "virtual" key). Please remove it.

By the way, what's the rationale for changing the key combination of the screenshot? Which is all the more curious given that comment: https://bugs.launchpad.net/ubuntu/+source/unity8/+bug/1390393/comments/5

Daniel d'Andrada (dandrader) wrote on 2014-12-11: Posted in a previous version of this proposal

Again on this eventAccepted thing: I was thinking we could remove it entirely from PhysicalKeysMapper and keep the accept/reject decision making in the WindowKeysFilter functions in Shell.qml, as they currently are. That way you clearly separate the mapping from the filtering logic and keep the filtering logic close to the item that actually does the filtering, which is WindowKeysFilter.

key mapping and event filtering are clearly two separate, independent tasks.

Daniel d'Andrada (dandrader) wrote on 2014-12-11: Posted in a previous version of this proposal

The PhysicalKeysMapper test requires no GUI, so it should belong to qmlunittests, not qmluitests.

Tests that require no GUI are run by "make test" and are also run when building the debian packages.

This diff shows how to make it a GUI-less test:
http://paste.ubuntu.com/9474472/

review: Needs Fixing

Daniel d'Andrada (dandrader) wrote on 2014-12-11: Posted in a previous version of this proposal

The convention we've been using for test names is test_fooBar, not test_foo_bar.

Josh Arenson (josharenson) wrote on 2014-12-11: Posted in a previous version of this proposal

> In qml/Components/PhysicalKeysMapper.qml
>
> """
> signal bothVolumeKeysPressed();
> """
>
> This signal is dead code as it's not being used in Shell.qml (since the
> screeshot is now power+volumeDown as has its own "virtual" key). Please remove
> it.
>
> By the way, what's the rationale for changing the key combination of the
> screenshot? Which is all the more curious given that comment:
> https://bugs.launchpad.net/ubuntu/+source/unity8/+bug/1390393/comments/5

Had a lengthy discussion w/ mzanetti and AlbertA (and a bit w/ saviq) about this. Decided that this was the way to go. It _does_ resolve the bug, and I see no real architectural issues.

PS Jenkins bot (ps-jenkins) wrote on 2014-12-12: Posted in a previous version of this proposal

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity8-ci/5032/rebuild

review: Needs Fixing (continuous-integration)

Daniel d'Andrada (dandrader) wrote on 2014-12-12: Posted in a previous version of this proposal

PhysicalKeysMapper is still among the tests that require a GUI. The position where you put it in tests/qmltests/CMakeLists.txt matters. It should be right below this line:

"""
add_qml_test(utils/Unity/Test UnityTest)
"""

As it will get covered by this line:
"""
set(qmltest_DEFAULT_PROPERTIES ENVIRONMENT "QT_QPA_PLATFORM=minimal")
"""
which makes it use the minimal QPA plugin, which does not have display output.

-------------------

You still didn't make (or said anything about) the Dialog changes I asked here:
https://code.launchpad.net/~josharenson/unity8/physical_keys_filter/+merge/244244/comments/602555

review: Needs Fixing

Albert Astals Cid (aacid) wrote on 2014-12-16: Posted in a previous version of this proposal

Text conflict in qml/Components/Dialogs.qml
1 conflicts encountered.

PS Jenkins bot (ps-jenkins) wrote on 2014-12-19: Posted in a previous version of this proposal

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity8-ci/5063/rebuild

review: Approve (continuous-integration)

Daniel d'Andrada (dandrader) wrote on 2015-01-13: Posted in a previous version of this proposal

Approving as the last couple of issues have been addressed.

review: Approve

Michał Sawicz (saviq) wrote on 2015-01-13: Posted in a previous version of this proposal

This is not enough, now I'm just getting screenshots of the greeter unless I hold the power key long enough for the screenshot to be taken.

You'll need to suppress the suspend code in unity-system-compositor if volume down was pressed while power was pressed.

review: Needs Fixing (ux)

Josh Arenson (josharenson) wrote on 2015-01-15: Posted in a previous version of this proposal

I think this introduces a circular dependency.

Case 1 - power key is pressed first:
USC suspends phone

Case 2 - volume key is pressed first:
Unity displays volume dialog which shows up in all screenshots

There is no way to satisfy everyone without introducing a delay

Michał Sawicz (saviq) wrote on 2015-01-15: Posted in a previous version of this proposal

Sure there is, USC needs to suppress suspending the phone if voldown is pressed while power is pressed.

Michał Sawicz (saviq) wrote on 2015-01-15: Posted in a previous version of this proposal

Don't get me wrong, the split in handling those keys between unity8 and usc is something we need to fix, but before we get there, USC needs to gain some knowledge on what unity8 does (as it already does have the knowledge for power long-press - it doesn't suspend, it wakes if suspended, even).

Josh Arenson (josharenson) wrote on 2015-01-16: Posted in a previous version of this proposal

> Don't get me wrong, the split in handling those keys between unity8 and usc is
> something we need to fix, but before we get there, USC needs to gain some
> knowledge on what unity8 does (as it already does have the knowledge for power
> long-press - it doesn't suspend, it wakes if suspended, even).

Working on a USC change that handles the following state
if (power_down && volume_down)
disable_suspend()
if (power_up)
enable_suspend()

I suppose I was thinking that suspend happens on power key pressed, but it should happen on power key released.

Albert Astals Cid (aacid) wrote on 2015-01-22: Posted in a previous version of this proposal

Text conflict in qml/Shell.qml
1 conflicts encountered.

PS Jenkins bot (ps-jenkins) wrote on 2015-01-23: Posted in a previous version of this proposal

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity8-ci/5180/rebuild

review: Needs Fixing (continuous-integration)

kevin gunn (kgunn72) wrote on 2015-01-23: Posted in a previous version of this proposal

this didn't test out well with the relevant u-s-c change
see https://code.launchpad.net/~josharenson/unity-system-compositor/handle_screenshot_button/+merge/247085/comments/612027

kevin gunn (kgunn72) wrote on 2015-01-23: Posted in a previous version of this proposal

meant to set "needs fixing"

review: Needs Fixing

PS Jenkins bot (ps-jenkins) wrote on 2015-01-29: Posted in a previous version of this proposal

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity8-ci/5209/rebuild

review: Needs Fixing (continuous-integration)

kevin gunn (kgunn72) wrote on 2015-02-02: Posted in a previous version of this proposal

forgot to mark this as approved per testing

review: Approve (testing)

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

"""
8 + function onKeyPressed(key) {
119 + var eventAccepted = true;
120 +
121 + /* Determine what key was pressed */
122 + if (key == Qt.Key_PowerDown || key == Qt.Key_PowerOff) {
123 +
124 + // FIXME: We only consider power key presses if the screen is
125 + // on because of bugs 1410830/1409003. The theory is that when
126 + // those bugs are encountered, there is a >2s delay between the
127 + // power press event and the power release event, which causes
128 + // the shutdown dialog to appear on resume. So to avoid that
129 + // symptom while we investigate the root cause, we simply won't
130 + // initiate any dialogs when the screen is off.
131 + // This also prevents taking screenshots when the screen is off.
132 + if (Powerd.status === Powerd.On) {
133 + if (!powerKeyLongPressTimer.running) {
134 + powerKeyLongPressTimer.restart();
135 + }
136 + d.powerKeyPressed = true;
137 + }
138 + } else if (key == Qt.Key_MediaTogglePlayPause || key == Qt.Key_MediaPlay) {
139 + eventAccepted = callManager.handleMediaKey(false);
140 + } else if (key == Qt.Key_VolumeDown) {
141 + d.volumeDownKeyPressed = true;
142 + } else if (key == Qt.Key_VolumeUp) {
143 + d.volumeUpKeyPressed = true;
144 + } else {
145 + eventAccepted = false;
146 + }
"""

As I told before, volume keys should not be filtered out.

In this code, you're filtering them out (eventAccepted will be true for them).

You might consider renaming eventAccepted to something like filterOutEvent to make more explicit what the return value actually does.

review: Needs Fixing

PS Jenkins bot (ps-jenkins) wrote on 2015-02-05: Posted in a previous version of this proposal

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity8-ci/5242/rebuild

review: Needs Fixing (continuous-integration)

PS Jenkins bot (ps-jenkins) wrote on 2015-02-10: Posted in a previous version of this proposal

PASSED: Continuous integration, rev:1506
http://jenkins.qa.ubuntu.com/job/unity8-ci/5266/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-vivid-touch/1269
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-phablet-qmluitests-vivid/430
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-vivid-amd64-ci/431
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-vivid-i386-ci/431
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-runner-vivid-mako/1122
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/1267
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/1267/artifact/work/output/*zip*/output.zip
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/17925

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity8-ci/5266/rebuild

review: Approve (continuous-integration)

Daniel d'Andrada (dandrader) wrote on 2015-02-11: Posted in a previous version of this proposal

Having both aPowerKeyWasReleased and powerKeyPressed smelled fishy to me. So I had a go at making the code more declarative. It does seem to simplify and clarify PhysicalKeysMapper logic:

http://paste.ubuntu.com/10176638/

What do you think? It also has the benefit of making PhysicalKeysMapper interface more QMLish.
Eg: MouseArea has pressed property, not a pressed() signal.

PS Jenkins bot (ps-jenkins) wrote on 2015-02-12: Posted in a previous version of this proposal

PASSED: Continuous integration, rev:1508
http://jenkins.qa.ubuntu.com/job/unity8-ci/5314/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-vivid-touch/1343
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-phablet-qmluitests-vivid/478
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-vivid-amd64-ci/479
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-vivid-i386-ci/479
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-runner-vivid-mako/1183
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/1341
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/1341/artifact/work/output/*zip*/output.zip
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/18034

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity8-ci/5314/rebuild

review: Approve (continuous-integration)

Josh Arenson (josharenson) wrote on 2015-02-12: Posted in a previous version of this proposal

Addressed issues that were discussed on IRC as per #1507 and #1508

Daniel d'Andrada (dandrader) wrote on 2015-02-13: Posted in a previous version of this proposal

Looks good.

Non-blockers:

In test_LongPressPowerButton(), could you please check that powerKeyLongPress() only gets emited *after* the timeout?

like this:

"""
compare(onPowerKeyLongPressCount, 0);
wait(physicalKeysMapper.powerKeyLongPressTimeMs * 2);
compare(onPowerKeyLongPressCount, 1);
"""

review: Approve

Daniel d'Andrada (dandrader) on 2015-02-13: Posted in a previous version of this proposal

review: Approve (codewise)

Daniel d'Andrada (dandrader) wrote on 2015-02-13: Posted in a previous version of this proposal

* Did you perform an exploratory manual test run of the code change and any related functionality?
No, but kgunn did.

* Did CI run pass? If not, please explain why.
Yes! All green!

* Did you make sure that the branch does not contain spurious tags?
Yes.

Albert Astals Cid (aacid) wrote on 2015-02-16: Posted in a previous version of this proposal

Relaying Saviq's comment over mail

*****
Ugh, I just realized that we might have needlessly changed the screenshot shortcut and made
lives hard for ourselves.

We need not have involved u-s-c if we kept the screenshot combination to
be vol+-, and since we've switched to reacting onReleased instead of
onPressed, we could have done the same with volume buttons...
The other thing that came to mind is that we need to verify that
long-press behaviour is still fine. Albert just checked and long-press
is broken.

Josh, when you're around, could you have a look at going back to vol+-
as the screenshot shortcut, and fixing the above. Have a look at
.isAutoRepeat of a key event to enable that.
*****

review: Needs Fixing

lp:~unity-team/unity8/physical-key-mapper updated on 2015-02-16

1515. By Michał Sawicz on 2015-02-16: doc++
1516. By Michał Sawicz on 2015-02-16: doc--
1517. By Michał Sawicz on 2015-02-16: Accept power events regardless of power state, don't trigger screenshots if screen off
1518. By Michał Sawicz on 2015-02-16: Rename signals properly.

PS Jenkins bot (ps-jenkins) wrote on 2015-02-17:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity8-ci/5325/rebuild

review: Needs Fixing (continuous-integration)

PS Jenkins bot (ps-jenkins) wrote on 2015-02-17:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity8-ci/5326/rebuild

review: Needs Fixing (continuous-integration)

lp:~unity-team/unity8/physical-key-mapper updated on 2015-02-17

1519. By Michał Sawicz on 2015-02-16: Fix signal more

Albert Astals Cid (aacid) wrote on 2015-02-17:

* Did you perform an exploratory manual test run of the code change and any related functionality?
Yes

* Did CI run pass?
No, Qt 5.4 brought some failures, investigating

* Did you make sure that the branch does not contain spurious tags?
Yes

review: Approve

PS Jenkins bot (ps-jenkins) wrote on 2015-02-17:

FAILED: Continuous integration, rev:1519
http://jenkins.qa.ubuntu.com/job/unity8-ci/5327/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-vivid-touch/1385
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/unity-phablet-qmluitests-vivid/491
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-vivid-amd64-ci/492
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-vivid-i386-ci/492
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-runner-vivid-mako/1223
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/1383
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/1383/artifact/work/output/*zip*/output.zip
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/18105

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity8-ci/5327/rebuild

review: Needs Fixing (continuous-integration)

Michael Zanetti (mzanetti) wrote on 2015-02-17:

IMO it feels quite odd that we only change the volume onReleased now...

Josh Arenson (josharenson) wrote on 2015-02-19: Posted in a previous version of this proposal

Can you please clarify what actions should trigger a screenshot, and which actions should trigger a volume event?

This is the order that I would assume

1. PressDecrease + PressIncrease = ScreenShot
2. PressDecrease + Release Decrease = Volume Down
3. PressIncrease + Release Increase = Volume Up
4. PressAndHoldIncrease (exclusive) = VolumeUp @ KeyRepeat Interval
5. PressAndHoldDecrease (exclusive) = VolumeDown @ KeyRepeat Interval

I have an issue with 2 & 3 (not receiving instant feedback on a volume button press). Also, should there be a delay for step 1, or should the screenshot happen instantly?

Daniel d'Andrada (dandrader) wrote on 2015-02-19: Posted in a previous version of this proposal

Better ask it here:
https://code.launchpad.net/~unity-team/unity8/physical-key-mapper/+merge/249890

Daniel d'Andrada (dandrader) wrote on 2015-02-19: Posted in a previous version of this proposal

For what it's worth, my Android phone only reacts (ie, show the volume bar etc) when I release the volume key, not on press.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk

Subscribers

People subscribed via source and target branches

to all changes:

Mohammad Rakibul Hasan

Unity Team

 === modified file 'qml/Components/Dialogs.qml'
 --- qml/Components/Dialogs.qml	2014-12-08 13:39:01 +0000
 +++ qml/Components/Dialogs.qml	2015-02-17 08:14:50 +0000
@@ -35,21 +35,12 @@
+         }
+     }
--    function onPowerKeyPressed() {
--        // FIXME: event.isAutoRepeat is always false on Nexus 4.
--        // So we use powerKeyTimer.running to avoid the PowerOff key repeat
--        // https://launchpad.net/bugs/1349416
--        if (!powerKeyTimer.running) {
--            powerKeyTimer.restart();
--        }
--    }
--
--    function onPowerKeyReleased() {
--        powerKeyTimer.stop();
--    }
--
      signal powerOffClicked();
++    function showPowerDialog() {
++        d.showPowerDialog();
++    }
++
      QtObject {
          id: d // private stuff
          objectName: "dialogsPrivate"
@@ -68,17 +59,6 @@
          active: false
+     }
--    Timer {
--        id: powerKeyTimer
--        interval: 2000
--        repeat: false
--        triggeredOnStart: false
--
--        onTriggered: {
--            d.showPowerDialog();
--        }
--    }
--
      Component {
          id: logoutDialogComponent
          ShellDialog {
 === added file 'qml/Components/PhysicalKeysMapper.qml'
 --- qml/Components/PhysicalKeysMapper.qml	1970-01-01 00:00:00 +0000
 +++ qml/Components/PhysicalKeysMapper.qml	2015-02-17 08:14:50 +0000
@@ -0,0 +1,113 @@
++/*
++ * Copyright (C) 2014-2015 Canonical, Ltd.
++ *
++ * This program is free software; you can redistribute it and/or modify
++ * it under the terms of the GNU General Public License as published by
++ * the Free Software Foundation; version 3.
++ *
++ * This program is distributed in the hope that it will be useful,
++ * but WITHOUT ANY WARRANTY; without even the implied warranty of
++ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
++ * GNU General Public License for more details.
++ *
++ * You should have received a copy of the GNU General Public License
++ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
++ */
++
++import QtQuick 2.0
++import Powerd 0.1
++
++/*!
++ \brief A mapper for the physical keys on the device
++
++ A mapper to handle events triggered by pressing physical keys on a device.
++ Keys included are
++    * Volume Decrease
++    * Volume Increase
++    * Power
++
++ This allows for handling the following events
++    * Power dialog
++    * Volume Decreases/Increases
++    * Screenshots
++
++*/
++
++Item {
++    id: root
++
++    signal powerKeyLongPressed;
++    signal volumeDownTriggered;
++    signal volumeUpTriggered;
++    signal screenshotTriggered;
++
++    QtObject {
++        id: d
++
++        property bool volumeDownKeyPressed: false
++        property bool volumeUpKeyPressed: false
++        property bool ignoreVolumeEvents: false
++    }
++
++    Timer {
++        id: powerKeyLongPressTimer
++
++        interval: 2000
++        onTriggered: root.powerKeyLongPressed();
++    }
++
++
++    function onKeyPressed(event) {
++        if ((event.key == Qt.Key_PowerDown || event.key == Qt.Key_PowerOff)
++            && !event.isAutoRepeat) {
++
++            // FIXME: We only consider power key presses if the screen is
++            // on because of bugs 1410830/1409003.  The theory is that when
++            // those bugs are encountered, there is a >2s delay between the
++            // power press event and the power release event, which causes
++            // the shutdown dialog to appear on resume.  So to avoid that
++            // symptom while we investigate the root cause, we simply won't
++            // initiate any dialogs when the screen is off.
++            if (Powerd.status === Powerd.On) {
++                powerKeyLongPressTimer.restart();
++            }
++            event.accepted = true;
++        } else if ((event.key == Qt.Key_MediaTogglePlayPause || event.key == Qt.Key_MediaPlay)
++                   && !event.isAutoRepeat) {
++            event.accepted = callManager.handleMediaKey(false);
++        } else if (event.key == Qt.Key_VolumeDown) {
++            if (event.isAutoRepeat && !d.ignoreVolumeEvents) root.volumeDownTriggered();
++            else if (!event.isAutoRepeat) {
++                if (d.volumeUpKeyPressed) {
++                    if (Powerd.status === Powerd.On) root.screenshotTriggered();
++                    d.ignoreVolumeEvents = true;
++                }
++                d.volumeDownKeyPressed = true;
++            }
++        } else if (event.key == Qt.Key_VolumeUp) {
++            if (event.isAutoRepeat && !d.ignoreVolumeEvents) root.volumeUpTriggered();
++            else if (!event.isAutoRepeat) {
++                if (d.volumeDownKeyPressed) {
++                    if (Powerd.status === Powerd.On) root.screenshotTriggered();
++                    d.ignoreVolumeEvents = true;
++                }
++                d.volumeUpKeyPressed = true;
++            }
++        }
++    }
++
++    function onKeyReleased(event) {
++        if (event.key == Qt.Key_PowerDown || event.key == Qt.Key_PowerOff) {
++            powerKeyLongPressTimer.stop();
++            event.accepted = true;
++        } else if (event.key == Qt.Key_VolumeDown) {
++            if (!d.ignoreVolumeEvents) root.volumeDownTriggered();
++            d.volumeDownKeyPressed = false;
++            if (!d.volumeUpKeyPressed) d.ignoreVolumeEvents = false;
++        } else if (event.key == Qt.Key_VolumeUp) {
++            if (!d.ignoreVolumeEvents) root.volumeUpTriggered();
++            d.volumeUpKeyPressed = false;
++            if (!d.volumeDownKeyPressed) d.ignoreVolumeEvents = false;
++        }
++    }
++}
 === removed file 'qml/Components/VolumeKeyFilter.qml'
 --- qml/Components/VolumeKeyFilter.qml	2014-10-30 02:39:38 +0000
 +++ qml/Components/VolumeKeyFilter.qml	1970-01-01 00:00:00 +0000
@@ -1,63 +0,0 @@
--/*
-- * Copyright (C) 2014 Canonical, Ltd.
-- *
-- * This program is free software; you can redistribute it and/or modify
-- * it under the terms of the GNU General Public License as published by
-- * the Free Software Foundation; version 3.
-- *
-- * This program is distributed in the hope that it will be useful,
-- * but WITHOUT ANY WARRANTY; without even the implied warranty of
-- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
-- * GNU General Public License for more details.
-- *
-- * You should have received a copy of the GNU General Public License
-- * along with this program.  If not, see <http://www.gnu.org/licenses/>.
-- */
--
--import QtQuick 2.0
--/*!
-- \brief A filter for volume keys
--
--A filter which treats volume keys as single tri-state key with the states:
--VolumeUp Pressed, VolumeDown Pressed or Volume Up+Down pressed
--*/
--QtObject {
--    id: root
--
--    signal volumeUpPressed()
--    signal volumeDownPressed()
--    signal bothVolumeKeysPressed()
--
--    property bool volumeUpKeyPressed: false
--    property bool volumeDownKeyPressed: false
--    property bool aVolumeKeyWasReleased: true
--
--    function onKeyPressed(key) {
--        if (key == Qt.Key_VolumeUp)
--            volumeUpKeyPressed = true;
--        else if (key == Qt.Key_VolumeDown)
--            volumeDownKeyPressed = true;
--
--        if (volumeDownKeyPressed && volumeUpKeyPressed) {
--            //avoids sending a signal repeatedly if both keys are held
--            //instead one of the keys must have been previously released
--            if (aVolumeKeyWasReleased)
--                bothVolumeKeysPressed();
--            aVolumeKeyWasReleased = false;
--        } else if (volumeDownKeyPressed) {
--            volumeDownPressed();
--        } else if (volumeUpKeyPressed) {
--            volumeUpPressed();
--        }
--    }
--
--    function onKeyReleased(key) {
--        if (key == Qt.Key_VolumeUp) {
--            volumeUpKeyPressed = false;
--            aVolumeKeyWasReleased = true;
--        } else if (key == Qt.Key_VolumeDown) {
--            volumeDownKeyPressed = false;
--            aVolumeKeyWasReleased = true;
--        }
--    }
--}
 === modified file 'qml/Shell.qml'
 --- qml/Shell.qml	2015-02-11 17:12:49 +0000
 +++ qml/Shell.qml	2015-02-17 08:14:50 +0000
@@ -160,6 +160,15 @@
          objectName: "dashCommunicator"
+     }
++    PhysicalKeysMapper {
++        id: physicalKeysMapper
++
++        onPowerKeyLongPressed: dialogs.showPowerDialog()
++        onVolumeDownTriggered: volumeControl.volumeDown();
++        onVolumeUpTriggered: volumeControl.volumeUp();
++        onScreenshotTriggered: screenGrabber.capture();
++    }
++
      ScreenGrabber {
          id: screenGrabber
          z: dialogs.z + 10
@@ -172,45 +181,9 @@
          value: launcher.shown || launcher.dashSwipe
+     }
--    VolumeKeyFilter {
--        id: volumeKeyFilter
--        onVolumeDownPressed: volumeControl.volumeDown()
--        onVolumeUpPressed: volumeControl.volumeUp()
--        onBothVolumeKeysPressed: screenGrabber.capture()
--    }
--
      WindowKeysFilter {
--        Keys.onPressed: {
--            // Nokia earpieces give TogglePlayPause, while the iPhone's earpiece gives Play
--            if (event.key == Qt.Key_MediaTogglePlayPause || event.key == Qt.Key_MediaPlay) {
--                event.accepted = callManager.handleMediaKey(false);
--            } else if (event.key == Qt.Key_PowerOff || event.key == Qt.Key_PowerDown) {
--                // FIXME: We only consider power key presses if the screen is
--                // on because of bugs 1410830/1409003.  The theory is that when
--                // those bugs are encountered, there is a >2s delay between the
--                // power press event and the power release event, which causes
--                // the shutdown dialog to appear on resume.  So to avoid that
--                // symptom while we investigate the root cause, we simply won't
--                // initiate any dialogs when the screen is off.
--                if (Powerd.status === Powerd.On) {
--                    dialogs.onPowerKeyPressed();
--                }
--                event.accepted = true;
--            } else {
--                volumeKeyFilter.onKeyPressed(event.key);
--                event.accepted = false;
--            }
--        }
--
--        Keys.onReleased: {
--            if (event.key == Qt.Key_PowerOff || event.key == Qt.Key_PowerDown) {
--                dialogs.onPowerKeyReleased();
--                event.accepted = true;
--            } else {
--                volumeKeyFilter.onKeyReleased(event.key);
--                event.accepted = false;
--            }
--        }
++        Keys.onPressed: physicalKeysMapper.onKeyPressed(event);
++        Keys.onReleased: physicalKeysMapper.onKeyReleased(event);
+     }
      Item {
 === modified file 'tests/qmltests/CMakeLists.txt'
 --- tests/qmltests/CMakeLists.txt	2015-02-11 17:11:41 +0000
 +++ tests/qmltests/CMakeLists.txt	2015-02-17 08:14:50 +0000
@@ -13,6 +13,7 @@
  set(qmltest_DEFAULT_NO_ADD_TEST FALSE)
  set(qmltest_DEFAULT_PROPERTIES ENVIRONMENT "QT_QPA_PLATFORM=minimal")
  add_qml_test(utils/Unity/Test UnityTest)
++add_qml_test(Components PhysicalKeysMapper)
  set(qmltest_DEFAULT_TARGETS qmluitests)
  set(qmltest_DEFAULT_NO_ADD_TEST TRUE)
 === added file 'tests/qmltests/Components/tst_PhysicalKeysMapper.qml'
 --- tests/qmltests/Components/tst_PhysicalKeysMapper.qml	1970-01-01 00:00:00 +0000
 +++ tests/qmltests/Components/tst_PhysicalKeysMapper.qml	2015-02-17 08:14:50 +0000
@@ -0,0 +1,119 @@
++/*
++ * Copyright (C) 2014-2015 Canonical, Ltd.
++ *
++ * This program is free software; you can redistribute it and/or modify
++ * it under the terms of the GNU General Public License as published by
++ * the Free Software Foundation; version 3.
++ *
++ * This program is distributed in the hope that it will be useful,
++ * but WITHOUT ANY WARRANTY; without even the implied warranty of
++ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
++ * GNU General Public License for more details.
++ *
++ * You should have received a copy of the GNU General Public License
++ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
++ */
++
++import QtQuick 2.0
++import QtTest 1.0
++import "../../../qml/Components"
++
++TestCase {
++    name: "PhysicalKeysMapper"
++
++    Loader {
++        // Using a Loader here to make sure mapper state is coherent
++        // regardless of unmatched KeyPress and KeyRelease events
++        id: loader
++        active: false
++        sourceComponent: PhysicalKeysMapper { }
++    }
++
++    SignalSpy {
++        id: powerSpy
++        target: loader.item
++        signalName: "powerKeyLongPressed"
++    }
++
++    SignalSpy {
++        id: volumeDownSpy
++        target: loader.item
++        signalName: "volumeDownTriggered"
++    }
++
++    SignalSpy {
++        id: volumeUpSpy
++        target: loader.item
++        signalName: "volumeUpTriggered"
++    }
++
++    SignalSpy {
++        id: screenshotSpy
++        target: loader.item
++        signalName: "screenshotTriggered"
++    }
++
++    function init() {
++        loader.active = true;
++        tryCompare(loader.status == Loader.Ready);
++    }
++
++    function cleanup() {
++        loader.active = false;
++        powerSpy.clear();
++        volumeDownSpy.clear();
++        volumeUpSpy.clear();
++        screenshotSpy.clear();
++    }
++
++    function test_LongPressPowerButton() {
++        loader.item.onKeyPressed({ key: Qt.Key_PowerDown });
++
++        expectFailContinue("", "Power signal should not be emitted within a second");
++        powerSpy.wait(1000);
++        powerSpy.clear();
++
++        loader.item.onKeyPressed({ key: Qt.Key_PowerDown, isAutoRepeat: true });
++        powerSpy.wait(1500);
++    }
++
++    function test_ScreenshotButtons_data() {
++        return [
++            { tag: "UpFirst", first: Qt.Key_VolumeUp, second: Qt.Key_VolumeDown },
++            { tag: "DownFirst", first: Qt.Key_VolumeDown, second: Qt.Key_VolumeUp },
++        ];
++    }
++
++    function test_ScreenshotButtons(data) {
++        loader.item.onKeyPressed({ key: data.first });
++        loader.item.onKeyPressed({ key: data.second });
++        screenshotSpy.wait();
++        loader.item.onKeyReleased({ key: data.first });
++        loader.item.onKeyReleased({ key: data.second });
++        expectFailContinue("", "VolumeUp signal should not fire");
++        volumeUpSpy.wait(100);
++        expectFailContinue("", "VolumeDown signal should not fire");
++        volumeDownSpy.wait(100);
++    }
++
++    function test_VolumeButton_data() {
++        return [
++            { tag: "Down", key: Qt.Key_VolumeDown, spy: volumeDownSpy },
++            { tag: "Up", key: Qt.Key_VolumeUp, spy: volumeUpSpy },
++        ];
++    }
++
++    function test_VolumeButton(data) {
++        loader.item.onKeyPressed({ key: data.key });
++        expectFailContinue("", "Signal should not fire on press");
++        data.spy.wait(100);
++        data.spy.clear();
++
++        loader.item.onKeyReleased({ key: data.key });
++        data.spy.wait();
++
++        loader.item.onKeyPressed({ key: data.key });
++        loader.item.onKeyPressed({ key: data.key, isAutoRepeat: true });
++        data.spy.wait();
++    }
++}