Merge lp://staging/~unity-team/unity8/keymapSwitching into lp://staging/unity8

Proposed by Michał Sawicz
Status: Merged
Approved by: Michael Terry
Approved revision: 2281
Merged at revision: 2291
Proposed branch: lp://staging/~unity-team/unity8/keymapSwitching
Merge into: lp://staging/unity8
Prerequisite: lp://staging/~unity-team/unity8/shell_chrome
Diff against target: 634 lines (+265/-10)
20 files modified
plugins/AccountsService/AccountsService.cpp (+26/-0)
plugins/AccountsService/AccountsService.h (+6/-0)
qml/Shell.qml (+50/-0)
qml/Stages/AbstractStage.qml (+1/-0)
qml/Stages/ApplicationWindow.qml (+4/-0)
qml/Stages/DesktopStage.qml (+4/-0)
qml/Stages/PhoneStage.qml (+2/-0)
qml/Stages/SpreadDelegate.qml (+1/-0)
qml/Stages/SurfaceContainer.qml (+13/-0)
qml/Stages/TabletStage.qml (+2/-0)
tests/mocks/AccountsService/AccountsService.cpp (+17/-0)
tests/mocks/AccountsService/AccountsService.h (+8/-0)
tests/mocks/QMenuModel/QDBusActionGroup.qml (+5/-5)
tests/mocks/Unity/Application/MirSurface.cpp (+18/-2)
tests/mocks/Unity/Application/MirSurface.h (+5/-0)
tests/plugins/AccountsService/PropertiesServer.cpp (+9/-0)
tests/plugins/AccountsService/PropertiesServer.h (+0/-1)
tests/plugins/AccountsService/client.cpp (+30/-0)
tests/qmltests/Stages/tst_DesktopStage.qml (+0/-2)
tests/qmltests/tst_Shell.qml (+64/-0)
To merge this branch: bzr merge lp://staging/~unity-team/unity8/keymapSwitching
Reviewer Review Type Date Requested Status
Daniel d'Andrada (community) Needs Information
Michael Terry Approve
Unity8 CI Bot continuous-integration Approve
Review via email: mp+288842@code.staging.launchpad.net

This proposal supersedes a proposal from 2016-03-11.

Commit message

Keymap switching support

Description of the change

 * Are there any related MPs required for this MP to build/function as expected? Please list.
    lp:~unity-team/unity-api/kbdLayout
    lp:~unity-team/qtmir/kbdLayout
    lp:~lukas-kde/qtubuntu/kbdLayout

 * Did you perform an exploratory manual test run of your code change and any related functionality?
Will in silo
 * Did you make sure that your branch does not contain spurious tags?
Y
 * If you changed the packaging (debian), did you subscribe the ubuntu-unity team to this MP?
Y
 * If you changed the UI, has there been a design review?
N/A

To post a comment you must log in.
Revision history for this message
Unity8 CI Bot (unity8-ci-bot) wrote : Posted in a previous version of this proposal
review: Needs Fixing (continuous-integration)
Revision history for this message
Michael Terry (mterry) wrote : Posted in a previous version of this proposal

Some inline comments. Haven't reviewed test code yet. Am still setting up my phone to test these branches there.

review: Needs Fixing
Revision history for this message
Unity8 CI Bot (unity8-ci-bot) wrote : Posted in a previous version of this proposal
review: Needs Fixing (continuous-integration)
Revision history for this message
Unity8 CI Bot (unity8-ci-bot) wrote : Posted in a previous version of this proposal
review: Needs Fixing (continuous-integration)
Revision history for this message
Unity8 CI Bot (unity8-ci-bot) wrote :

FAILED: Continuous integration, rev:2278
https://unity8-jenkins.ubuntu.com/job/lp-unity8-ci/695/
Executed test runs:
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-0-fetch/914
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/931
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial/931
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=vivid+overlay/929
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=vivid+overlay/929/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=xenial/929
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=xenial/929/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=vivid+overlay/929
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=vivid+overlay/929/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=xenial/929
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=xenial/929/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=vivid+overlay/929
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=vivid+overlay/929/artifact/output/*zip*/output.zip
    FAILURE: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=xenial/929/console

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

review: Needs Fixing (continuous-integration)
2279. By Lukáš Tinkl

refine test for switching keymaps, move to tst_Shell

Revision history for this message
Unity8 CI Bot (unity8-ci-bot) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
Michael Terry (mterry) wrote :

As mentioned in telegram, I think we can drop activeKeymap. And the qmltest should test going past the front / end of keymap list. But besides that, it looks fine. Haven't tested yet, waiting for silo to rebuild.

2280. By Lukáš Tinkl

remove activeKeymapIndex, refine tests

2281. By Lukáš Tinkl

remove debug

Revision history for this message
Unity8 CI Bot (unity8-ci-bot) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
Lukáš Tinkl (lukas-kde) wrote :

> As mentioned in telegram, I think we can drop activeKeymap. And the qmltest
> should test going past the front / end of keymap list. But besides that, it
> looks fine. Haven't tested yet, waiting for silo to rebuild.

Fixed both issues

Revision history for this message
Unity8 CI Bot (unity8-ci-bot) wrote :

PASSED: Continuous integration, rev:2281
https://unity8-jenkins.ubuntu.com/job/lp-unity8-ci/738/
Executed test runs:
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/test-0-autopkgtest/label=amd64,release=vivid+overlay,testname=qmluitests.sh/414
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/test-0-autopkgtest/label=amd64,release=xenial,testname=qmluitests.sh/414
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/test-0-autopkgtest/label=phone-armhf,release=vivid+overlay,testname=autopilot.sh/414
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-0-fetch/967
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/984
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial/984
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=vivid+overlay/982
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=vivid+overlay/982/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=xenial/982
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=xenial/982/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=vivid+overlay/982
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=vivid+overlay/982/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=xenial/982
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=xenial/982/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=vivid+overlay/982
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=vivid+overlay/982/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=xenial/982
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=xenial/982/artifact/output/*zip*/output.zip

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

review: Approve (continuous-integration)
Revision history for this message
Michael Terry (mterry) wrote :

Note to self:

sudo gdbus call --system --dest org.freedesktop.Accounts --object-path /org/freedesktop/Accounts/User32011 --method org.freedesktop.Accounts.User.SetInputSources '[{"xkb": "us"}, {"xkb": "zh"}]'

Revision history for this message
Michael Terry (mterry) wrote :

OK, worked fine in my testing. Modulo a crasher if you give invalid input. (bug 1557634) But that's not a blocker for now.

Thanks!

review: Approve
Revision history for this message
Daniel d'Andrada (dandrader) wrote :

"""
    function switchToKeymap(keymap) {
        var finalKeymap = keymap.split("+");
        savedKeymap = keymap; // save the keymap in case the surface changes later

        if (surface) {
            surface.setKeymap(finalKeymap[0], finalKeymap[1] || "");
        }
    }
"""

I know I'm late to the party, but as I'm rebasing surface-based WM on top of silo 041 I couldn't help frowning when I saw that.

This kind of imperative work doesn't belong to QML. Can't the surface have a property that takes the full string and does any and all parsing needed behind the scenes?

Something like that:

"""
SurfaceContainer {
    id: root
    property string keymap
    Binding {
        target: surface
        property: "keymap"
        value: root.keymap
    }
}
"""

review: Needs Information
Revision history for this message
Daniel d'Andrada (dandrader) wrote :

And as a continuation of the idea in my previous comment, in ApplicationWindow:

"""
function switchToKeymap(keymap) {
        sessionContainer.surfaceContainer.switchToKeymap(keymap);
    }
"""

Would be something like:

"""
property alias keymap: sessionContainer.surfaceContainer.keymap
"""

Revision history for this message
Lukáš Tinkl (lukas-kde) wrote :

Yeah, thanks for the comments, I'm totally aware this will need some rework with surface based WM (this and the switching based on the currently focused _app_, not surface).

Revision history for this message
Daniel d'Andrada (dandrader) wrote :

My comments are not related to surface-based versus application-based WM. They are about imperative versus declarative code.

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