Merge lp://staging/~dandrader/unity8/allowClientResize into lp://staging/unity8

Proposed by Daniel d'Andrada
Status: Merged
Approved by: Lukáš Tinkl
Approved revision: 2869
Merged at revision: 2880
Proposed branch: lp://staging/~dandrader/unity8/allowClientResize
Merge into: lp://staging/unity8
Prerequisite: lp://staging/~mzanetti/unity8/surfacetitles-in-quicklist
Diff against target: 190 lines (+56/-5)
6 files modified
debian/control (+2/-2)
plugins/WindowManager/Window.cpp (+26/-1)
plugins/WindowManager/Window.h (+14/-1)
qml/Stage/Stage.qml (+8/-0)
tests/mocks/Unity/Application/MirSurface.h (+3/-0)
tests/plugins/Unity/Launcher/launchermodeltest.cpp (+3/-1)
To merge this branch: bzr merge lp://staging/~dandrader/unity8/allowClientResize
Reviewer Review Type Date Requested Status
Unity8 CI Bot continuous-integration Approve
Gerry Boland (community) Approve
Lukáš Tinkl Pending
Review via email: mp+319812@code.staging.launchpad.net

This proposal supersedes a proposal from 2017-03-14.

Commit message

Don't let clients resize their surfaces while in staged (phone/tablet) mode

Description of the change

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

That doesn't fix the bug for gtk+ apps though. I think gtk-mir needs some work.

* Are there any related MPs required for this MP to build/function as expected? Please list.
https://code.launchpad.net/~dandrader/qtmir/allowClientResize/+merge/319209
https://code.launchpad.net/~dandrader/unity-api/surfaceAllowClientResize/+merge/319730

* 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?
Not applicable

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

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
Lukáš Tinkl (lukas-kde) wrote : Posted in a previous version of this proposal

The code looks good, except why use this lambda?

