Merge lp:~vanvugt/mir/hidden into lp:mir

[Daniel van Vugt]

Add client API access to set surfaces "hidden".

The obvious question is: Why don't you just destroy a surface you don't
want to see? The answer is: Because toolkit ports expect this feature,
and to not have to recreate the surface (and its contents). [hikiko]

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:2210
http://jenkins.qa.ubuntu.com/job/mir-ci/2594/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/769
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/769
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/731
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/591
        deb: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/591/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/731
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/731/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/3861
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/16921

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/2594/rebuild

review: Approve (continuous-integration)

[Robert Carr (robertcarr)]

Needs information/discussion:

1. Are we sure we want this as an alternative to minimize? I kind of recall at one point maybe Chris and I came to the conclusion that we didn't. The difference would be I suppose that a client may
create a hidden surface which does not show up in the task switcher as opposed to a minimized
surface which would...but I'm not really sure this is desirable!
2. It seems unfortunate that this is now the 3rd boolean of basic surface visibility (occlusion, visible, and now hidden). Can we do anything to reduce this?

>> Add client API access to set surfaces "hidden". This is known as being
>> "unmapped" in other APIs. << SNIP >> (and its contents)

Unmapped means the contents are released so this isn't analogous.

review: Needs Information

[Daniel van Vugt (vanvugt)]

Robert:
1. I thought about that and suggested mir_surface_state_minimized myself. But yeah I guess the use case is when a surface's contents are either not ready yet (long renders) or not presently relevant. In both of those cases, minimized is not sufficient. Because "minimized" indicates something you can restore at will (it has an icon and can be switched to). P.S. Unity has a history of not "hiding" surfaces when minimized (it overrides Compiz), so it can provide live previews of them. I never liked it, but it's a thing.

2. Yeah I see how the varying terminology might sound redundant but it's actually not. From the client API there is only one way to totally hide a surface without losing its contents. That is introduced here.
  From the server's perspective it is still useful to separate the "hidden" flag from state, because "hidden" can be true for other reasons (first frame not posted, traditional minimize or mir_surface_state_hidden).

3. Removed the "unmapped" comment.

I don't know the specifics of hikiko's use case but it all sounds reasonable to me. And as you can see, it's something we already have code for.

[Daniel van Vugt (vanvugt)]

Correction:
Unity has a history of not unmapping surfaces when minimized (it overrides Compiz), so it can provide live previews of them. The unfortunate side-effect of this is that minimized windows still consume GPU cycles.

[Gerry Boland (gerboland)]

How is a shell to present an application with a hidden surface to the user? Say app creates just 1 hidden surface - should the application icon appear in the list of running apps - but clicking it does nothing? This feature opens up shell design problems I'm unclear on how to solve.

review: Needs Information

[Robert Carr (robertcarr)]

>> But yeah I guess the use case is when a surface's contents are either not ready yet (long renders) >> or not presently relevant.

I don't think either of these are the use case. In the first case, I believe the first frame posted logic accounts for this. In the second case, the application has "frozen" and the correct answer is to display the stale frame not to remove the application from the task list.

>> 2. Yeah I see how the varying terminology might sound redundant but it's actually not. From the
>> client API there is only one way to totally hide a surface without losing its contents. That is
>> introduced here.
>> From the server's perspective it is still useful to separate the "hidden" flag from state, because >> "hidden" can be true for other reasons (first frame not posted, traditional minimize or
>> mir_surface_state_hidden).

First frame not posted currently leads to visible=false but we dont say that the surface is hidden. Likewise minimize wouldn't make hidden true.

[Robert Carr (robertcarr)]

Missing implementation of ms::BasicSurface::query and test.

review: Needs Fixing

[Daniel van Vugt (vanvugt)]

By "long renders" I mean long additive renders of multiple frames. Although yes I can see that's not a valid case either -- because long additive renders are either the first frame, or spanning multiple frames so there's still something you want to display during the progress.

Robert:
ms::BasicSurface::query is already implemented, and has tests.

Gerry:
Good question. I suspect the answer is: If app has one or more _visible_ surfaces then give it an icon in the launcher. This possibly is a pre-existing problem similar to "what if you have a MirConnection with no surfaces?".

[Daniel van Vugt (vanvugt)]

hikiko:
Can you provide a link/docs about the toolkit port you need this for?

[Daniel van Vugt (vanvugt)]

Hmm, OK. The use case is actually to implement XUnmapWindow() as part of a full Xlib port.

