Merge lp://staging/~nick-dedekind/unity-api/shell_chrome into lp://staging/unity-api

Proposed by Nick Dedekind
Status: Merged
Approved by: Michael Terry
Approved revision: 212
Merged at revision: 218
Proposed branch: lp://staging/~nick-dedekind/unity-api/shell_chrome
Merge into: lp://staging/unity-api
Diff against target: 124 lines (+40/-1)
4 files modified
include/unity/shell/application/CMakeLists.txt (+1/-1)
include/unity/shell/application/Mir.h (+23/-0)
include/unity/shell/application/MirSurfaceInterface.h (+8/-0)
include/unity/shell/application/MirSurfaceItemInterface.h (+8/-0)
To merge this branch: bzr merge lp://staging/~nick-dedekind/unity-api/shell_chrome
Reviewer Review Type Date Requested Status
Gerry Boland (community) Approve
Unity8 CI Bot continuous-integration Approve
PS Jenkins bot (community) continuous-integration Approve
Review via email: mp+286309@code.staging.launchpad.net

Commit message

Added support for low shell chrome

To post a comment you must log in.
211. By Nick Dedekind

whitespace

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

PASSED: Continuous integration, rev:211
https://unity8-jenkins.ubuntu.com/job/lp-unity-api-1-ci/22/
Executed test runs:
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build/548
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-0-fetch/571
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/589
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial/589
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=vivid+overlay/585
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=vivid+overlay/585/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=xenial/585
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=xenial/585/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=vivid+overlay/585
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=vivid+overlay/585/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=xenial/585
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=xenial/585/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=vivid+overlay/585
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=vivid+overlay/585/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=xenial/585
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=xenial/585/artifact/output/*zip*/output.zip

Click here to trigger a rebuild:
https://unity8-jenkins.ubuntu.com/job/lp-unity-api-1-ci/22/rebuild

