Merge lp:~brandontschaefer/mir/add-mir-chrome-none into lp:mir

[Brandon Schaefer]

Add missing chrome_none field.

Add missing chrome_none field.

[Mir CI Bot (mir-ci-bot)]

FAILED: Continuous integration, rev:3885
https://mir-jenkins.ubuntu.com/job/mir-ci/2425/
Executed test runs:
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-mir/3156/console
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/3223
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/3215
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial+overlay/3215
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=yakkety/3215
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=yakkety/3185
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=yakkety/3185/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/3185
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/3185/artifact/output/*zip*/output.zip
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=yakkety/3185/console
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/3185/console
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/3185/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/3185
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/3185/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/3185
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/3185/artifact/output/*zip*/output.zip

Click here to trigger a rebuild:
https://mir-jenkins.ubuntu.com/job/mir-ci/2425/rebuild

review: Needs Fixing (continuous-integration)

[Cemil Azizoglu (cemil-azizoglu)]

It would make sense to mention why we need this. Also, this should have been the first element in the enum, is it too late for that? If it is, do you want to assign some value to it to leave a gap in case you need other chrome values? (i.e.

mir_shell_chrome_normal,
mir_shell_chrome_low,
mir_shell_chrome_none = 1000

or some such thing?)

review: Needs Information

[Daniel van Vugt (vanvugt)]

If we're breaking the ABI _and_ API that uses it then you can probably reorder the enum. Otherwise I would lean toward not reordering, for safety.

In terms of documenting its purpose, I think it's the same thing as mir::compositor::Decoration::Type. We could replace that Type with MirShellChrome I guess but the noun "chrome" only has meaning within Canonical design/management AFAIK. It's not a recognised word in wider circles so maybe this is an opportunity to rename it to something more correct like MirWindowDecoration {none, low, normal}...

review: Needs Information

[Chris Halse Rogers (raof)]

I don't see any particular reason why mir_shell_chrome_none *should* be the first member, nor is the first member all that special.

You probably want to introduce this as a part of something that actually uses it, though.

[Daniel van Vugt (vanvugt)]

The only public interface using it is:

   include/client/mir_toolkit/mir_surface.h:void mir_surface_spec_set_shell_chrome(MirSurfaceSpec* spec, MirShellChrome style);

And we want to replace that anyway. So we should take the opportunity to deprecate the whole enum and replace it with our newer naming convention:

   enum MirWindowDecor /* or Decoration or Chrome or whatever */
   mir_window_spec_set_decor()

Although this particular change is harmless, it clearly also is about to be deprecated.

review: Disapprove

[Alan Griffiths (alan-griffiths)]

When (and why) would this be useful?

review: Needs Information

[Daniel van Vugt (vanvugt)]

It's useful because this is the mechanism by which a toolkit can say "hey this is a normal window but I will draw my own decorations so dear shell; don't do it for me". Hence chrome is "none".

It's not a specific window type that might be client-decorated. Any window type might be client-decorated, depending on what the toolkit wants (GTK especially).

[Daniel van Vugt (vanvugt)]

Since I was the person who proposed this change in conversation, and the one disapproving it, and the one suggesting an alternative, I think that's enough grounds for me to reject this branch.

I'll propose something more appropriate in future...

Unmerged revisions

3885. By Brandon Schaefer on 2016-12-17

* Add chrome_none

=== modified file 'include/core/mir_toolkit/common.h'
--- include/core/mir_toolkit/common.h	2016-11-11 07:56:09 +0000
+++ include/core/mir_toolkit/common.h	2016-12-17 00:12:25 +0000
@@ -350,6 +350,7 @@
 {
     mir_shell_chrome_normal,
     mir_shell_chrome_low,
+    mir_shell_chrome_none
 } MirShellChrome;
 
 /**