In that case, surface contents don't need to be kept because X is traditionally only single-buffered by default. But attributes such as size and position need to remain attached to some hidden window/surface object.

While that could be implemented by destroying and recreating the Mir surface, I think the cleanest solution is still this one.

[Gerry Boland (gerboland)]

 > Gerry:
> Good question. I suspect the answer is: If app has one or more _visible_
> surfaces then give it an icon in the launcher. This possibly is a pre-existing
> problem similar to "what if you have a MirConnection with no surfaces?".

That would be confusing, as if I click an icon to launch an app, the app could appear to not have executed at all. I don't have a solution in mind, but an app without a window can be a confusing notion for a user IMO.

[Alan Griffiths (alan-griffiths)]

I've no objection to "mir_surface_state_hidden".

But the bigger question is: What surface states should exist for Mir?

The common.h header already mentions "mir_surface_state_semimaximized" (without providing it). And symmetry suggests that mir_surface_state_vertmaximized implies "mir_surface_state_horizmaximized". Might other hypothetical shells want to offer other states?

Regarding the discussion on what it means for a shell to represent "hidden" it is up to any shells to decide what states to support and how to represent them to the user. (Just because we have mir_surface_state_at_top_of_every_output doesn't mean Unity8 has to support it.)

review: Needs Information

[Robert Carr (robertcarr)]

review: Abstain

[Daniel van Vugt (vanvugt)]

I feel the answer to surface states is actually quite simple. It's an enum so any new state should be mutually exclusive with the others. And "hidden" meets that criterion.

Alan's right other shells might want more than we provide (like a mir_surface_state_horizmaximized which is something Compiz has, but the initial surface states design omits that one). We're free to add more options not in the design, just so long as the design itself is supported by our choices still.

Gerry's concern about clicking on an icon which appears to do nothing is actually an old problem that we can't avoid. This already happens if either the app crashes, or if the app chooses to not create a surface. It's not a new problem with this branch.

[Daniel van Vugt (vanvugt)]

And yeah, we expect shells to implement different states, differently. Although "hidden" I feel is not something that should be allowed to be customized by any shell. Otherwise I would have implemented it as a derivative of the managed-surface branch.

[Daniel van Vugt (vanvugt)]

Oh, actually shells can still ignore "hidden" requests using managed-surface, if they want. :)

[Alan Griffiths (alan-griffiths)]

OK I have some misgivings about managing this enum but that's definitely not a consequence of this MP

review: Approve

[Robert Carr (robertcarr)]

>> Gerry's concern about clicking on an icon which appears to do nothing is actually an old problem
>> that we can't avoid. This already happens if either the app crashes, or if the app chooses to not
>> create a surface. It's not a new problem with this branch.

I think it is. First lets look at the two problems you outline:

1. An app crashes. In this case we can remove its launcher icon. If the app is just frozen either it has never submitted its first buffer (in which case the launcher icon will never reach the opened state) or we always have a buffer available to render....so the launcher icon can always do something.
2. If an app chooses not to create a surface it will be in the Launching state...until it times out and is killed. If it just never creates a surface, then I suppose its not an app you launch from the launcher.

This branch introduces a new scenario:
3. An app is open and the launcher icon in the "launched" state, but all the surfaces are hidden. Not only is this a sort of violation of the app model (the app is consuming resources with windows that the user cant see?) but introduces this inconsistency where the launcher icon does nothing.

[Robert Carr (robertcarr)]

Now that we know the use case is for XUnmapWindow I think we can say mir_surface_state_minimized is correct right? Given that XUnmapWindow...is how you minimize winodws in X11?

review: Needs Information

[Daniel van Vugt (vanvugt)]

It's not a new problem because I remember spending parts of 2011/2012 dealing with such bugs in Unity6/7. Apps (which are locked to the launcher) that fail to start still appear to be starting for some long period (and the pulse animation was so subtle many users never even noticed it).

mir_surface_state_minimized: Yes and no.
Remember in X11 each window is made up of other little windows. Even a button is a Window in X. So the primary X use case is for widgets. That's not something we aim to replicate in Mir.
  Secondly, "mapping" a window in X is actually the realization stage in "two-stage" window creation. All windows start unmapped, you configure them, and then map. "Unmap" is the reverse but less useful... I don't think it's terrible if we continue saying we don't need "hidden" and see how it plays out.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:2212
http://jenkins.qa.ubuntu.com/job/mir-ci/2642/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/845
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/845
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/807
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/639
        deb: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/639/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/807
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/807/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/3927
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/17063

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/2642/rebuild