review: Approve (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
Gerry Boland (gerboland) wrote :

I totally reviewed this, but it got lost somehow. Doing again.

Mainly, I don't understand what is meant by shell chrome here, and what it has to do with the linked bug. You referring to titlebar?

+ * @brief The Shell crhome mode
typo

Revision history for this message
Nick Dedekind (nick-dedekind) wrote :

> I totally reviewed this, but it got lost somehow. Doing again.
>
> Mainly, I don't understand what is meant by shell chrome here, and what it has
> to do with the linked bug. You referring to titlebar?
>
> + * @brief The Shell crhome mode
> typo

"Shell chrome" is a mir concept, but my understanding is that it refers to whether "non application" parts of the shell (indicators in our use case) are visible.

For how this relates to the bug; we have changed touch apps to use low chrome mode when wanting "full screen" in phone/tablet form factor, and Window.state = FullScreen when wanting it in desktop mode.

Revision history for this message
Gerry Boland (gerboland) wrote :

> "Shell chrome" is a mir concept, but my understanding is that it refers to
> whether "non application" parts of the shell (indicators in our use case) are
> visible.

So, it is a hint from the server to the client, telling the client about a shell visual state, allowing client to decide surface state? Or is it a hint from the client to the server to recommend server to hide its chrome? What if server doesn't support or allow different chrome modes?

Also, are we're implying that "low chrome" is zero chrome?

I can find no documentation for the exact purpose of this thing in Mir. Frankly I'm not a fan of it.

> For how this relates to the bug; we have changed touch apps to use low chrome
> mode when wanting "full screen" in phone/tablet form factor, and Window.state
> = FullScreen when wanting it in desktop mode.

I understand this is now Mir api, but I won't be the first person to find this confusing. If this is indeed a hint from server->client, then (aside from "low chrome" being a vague concept) at least it means the client can decide the suitable surface state. If it a client->server hint, I'm less clear on what happens.

Revision history for this message
Alan Griffiths (alan-griffiths) wrote :

> I understand this is now Mir api, but I won't be the first person to find this
> confusing. If this is indeed a hint from server->client, then (aside from "low
> chrome" being a vague concept) at least it means the client can decide the
> suitable surface state. If it a client->server hint, I'm less clear on what
> happens.

This API comes from a request from U8 and discussions at the January sprint.

"low chrome" is a request from the client to the server. What the server does with it is up to the server and can depend upon the surface state. The "chrome" terminology was suggested by Chris as being a "term of art" that covered "indicators and other stuff".

Revision history for this message
Nick Dedekind (nick-dedekind) wrote :

> > "Shell chrome" is a mir concept, but my understanding is that it refers to
> > whether "non application" parts of the shell (indicators in our use case)
> are
> > visible.
>
> So, it is a hint from the server to the client, telling the client about a
> shell visual state, allowing client to decide surface state? Or is it a hint
> from the client to the server to recommend server to hide its chrome? What if
> server doesn't support or allow different chrome modes?

It's a hint from client to server to tell the server what level of shell chrome we want visible for the application.
This is essentially the same thing we were using the fullscreen window state for previously.
For now we've hacked in a "well known" window flag into qtubuntu & the clients to represent this "low chrome" mode client side.
Believe me, this is far from a "good solution". I'm not sure this is intended to stay this way or not. There is already a flag in qt 5.5 which may be suitable, but we're not there yet.
I still believe handling form factor changes client side was the way to go with this, but others disagreed. Plus we couldn't get that working in time...

>
> Also, are we're implying that "low chrome" is zero chrome?
>

Chrome state is interpreted by the shell. In our case low chrome means "fullscreen" for phone/tablet.

> I can find no documentation for the exact purpose of this thing in Mir.
> Frankly I'm not a fan of it.
>
> > For how this relates to the bug; we have changed touch apps to use low
> chrome
> > mode when wanting "full screen" in phone/tablet form factor, and
> Window.state
> > = FullScreen when wanting it in desktop mode.
>
> I understand this is now Mir api, but I won't be the first person to find this
> confusing. If this is indeed a hint from server->client, then (aside from "low
> chrome" being a vague concept) at least it means the client can decide the
> suitable surface state. If it a client->server hint, I'm less clear on what
> happens.

Not sure where I can go with this and I agree that it is confusing. Perhaps we can refine it; but this is wanted for OTA 10; so....

Revision history for this message
Nick Dedekind (nick-dedekind) wrote :

> > "Shell chrome" is a mir concept, but my understanding is that it refers to
> > whether "non application" parts of the shell (indicators in our use case)
> are
> > visible.
>
> So, it is a hint from the server to the client, telling the client about a
> shell visual state, allowing client to decide surface state? Or is it a hint
> from the client to the server to recommend server to hide its chrome? What if
> server doesn't support or allow different chrome modes?

It's a hint from client to server to tell the server what level of shell chrome we want visible for the application.
This is essentially the same thing we were using the fullscreen window state for previously.
For now we've hacked in a "well known" window flag into qtubuntu & the clients to represent this "low chrome" mode client side.
Believe me, this is far from a "good solution". I'm not sure this is intended to stay this way or not. There is already a flag in qt 5.5 which may be suitable, but we're not there yet.
I still believe handling form factor changes client side was the way to go with this, but others disagreed. Plus we couldn't get that working in time...

>
> Also, are we're implying that "low chrome" is zero chrome?
>

Chrome state is interpreted by the shell. In our case low chrome means "fullscreen" for phone/tablet.

> I can find no documentation for the exact purpose of this thing in Mir.
> Frankly I'm not a fan of it.
>
> > For how this relates to the bug; we have changed touch apps to use low
> chrome
> > mode when wanting "full screen" in phone/tablet form factor, and
> Window.state
> > = FullScreen when wanting it in desktop mode.
>
> I understand this is now Mir api, but I won't be the first person to find this
> confusing. If this is indeed a hint from server->client, then (aside from "low
> chrome" being a vague concept) at least it means the client can decide the
> suitable surface state. If it a client->server hint, I'm less clear on what
> happens.

Not sure where I can go with this and I agree that it is confusing. Perhaps we can refine it; but this is wanted for OTA 10; so....

Revision history for this message
Gerry Boland (gerboland) wrote :

Ok. Will accept this is a client to server hint.

But please add more documentation to indicate the expected consequence in the shell of a client setting this.

Now I need to ask about how it will behave (this being as good a place as any to bring up these points):

1. does this only impact a Windowed surface? i.e. what does NormalChrome mode mean for a fullscreen surface? what does LowChrome mean for VerticallyMaximised?

2. I, as a client, ask for a Windowed surface with LowChrome. On phone, this will end up being a fullscreen surface, but windowed on desktop. Do I, as a client, know that I'm fullscreen on the phone? Or do I think I'm windowed? i.e. does shell actually inform the client of the dynamically chosen surface state it has chosen?

review: Needs Information
Revision history for this message
Nick Dedekind (nick-dedekind) wrote :

> Ok. Will accept this is a client to server hint.
>
> But please add more documentation to indicate the expected consequence in the
> shell of a client setting this.

Will do.

>
> Now I need to ask about how it will behave (this being as good a place as any
> to bring up these points):
>
> 1. does this only impact a Windowed surface? i.e. what does NormalChrome mode
> mean for a fullscreen surface? what does LowChrome mean for
> VerticallyMaximised?

Chrome only has meaning for phone/tablet stages at the moment (and to determine if fullscreen surfaces are restored when changing to desktop). The logic for treatment of chrome is only in unity8 stages code. All the work in qtmir/unity-api/qtubuntu is just pass-through for the chrome & states info.

>
> 2. I, as a client, ask for a Windowed surface with LowChrome. On phone, this
> will end up being a fullscreen surface, but windowed on desktop. Do I, as a
> client, know that I'm fullscreen on the phone? Or do I think I'm windowed?
> i.e. does shell actually inform the client of the dynamically chosen surface
> state it has chosen?

Here is the logic behind what happens when we change the chrome state and fullscreen.
Clients are informed of surface state changes related to the chrome logic.

In phone/tablet stage:
Flag change to LowChrome -> server sets client window state to "fullscreen"
Flag change to NormalChrome -> server sets client window to "restored" state.
Flag set and state change to restored -> server RESETS client window state to "fullscreen"
Flag not set and state change to restore -> client window stays "restored"
Flag not set and state change to fulscreen -> client window stays "fullscreen"

In desktop stage (only on switching to desktop mode):
Flag set to LowChrome and state is fullscreen -> server sets client window state to "restored"

212. By Nick Dedekind

typo

Revision history for this message
Nick Dedekind (nick-dedekind) wrote :

> I totally reviewed this, but it got lost somehow. Doing again.
>
> Mainly, I don't understand what is meant by shell chrome here, and what it has
> to do with the linked bug. You referring to titlebar?
>
> + * @brief The Shell crhome mode
> typo

Fixed.
My new least favourite word. You have no idea how many times I've misspelled chrome. crome, crhome ...arggg!

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

PASSED: Continuous integration, rev:212
https://unity8-jenkins.ubuntu.com/job/lp-unity-api-ci/32/
Executed test runs:
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-0-fetch/884
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/900
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial/900
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=vivid+overlay/898
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=vivid+overlay/898/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=xenial/898
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=xenial/898/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=vivid+overlay/898
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=vivid+overlay/898/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=xenial/898
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=xenial/898/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=vivid+overlay/898
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=vivid+overlay/898/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=xenial/898
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=xenial/898/artifact/output/*zip*/output.zip

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

review: Approve (continuous-integration)
Revision history for this message
Gerry Boland (gerboland) wrote :

Thanks for patiently answering my queries. This looks fine.

review: Approve

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

to all changes: