Merge lp://qastaging/~mzanetti/unity8/surfacetitles-in-quicklist into lp://qastaging/unity8

Proposed by Michael Zanetti
Status: Merged
Approved by: Lukáš Tinkl
Approved revision: 2848
Merged at revision: 2879
Proposed branch: lp://qastaging/~mzanetti/unity8/surfacetitles-in-quicklist
Merge into: lp://qastaging/unity8
Prerequisite: lp://qastaging/~mzanetti/unity8/stripped-down-quicklist
Diff against target: 472 lines (+183/-31)
14 files modified
CMakeLists.txt (+1/-1)
debian/control (+1/-1)
plugins/Unity/Launcher/CMakeLists.txt (+2/-0)
plugins/Unity/Launcher/launcheritem.cpp (+27/-6)
plugins/Unity/Launcher/launcheritem.h (+3/-2)
plugins/Unity/Launcher/launchermodel.cpp (+47/-14)
plugins/Unity/Launcher/launchermodel.h (+3/-1)
plugins/Unity/Launcher/quicklistmodel.cpp (+7/-0)
plugins/Unity/Launcher/quicklistmodel.h (+1/-0)
qml/Launcher/LauncherPanel.qml (+1/-0)
tests/mocks/Unity/Application/MirSurface.cpp (+4/-0)
tests/mocks/Unity/Application/MirSurface.h (+1/-0)
tests/plugins/Unity/Launcher/CMakeLists.txt (+3/-0)
tests/plugins/Unity/Launcher/launchermodeltest.cpp (+82/-6)
To merge this branch: bzr merge lp://qastaging/~mzanetti/unity8/surfacetitles-in-quicklist
Reviewer Review Type Date Requested Status
Unity8 CI Bot continuous-integration Approve
Lukáš Tinkl (community) Approve
Review via email: mp+318771@code.qastaging.launchpad.net

Commit message

add surface titles to launcher's quicklist

Description of the change

Prereq-archive: ppa:ci-train-ppa-service/2514

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

see prereq +
https://code.launchpad.net/~mzanetti/qtmir/rebuild-for-mirsurface-appid/+merge/319951
https://code.launchpad.net/~mzanetti/unity-api/mirsurface-appid/+merge/319949

 * Did you perform an exploratory manual test run of your code change and any related functionality?
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?
n/a

To post a comment you must log in.
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 :

I think you could "typedef" QList<QPair<QString, QString>> so that you don't have to repeat it several times throughout the code

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

+ } else if (actionId.startsWith("surface_")){

Could use a QStringLiteral()

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

+ surfaces.append(qMakePair<QString, QString>(iface->persistentId(), iface->name()));

Imho you could just use the initializer_list CTOR here, so:

+ surfaces.append({iface->persistentId(), iface->name()});

review: Needs Information
2841. By Michael Zanetti

merge prereq

2842. By Michael Zanetti

use QStringLiteral for string literals

2843. By Michael Zanetti

shorten creation of QPair

Revision history for this message
Michael Zanetti (mzanetti) wrote :

fixed the last 2. Yes, could typedef the first one but I personally am not a fan of typedefs... makes the code more opaque IMO as the reader has to look up what the type actually is...

2844. By Michael Zanetti

merge prereq

Revision history for this message
Unity8 CI Bot (unity8-ci-bot) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
Unity8 CI Bot (unity8-ci-bot) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
Daniel d'Andrada (dandrader) wrote :

> fixed the last 2. Yes, could typedef the first one but I personally am not a
> fan of typedefs... makes the code more opaque IMO as the reader has to look up
> what the type actually is...

+1

They're not always helpful. Telling from experience from reading Mir and MirAL code.

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

If the surface title is too long, it just overflows the popup menu and doesn't do any kind of ellision; also there's no right margin

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

The surface title in the quicklist menu doesn't observe/respect the window title changes. For example in webbrowser, if you navigate to a new link or open a new tab, the window title changes but the launcher popup title stays the same: fixed to the string it had at startup.

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

Some windows (unfortunately) do not have surface titles and then you get an empty title in the quicklist (see e.g. System Settings -> Launcher). Imo you should fall back to app title in that case (or whatever the window is displaying in the caption)

review: Needs Fixing
2845. By Michael Zanetti

merge prereq

Revision history for this message
Unity8 CI Bot (unity8-ci-bot) wrote :
review: Needs Fixing (continuous-integration)
2846. By Michael Zanetti

fix issues from review

Revision history for this message
Michael Zanetti (mzanetti) wrote :

fixed the all the reported issues

Revision history for this message
Unity8 CI Bot (unity8-ci-bot) wrote :
review: Needs Fixing (continuous-integration)
2847. By Michael Zanetti

merge prereq

Revision history for this message
Unity8 CI Bot (unity8-ci-bot) wrote :
review: Needs Fixing (continuous-integration)
2848. By Michael Zanetti

make mocks build

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

FAILED: Continuous integration, rev:2848
https://unity8-jenkins.ubuntu.com/job/lp-unity8-ci/3414/
Executed test runs:
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build/4501
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/test-0-autopkgtest/label=amd64,release=xenial+overlay,testname=qmluitests.sh/2705
    UNSTABLE: https://unity8-jenkins.ubuntu.com/job/test-0-autopkgtest/label=amd64,release=zesty,testname=qmluitests.sh/2705
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-0-fetch/4529
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=xenial+overlay/4356
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=xenial+overlay/4356/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=zesty/4356
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=zesty/4356/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=xenial+overlay/4356
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=xenial+overlay/4356/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=zesty/4356
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=zesty/4356/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=xenial+overlay/4356
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=xenial+overlay/4356/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=zesty/4356
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=zesty/4356/artifact/output/*zip*/output.zip

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

review: Needs Fixing (continuous-integration)
Revision history for this message
Lukáš Tinkl (lukas-kde) wrote :

Yeah, works fine now

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

Yes

* Did CI run pass? If not, please explain why.

Unrelated test failure, waiting for a rebuild

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

PASSED: Continuous integration, rev:2848
https://unity8-jenkins.ubuntu.com/job/lp-unity8-ci/3424/
Executed test runs:
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build/4516
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/test-0-autopkgtest/label=amd64,release=xenial+overlay,testname=qmluitests.sh/2714
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/test-0-autopkgtest/label=amd64,release=zesty,testname=qmluitests.sh/2714
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-0-fetch/4544
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=xenial+overlay/4371
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=xenial+overlay/4371/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=zesty/4371
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=zesty/4371/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=xenial+overlay/4371
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=xenial+overlay/4371/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=zesty/4371
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=zesty/4371/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=xenial+overlay/4371
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=xenial+overlay/4371/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=zesty/4371
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=zesty/4371/artifact/output/*zip*/output.zip

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

review: Approve (continuous-integration)

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