review: Approve (continuous-integration)

[Robert Carr (robertcarr)]

I guess what I was getting at is unmapped top level windows still show up in the 'window switcher'/'window list' etc as minimized/iconified windows. I think the use cases draws a closer metaphor to hide then (since there will only be surfaces for top level windows...and thus the unmap will always be a minimize?)

[Cemil Azizoglu (cemil-azizoglu)]

I'm also of the opinion that it doesn't hurt to have 'hidden' as a surface state. Shells do not have to use it if they don't want to.

One problem that I noticed in the implementation is that set_hidden() gets called from within set_state(). Both of these notify the observers. Essentially we are reporting the same event twice.

I suggest we do away with 'hidden_set_to' since now that hidden is a state, it'll be reported as the others through set_state. However, that'd mean show() and hide() would no longer trigger 'hidden_set_to' to the observers. Not sure if that's something we want to keep. So something needs to be done about them as well.

review: Needs Fixing

[Daniel van Vugt (vanvugt)]

Cemil: I fear removing a feature like that where I don't fully understand the consequences is a risk better left for another day, another branch. What I do understand is that yes we have extra notifications floating around. And we don't seem to ever make assumptions about the order or number of notifications so this change feels safe as it is.

Robert: Umap is not always minimize. I think you will find "minimize" is a concept that doesn't exist in X. It's a common pattern implemented by many window managers, only implemented using Unmap. Certainly during window creation in an X app you have an unmapped window that is _not_ minimized. Thereafter, it might be that the dominant use case of Unmap on top-level windows is to only use it in implementing Minimize, but I feel like that's too much of an assumption.

[Cemil Azizoglu (cemil-azizoglu)]

> Cemil: I fear removing a feature like that where I don't fully understand the
> consequences is a risk better left for another day, another branch. What I do
> understand is that yes we have extra notifications floating around. And we
> don't seem to ever make assumptions about the order or number of notifications
> so this change feels safe as it is.

Not ideal but I guess, as long as the observers can handle double notifications.... Won't block.

review: Approve

[Alberto Aguirre (albaguirre)]

A shell could always decide to implement hidden to mean the same as minimized, so one more state should be fine.

review: Approve

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:2215
http://jenkins.qa.ubuntu.com/job/mir-ci/2672/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/885
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/885
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/847
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/669
        deb: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/669/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/847
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/847/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/3961
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/17141

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/2672/rebuild

review: Approve (continuous-integration)

[Daniel van Vugt (vanvugt)]

This whole discussion has left me to second-guess the value of this branch. Maybe needs more reviews.

=== modified file 'client-ABI-sha1sums'
--- client-ABI-sha1sums	2015-01-15 13:07:37 +0000
+++ client-ABI-sha1sums	2015-01-16 03:11:51 +0000
@@ -10,7 +10,7 @@
 b141c4d79802ad626d969249c0004744e5c2a525  include/client/mir_toolkit/mir_wait.h
 6f7b4ecc22afba923806ed2bd7d8244be90b0cfd  include/client/mir_toolkit/version.h
 3350dac884d6006753de2a955bc7a05663cd9449  include/common/mir_toolkit/client_types.h
-76f831c2160dd42807fab3fa560463d04f141ade  include/common/mir_toolkit/common.h
+b042ff8f542759fb82ad4cf1f4bab4aa2824e7f0  include/common/mir_toolkit/common.h
 fce4c1a9e0d037244f7e9e96ea2d8eaab4fc404c  include/common/mir_toolkit/cursors.h
 f4d39e9893ce6308bddd83a49b90f0051f565323  include/common/mir_toolkit/event.h
 2507f2929415aa423f9551d3c595c439fe1c6efd  include/common/mir_toolkit/events/event_deprecated.h

=== modified file 'common-ABI-sha1sums'
--- common-ABI-sha1sums	2015-01-15 13:07:37 +0000
+++ common-ABI-sha1sums	2015-01-16 03:11:51 +0000
@@ -17,7 +17,7 @@
 31b9c24e2ce7194aeea6694e81c160354033d28a  include/common/mir/optional_value.h
 9ae8473df05dd9e048a73797f01a2f34f7447554  include/common/mir/time/types.h
 3350dac884d6006753de2a955bc7a05663cd9449  include/common/mir_toolkit/client_types.h
