Merge lp:~josharenson/unity8/physical_keys_filter into lp:unity8
| Status: | Superseded |
|---|---|
| Proposed branch: | lp:~josharenson/unity8/physical_keys_filter |
| Merge into: | lp:unity8 |
| Diff against target: |
404 lines (+216/-120) 6 files modified
qml/Components/Dialogs.qml (+4/-24) qml/Components/PhysicalKeysMapper.qml (+114/-0) qml/Components/VolumeKeyFilter.qml (+0/-63) qml/Shell.qml (+12/-33) tests/qmltests/CMakeLists.txt (+1/-0) tests/qmltests/Components/tst_PhysicalKeysMapper.qml (+85/-0) |
| To merge this branch: | bzr merge lp:~josharenson/unity8/physical_keys_filter |
| Related bugs: |
| Reviewer | Review Type | Date Requested | Status |
|---|---|---|---|
| Albert Astals Cid (community) | 2014-12-10 | Needs Fixing on 2015-02-16 | |
| Daniel d'Andrada (community) | codewise | Approve on 2015-02-13 | |
| PS Jenkins bot | continuous-integration | Approve on 2015-02-12 | |
| kevin gunn (community) | testing | Approve on 2015-02-02 | |
| Michał Sawicz | ux | Needs Fixing on 2015-01-13 | |
|
Review via email:
|
|||
This proposal has been superseded by a proposal from 2015-02-16.
Commit Message
Create a PhysicalKeyMapper to handle all physical/hardware keys
In addition, change the key combination for taking a screenshot from (Volume Up + Volume Down) to (Power + Volume Down).
Description of the Change
* Are there any related MPs required for this MP to build/function as expected? Please list.
Yes,
https:/
* 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 : | # |
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.
| Daniel d'Andrada (dandrader) wrote : | # |
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 : | # |
Please follow the MP submission checklist regarding the commit message:
https:/
| Daniel d'Andrada (dandrader) wrote : | # |
> Please follow the MP submission checklist regarding the commit message:
> https:/
Also the "--fixes lp:1390393" line is not necessary as CI will automatically add it when merging the branch.
| Daniel d'Andrada (dandrader) wrote : | # |
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.
| Daniel d'Andrada (dandrader) wrote : | # |
> 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 : | # |
> 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 : | # |
FAILED: Continuous integration, rev:1492
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
None: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
| Daniel d'Andrada (dandrader) wrote : | # |
In qml/Shell.qml
"""
Dialogs {
...
Connections {
target: physicalKeysMapper
}
...
}
"""
Why not put the onPowerKeyLongPress line directly into PhysicalKeysMapper, like it's done for all other connections?
-----------------
In qml/Components/
"""
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/
"""
property bool eventAccepted: false
...
function onKeyPressed(key) {
...
...
...
...
return eventAccepted;
}
function onKeyReleased(key) {
...
...
...
...
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;
...
...
...
...
return eventAccepted;
}
"""
-------
In qml/Components/
"""
} else if (key == Qt.Key_VolumeDown) {
} else if (key == Qt.Key_VolumeUp) {
}
"""
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/
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/
"""
property bool aPowerKeyWasRel
property bool powerKeyPressed: false
property bool aVolumeKeyWasRe
property bool volumeDownKeyPr
property bool volumeUpKeyPressed: false
"""
Could you please wrap those properties in a "property QtObject d: QtObject { id: d }" as they're private?
| Daniel d'Andrada (dandrader) wrote : | # |
In qml/Components/
"""
signal bothVolumeKeysP
"""
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:/
| Daniel d'Andrada (dandrader) wrote : | # |
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 : | # |
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://
| Daniel d'Andrada (dandrader) wrote : | # |
The convention we've been using for test names is test_fooBar, not test_foo_bar.
| Josh Arenson (josharenson) wrote : | # |
> In qml/Components/
>
> """
> signal bothVolumeKeysP
> """
>
> 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:/
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 : | # |
FAILED: Continuous integration, rev:1497
http://
Executed test runs:
UNSTABLE: http://
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
UNSTABLE: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
| Daniel d'Andrada (dandrader) wrote : | # |
PhysicalKeysMapper is still among the tests that require a GUI. The position where you put it in tests/qmltests/
"""
add_qml_
"""
As it will get covered by this line:
"""
set(qmltest_
"""
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:/
| Albert Astals Cid (aacid) wrote : | # |
Text conflict in qml/Components/
1 conflicts encountered.
| PS Jenkins bot (ps-jenkins) wrote : | # |
PASSED: Continuous integration, rev:1501
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
| Daniel d'Andrada (dandrader) wrote : | # |
Approving as the last couple of issues have been addressed.
| Michał Sawicz (saviq) wrote : | # |
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-
| Josh Arenson (josharenson) wrote : | # |
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 : | # |
Sure there is, USC needs to suppress suspending the phone if voldown is pressed while power is pressed.
| Michał Sawicz (saviq) wrote : | # |
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 : | # |
> 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_
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 : | # |
Text conflict in qml/Shell.qml
1 conflicts encountered.
| PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:1504
http://
Executed test runs:
UNSTABLE: http://
UNSTABLE: http://
SUCCESS: http://
SUCCESS: http://
UNSTABLE: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
| kevin gunn (kgunn72) wrote : | # |
this didn't test out well with the relevant u-s-c change
see https:/
| PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:1505
http://
Executed test runs:
UNSTABLE: http://
UNSTABLE: http://
SUCCESS: http://
SUCCESS: http://
UNSTABLE: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
| kevin gunn (kgunn72) wrote : | # |
forgot to mark this as approved per testing
| Daniel d'Andrada (dandrader) wrote : | # |
"""
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 (!powerKeyLongP
134 + powerKeyLongPre
135 + }
136 + d.powerKeyPressed = true;
137 + }
138 + } else if (key == Qt.Key_
139 + eventAccepted = callManager.
140 + } else if (key == Qt.Key_VolumeDown) {
141 + d.volumeDownKey
142 + } else if (key == Qt.Key_VolumeUp) {
143 + d.volumeUpKeyPr
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.
| PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:1506
http://
Executed test runs:
UNSTABLE: http://
UNSTABLE: http://
SUCCESS: http://
SUCCESS: http://
UNSTABLE: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
| PS Jenkins bot (ps-jenkins) wrote : | # |
PASSED: Continuous integration, rev:1506
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
| Daniel d'Andrada (dandrader) wrote : | # |
Having both aPowerKeyWasRel
http://
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 : | # |
PASSED: Continuous integration, rev:1508
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
| Josh Arenson (josharenson) wrote : | # |
Addressed issues that were discussed on IRC as per #1507 and #1508
| Daniel d'Andrada (dandrader) wrote : | # |
Looks good.
Non-blockers:
In test_LongPressP
like this:
"""
compare(
wait(physicalKe
compare(
"""
| Daniel d'Andrada (dandrader) wrote : | # |
* 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 : | # |
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.
*****
| Josh Arenson (josharenson) wrote : | # |
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. PressAndHoldInc
5. PressAndHoldDec
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 : | # |
Better ask it here:
https:/
| Daniel d'Andrada (dandrader) wrote : | # |
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.
The else ifs need to be written in our style
} else if (volumeDownKeyP
instead of
}
else if (volumeDownKeyP