connect(surface, &unityapi::MirSurfaceInterface::allowClientResizeChanged, this, [this]() {...}

Can't you just connect to the &Window::setAllowClientResize slot?

review: Needs Information
Revision history for this message
Daniel d'Andrada (dandrader) wrote : Posted in a previous version of this proposal

On 07/03/2017 11:27, Lukáš Tinkl wrote:
> Review: Needs Information
>
> The code looks good, except why use this lambda?
>
> connect(surface, &unityapi::MirSurfaceInterface::allowClientResizeChanged, this, [this]() {...}
>
> Can't you just connect to the &Window::setAllowClientResize slot?

No, they are different.

Window::setAllowClientResize is called from unity8. It sets the Window
property and then the Window forwards that to its surface, if any. So
the direction is Window -> Surface

That lambda comes in response to a change in the surface property.
Window then updates its value appropriately. So the direction is Surface
-> Window

Revision history for this message
Lukáš Tinkl (lukas-kde) wrote : Posted in a previous version of this proposal

Thx for the explanation

Revision history for this message
Gerry Boland (gerboland) wrote : Posted in a previous version of this proposal

+{
+ if (value != m_allowClientResize) {
+ DEBUG_MSG << "("<<value<<")";
+ m_allowClientResize = value;
+ Q_EMIT allowClientResizeChanged(m_allowClientResize);
+ if (m_surface) {
+ m_surface->setAllowClientResize(value);
+ }
+ }
Why emit the signal before doing the operation? Typically a signal is emitted after the new state has been applied.

Rest looks fine

review: Needs Information
Revision history for this message
Daniel d'Andrada (dandrader) wrote : Posted in a previous version of this proposal

On 08/03/2017 09:20, Gerry Boland wrote:
> Review: Needs Information
>
> +{
> + if (value != m_allowClientResize) {
> + DEBUG_MSG << "("<<value<<")";
> + m_allowClientResize = value;
> + Q_EMIT allowClientResizeChanged(m_allowClientResize);
> + if (m_surface) {
> + m_surface->setAllowClientResize(value);
> + }
> + }
> Why emit the signal before doing the operation? Typically a signal is emitted after the new state has been applied.
>

Because m_allowClientResize is already changed, which is what the getter
uses. Anyway, moved the signal emission to the bottom.

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
Gerry Boland (gerboland) : Posted in a previous version of this proposal
review: Approve
Revision history for this message
Lukáš Tinkl (lukas-kde) wrote : Posted in a previous version of this proposal

You forgot to update the other mocks, it doesn't compile:

/<<BUILDDIR>>/unity8-8.15+16.04.20170314/tests/plugins/Unity/Launcher/launchermodeltest.cpp: In member function ‘void LauncherModelTest::testSurfaceCountUpdates()’:
/<<BUILDDIR>>/unity8-8.15+16.04.20170314/tests/plugins/Unity/Launcher/launchermodeltest.cpp:814:60: error: invalid new-expression of abstract class type ‘MockSurface’
         surfaces->append(new MockSurface("foobar", surfaces));
                                                            ^
/<<BUILDDIR>>/unity8-8.15+16.04.20170314/tests/plugins/Unity/Launcher/launchermodeltest.cpp:41:7: note: because the following virtual functions are pure within ‘MockSurface’:
 class MockSurface: public unity::shell::application::MirSurfaceInterface
       ^
In file included from /<<BUILDDIR>>/unity8-8.15+16.04.20170314/plugins/Unity/Launcher/launcheritem.h:26:0,
                 from /<<BUILDDIR>>/unity8-8.15+16.04.20170314/tests/plugins/Unity/Launcher/launchermodeltest.cpp:22:
/usr/include/unity/shell/application/MirSurfaceInterface.h:231:18: note: virtual bool unity::shell::application::MirSurfaceInterface::allowClientResize() const
     virtual bool allowClientResize() const = 0;
                  ^
/usr/include/unity/shell/application/MirSurfaceInterface.h:232:18: note: virtual void unity::shell::application::MirSurfaceInterface::setAllowClientResize(bool)
     virtual void setAllowClientResize(bool) = 0;
                  ^

review: Needs Fixing
Revision history for this message
Gerry Boland (gerboland) : Posted in a previous version of this proposal
review: Approve
Revision history for this message
Gerry Boland (gerboland) :
review: Approve
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:2869
https://unity8-jenkins.ubuntu.com/job/lp-unity8-ci/3463/
Executed test runs:
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build/4566
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/test-0-autopkgtest/label=amd64,release=xenial+overlay,testname=qmluitests.sh/2751
    UNSTABLE: https://unity8-jenkins.ubuntu.com/job/test-0-autopkgtest/label=amd64,release=zesty,testname=qmluitests.sh/2751
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-0-fetch/4594
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=xenial+overlay/4424
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=xenial+overlay/4424/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=zesty/4424
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=zesty/4424/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=xenial+overlay/4424
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=xenial+overlay/4424/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=zesty/4424
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=zesty/4424/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=xenial+overlay/4424
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=xenial+overlay/4424/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=zesty/4424
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=zesty/4424/artifact/output/*zip*/output.zip

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

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

PASSED: Continuous integration, rev:2869
https://unity8-jenkins.ubuntu.com/job/lp-unity8-ci/3465/
Executed test runs:
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build/4574
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/test-0-autopkgtest/label=amd64,release=xenial+overlay,testname=qmluitests.sh/2757
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/test-0-autopkgtest/label=amd64,release=zesty,testname=qmluitests.sh/2757
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-0-fetch/4602
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=xenial+overlay/4429
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=xenial+overlay/4429/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=zesty/4429
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=zesty/4429/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=xenial+overlay/4429
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=xenial+overlay/4429/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=zesty/4429
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=zesty/4429/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=xenial+overlay/4429
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=xenial+overlay/4429/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=zesty/4429
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=zesty/4429/artifact/output/*zip*/output.zip

Click here to trigger a rebuild:
https://unity8-jenkins.ubuntu.com/job/lp-unity8-ci/3465/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