-76f831c2160dd42807fab3fa560463d04f141ade  include/common/mir_toolkit/common.h
+b042ff8f542759fb82ad4cf1f4bab4aa2824e7f0  include/common/mir_toolkit/common.h
 fce4c1a9e0d037244f7e9e96ea2d8eaab4fc404c  include/common/mir_toolkit/cursors.h
 f4d39e9893ce6308bddd83a49b90f0051f565323  include/common/mir_toolkit/event.h
 2507f2929415aa423f9551d3c595c439fe1c6efd  include/common/mir_toolkit/events/event_deprecated.h

=== modified file 'include/common/mir_toolkit/common.h'
--- include/common/mir_toolkit/common.h	2015-01-14 12:29:17 +0000
+++ include/common/mir_toolkit/common.h	2015-01-16 03:11:51 +0000
@@ -75,6 +75,7 @@
        differs only in the X coordinate. */
     mir_surface_state_fullscreen,
     mir_surface_state_horizmaximized,
+    mir_surface_state_hidden,
     mir_surface_states
 } MirSurfaceState;
 

=== modified file 'platform-ABI-sha1sums'
--- platform-ABI-sha1sums	2015-01-15 13:07:37 +0000
+++ platform-ABI-sha1sums	2015-01-16 03:11:51 +0000
@@ -17,7 +17,7 @@
 31b9c24e2ce7194aeea6694e81c160354033d28a  include/common/mir/optional_value.h
 9ae8473df05dd9e048a73797f01a2f34f7447554  include/common/mir/time/types.h
 3350dac884d6006753de2a955bc7a05663cd9449  include/common/mir_toolkit/client_types.h
-76f831c2160dd42807fab3fa560463d04f141ade  include/common/mir_toolkit/common.h
+b042ff8f542759fb82ad4cf1f4bab4aa2824e7f0  include/common/mir_toolkit/common.h
 fce4c1a9e0d037244f7e9e96ea2d8eaab4fc404c  include/common/mir_toolkit/cursors.h
 f4d39e9893ce6308bddd83a49b90f0051f565323  include/common/mir_toolkit/event.h
 2507f2929415aa423f9551d3c595c439fe1c6efd  include/common/mir_toolkit/events/event_deprecated.h

=== modified file 'server-ABI-sha1sums'
--- server-ABI-sha1sums	2015-01-15 13:07:37 +0000
+++ server-ABI-sha1sums	2015-01-16 03:11:51 +0000
@@ -17,7 +17,7 @@
 31b9c24e2ce7194aeea6694e81c160354033d28a  include/common/mir/optional_value.h
 9ae8473df05dd9e048a73797f01a2f34f7447554  include/common/mir/time/types.h
 3350dac884d6006753de2a955bc7a05663cd9449  include/common/mir_toolkit/client_types.h
-76f831c2160dd42807fab3fa560463d04f141ade  include/common/mir_toolkit/common.h
+b042ff8f542759fb82ad4cf1f4bab4aa2824e7f0  include/common/mir_toolkit/common.h
 fce4c1a9e0d037244f7e9e96ea2d8eaab4fc404c  include/common/mir_toolkit/cursors.h
 f4d39e9893ce6308bddd83a49b90f0051f565323  include/common/mir_toolkit/event.h
 2507f2929415aa423f9551d3c595c439fe1c6efd  include/common/mir_toolkit/events/event_deprecated.h

=== modified file 'src/server/scene/basic_surface.cpp'
--- src/server/scene/basic_surface.cpp	2014-12-28 08:33:45 +0000
+++ src/server/scene/basic_surface.cpp	2015-01-16 03:11:51 +0000
@@ -417,6 +417,7 @@
     {
         state_ = s;
         lg.unlock();
+        set_hidden(s == mir_surface_state_hidden);
         
         observers.attrib_changed(mir_surface_attrib_state, s);
     }

=== modified file 'tests/unit-tests/scene/test_basic_surface.cpp'
--- tests/unit-tests/scene/test_basic_surface.cpp	2014-12-28 08:33:45 +0000
+++ tests/unit-tests/scene/test_basic_surface.cpp	2015-01-16 03:11:51 +0000
@@ -309,6 +309,9 @@
 
     surface.set_hidden(false);
     EXPECT_TRUE(surface.visible());
+
+    surface.configure(mir_surface_attrib_state, mir_surface_state_hidden);
+    EXPECT_FALSE(surface.visible());
 }
 
 TEST_F(BasicSurfaceTest, test_surface_hidden_notifies_changes)