Mir

Merge lp:~robertcarr/mir/client-focus-notifications into lp:~mir-team/mir/trunk

Proposed by Robert Carr
Status: Merged
Approved by: kevin gunn
Approved revision: 782
Merged at revision: 1003
Proposed branch: lp:~robertcarr/mir/client-focus-notifications
Merge into: lp:~mir-team/mir/trunk
Diff against target: 1138 lines (+481/-104)
31 files modified
include/server/mir/frontend/message_processor_report.h (+4/-0)
include/server/mir/frontend/null_message_processor_report.h (+2/-0)
include/server/mir/frontend/shell.h (+2/-0)
include/server/mir/logging/message_processor_report.h (+2/-0)
include/server/mir/lttng/message_processor_report.h (+1/-0)
include/server/mir/shell/default_focus_mechanism.h (+7/-1)
include/server/mir/shell/session_manager.h (+2/-0)
include/shared/mir_toolkit/common.h (+12/-3)
include/test/mir_test/event_matchers.h (+14/-0)
include/test/mir_test_doubles/mock_shell.h (+1/-0)
include/test/mir_test_doubles/stub_shell.h (+4/-1)
include/test/mir_test_framework/input_testing_server_configuration.h (+2/-3)
src/client/mir_connection.cpp (+4/-1)
src/client/mir_surface.cpp (+3/-4)
src/client/mir_surface.h (+1/-1)
src/server/frontend/null_message_processor_report.cpp (+4/-0)
src/server/frontend/session_mediator.cpp (+35/-29)
src/server/frontend/socket_messenger.cpp (+2/-0)
src/server/logging/message_processor_report.cpp (+6/-3)
src/server/lttng/message_processor_report.cpp (+6/-0)
src/server/shell/default_focus_mechanism.cpp (+7/-0)
src/server/shell/session_manager.cpp (+13/-2)
src/server/shell/surface.cpp (+5/-2)
tests/acceptance-tests/CMakeLists.txt (+1/-0)
tests/acceptance-tests/test_client_focus_notification.cpp (+232/-0)
tests/acceptance-tests/test_client_input.cpp (+3/-1)
tests/acceptance-tests/test_focus_selection.cpp (+1/-0)
tests/acceptance-tests/test_shell_control_of_surface_configuration.cpp (+7/-0)
tests/mir_test_framework/input_testing_server_options.cpp (+38/-53)
tests/unit-tests/shell/test_default_focus_mechanism.cpp (+26/-0)
tests/unit-tests/shell/test_surface.cpp (+34/-0)
To merge this branch: bzr merge lp:~robertcarr/mir/client-focus-notifications
Reviewer Review Type Date Requested Status
PS Jenkins bot (community) continuous-integration Approve
kevin gunn (community) Approve
Daniel van Vugt Needs Fixing
Alan Griffiths Abstain
Robert Ancell Approve
Chris Halse Rogers Approve
Alexandros Frantzis Pending
Review via email: mp+180903@code.launchpad.net

This proposal supersedes a proposal from 2013-08-15.

Commit message

Send focus notifications to client.

Description of the change

This time the dependency is actually removed...properly! accidentally reverse merged some stuff which was making the diff huge.

===

Another iteration. This time I've removed the dependency on connect-display-request-merge-trunk. I've adapted the handle_surface_created idea from that branch (thanks kevin :D) to enable things to work without the cloggable sink.

Currently the shell mixes responsibilities of a factory interface, and reactive behavior (largely through acting as part of the factory/builder). I hope we can run with this and split out the factory part, in to classes which do that, and be left with a sensible interface like:

mf::Shell
{
handle_session_created
handle_sesssion_destroyed
handle_surface_created
handle_surface_destroyed
etc etc
}

The session mediator (and presumably a to be coming InternalClientMediator) then acts to notify the shell after building the objects.

=== Last description ==
This branch is a new version of client-focus-notifications based on ~mir-team/mir/connect-display-request-merge-trunk which enables removal of the cloggable event sink (notice how focus is now set in handle_surface_created, executed after the surface frontend processing has completed).

I've elected to still use surface attributes for focus, for two reasons:
1. I can't think of anything else to use
2. mir_surface_get_state(mir_surface_attrib_focus...seems to make sense as client API.

I think perhaps though, the default_focus_mechanism should handle the response of surface->configure with surface_attrib_focus. This way the shell can use the surface configurator to interfere and say, actually this surface may not take focus. I'm happy to include it in this MP if it's a popular idea or propose a followup for discussion after.

To post a comment you must log in.
Revision history for this message
Robert Carr (robertcarr) wrote : Posted in a previous version of this proposal

There are a few open questions:

Should focus be modeled as a Surface attribute?
Is there any reason for the Surface to keep track of if it has focus.

Revision history for this message
kevin gunn (kgunn72) wrote : Posted in a previous version of this proposal

> There are a few open questions:
>
> Should focus be modeled as a Surface attribute?
> Is there any reason for the Surface to keep track of if it has focus.

makes sense to me it would be a surface attribute. as to why it might want to keep track....optimizations if there is no focus? and possibly, other client requests that won't work/don't make sense if focused or not.

just curious...is this some convention ?
mir_surface_focus_arraysize_
didn't understand why it was in the surf attrib for focusstate

in general...might be my lack of understanding. there are some mutex/surface locks in the proposed code....but i don't see unlocks (do those happen automagically as the code goes out of scope?)

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote : Posted in a previous version of this proposal
review: Approve (continuous-integration)
Revision history for this message
Alexandros Frantzis (afrantzis) wrote : Posted in a previous version of this proposal

> just curious...is this some convention ?
> mir_surface_focus_arraysize_
> didn't understand why it was in the surf attrib for focusstate

Yes, it is a sentinel element for the array, and its value is equal to the array size.

> in general...might be my lack of understanding. there are some mutex/surface
> locks in the proposed code....but i don't see unlocks (do those happen
> automagically as the code goes out of scope?)

std::unique_lock<> and std::lock_guard<> are scoped locks. They lock the associated mutex when created, and unlock it when going out of scope.

Revision history for this message
Alexandros Frantzis (afrantzis) wrote : Posted in a previous version of this proposal

202 + std::unique_lock<std::mutex> lg(lock);
207 + std::unique_lock<std::mutex> lg(lock);

Are these locks necessary? Can a SessionMediator service two requests (in this case, two create_surface requests) concurrently? If it can, then the locks, as they are, have the following problem:

(t1) clog
(t2) clog
(t1) unclog, oops, any events for the surface created in (t2) will be sent to the client, too

214 + void handle_event(MirEvent const& ev)
215 + {
216 + if (clogged)

Taking into account the previous point (assuming the mediator can't service two requests from the same connection concurrently) and since this access to clogged is not protected, I suggest that we make clogged atomic<bool> and get rid of the locks.

315 + notify_change(attrib, value);
316 + break;

Indented one level deeper than required.

374 +namespace
375 +{
376 +
377 + struct ClientConfigCommon : TestingClientConfiguration
378 + {
379 + ClientConfigCommon() :
380 + connection(0),
381 + surface(0)
382 + {
383 + }
...

No need for indentation in namespace block.

415 + surface = NULL;

nullptr

505 +TEST_F(BespokeDisplayServerTestFixture, two_surfaces_are_notified_of_gaining_and_losing_focus)
552 + // We need some synchronization to ensure client two does not connect before client one.

Doesn't waiting for ready_for_second_client semaphore take care of that (it triggers only after the first client has received a surface created event, which means it has already connected)?

However, the test is still fragile. If the second client hasn't managed to connect and create a surface in the 5 seconds the first test has to finish, e.g., because of super high system load, then all_events_received.wait_for_at_most_seconds(5) in the first client will expire and the test will fail.

review: Needs Fixing
Revision history for this message
Alan Griffiths (alan-griffiths) wrote : Posted in a previous version of this proposal

CloggableEventSink looks horribly broken:

There is no synchronization around "216 + if (clogged)" - which means it could be in different states in different threads.

"clogged" could change between "if (clogged)" being evaluated and the next statement.

There is nothing to prevent multiple calls to clog() - perhaps clogged should be an atomic counter?

I assume that clog() and unclog() are intended to be paired operations - but the calls I see don't deal with exceptions being thrown (please use RAII for paired operations).

What is the motivation for CloggableEventSink? Is this the right place for synchronization?

review: Needs Fixing
Revision history for this message
Robert Carr (robertcarr) wrote : Posted in a previous version of this proposal

Hi all! Thanks for review...I think I have improved it some but still not found the final solution.

Starting with Alf's comments:

>> Are these locks necessary? Can a SessionMediator service two requests (in this case, two create_surface
>> requests) concurrently

As it stands the SessionMediator can not service two requests concurrently (the SocketSession only reads a new request after processing the last one). These locks are necessary however, as handle_event may be called from any thread (i.e. the shell configuring a surface from any thread), to prevent a data race on the event buffer.

I would propose we make explicit that the SessionMediator can not service two requests with locking.

>> SNIP

Indentation and nullptr and such fixed.

>> Doesn't waiting for ready_for_second_client semaphore take care of that (it triggers only after
>> the first client has received a surface created event, which means it has already connected)?

Yes sorry I moved the wait without moving the comment making this misleading. This is in fact the point of ready_for_second_client.

>> However, the test is still fragile. If the second client hasn't managed to connect and create a surface in
>> the 5 seconds the first test has to finish, e.g., because of super high system load, then
>> all_events_received.wait_for_at_most_seconds(5) in the first client will expire and the test will fail.

The test will always be fragile or have the possibility to lock indefinitely. I think 5 seconds is perhaps to short though (things have taken longer than this in valgrind on jenkins in the past) so I've bumped the timeouts to 60 seconds. We could consider a standard set of timeout values.

Alan:

>>
>> There is no synchronization around "216 + if (clogged)" - which means it could be in different states in >> different threads.
>>

This is just in error of course. Fixed.

>>
>> What is the motivation for CloggableEventSink? Is this the right place for synchronization?
>>

The motivation is that sending a surface configuration event (i.e. one generated before shell::SessionManager::create_surface returns) before replying to the create_surface request leaves client session in a situation which it can't know how to rectify. I think the SessionMediator acts to arbitrate communication between the Session and the IPC layer, and so this is in fact the right place for synchronization.

>>
>> There is nothing to prevent multiple calls to clog() - perhaps clogged should be an atomic counter?
>> I assume that clog() and unclog() are intended to be paired operations - but the calls I see don't deal
>> with exceptions being thrown (please use RAII for paired operations).
>>

I'll update the API to be safer at the beginning of next week after some feedback from the team on the high level approach.

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote : Posted in a previous version of this proposal
review: Needs Fixing (continuous-integration)
Revision history for this message
Alan Griffiths (alan-griffiths) wrote : Posted in a previous version of this proposal

Events should be removed from buffered_events once handled. (Consider also: what should happen if handle_event() throws.)

review: Needs Fixing
Revision history for this message
Robert Carr (robertcarr) wrote : Posted in a previous version of this proposal

Fixed the failure to clear buffered_events (whoops!)

There is something strange going on with the acceptance tests under valgrind (intermittent failure to complete, client never receives the event). I'll stew on it (and the whole branch) overnight and revisit tomorrow.

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote : Posted in a previous version of this proposal
review: Needs Fixing (continuous-integration)
Revision history for this message
Robert Carr (robertcarr) wrote : Posted in a previous version of this proposal

The valgrind-induced race-triggered test-failure was due to the test client code, see the comment and change at l555 (vs the previous location of mir_surface_set_event_handler).

I think EventDelegate should be moved to the surface_create arguments. As it stands now mir_surface_create_sync should be considered dangerous.

Revision history for this message
Robert Carr (robertcarr) wrote : Posted in a previous version of this proposal

In the process of debugging message_processor_report grew a sent_event method, as did the logging implementation (not sure how to print MirEvent from lttng...). Note the diff to add a single print line amounts to 141 lines...a bit unsatisfying imo.

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote : Posted in a previous version of this proposal
review: Approve (continuous-integration)
Revision history for this message
Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal

Nice to see the attribute logic being reused, but I don't think focus is an attribute of a Surface in the traditional sense. But it's kind of OK too because you're only doing notifications here.

I would like to say focus should never be an attribute because that increases the risk and confusion of potentially having multiple focused surfaces. And internally at least the attribute system is one for getting/setting simple values. But being able to set focus per surface would be dangerous.

This risk is mitigated by the absence of a mir_surface_set_focus. It's only a bit wrong internally then [1]. And I agree you need focus to be an attribute to get notifications for it. So I'm glad you're using the attribute notifications as I intended.

I'm not sure though.

If anything I'll just say that "focused" should be the attribute (boolean 0 or 1) instead of focus. And even if you disagree with renaming then please reorder the enum MirSurfaceFocusState so it's usable as a boolean.

[1] The "bit wrong internally" comment is really about this:
437 + case mir_surface_attrib_focus:
438 + notify_change(attrib, value);
439 + break;
You've implemented a stub setter for focus in msh::Surface::configure which doesn't set anything. In fact instead of notify_change and returning the default value 0 (which is wrong BTW) it should be throwing an exception on any attempt to set ("configure") focus. There must be a nicer way to do it, without modifying msh::Surface::configure (hence letting it throw on any attempt to configure focus).

review: Needs Fixing
Revision history for this message
Alan Griffiths (alan-griffiths) wrote : Posted in a previous version of this proposal

198 + virtual ~IPCSemaphore();

Prefer a non-virtual, noexcept destructor

~~~~

549 + client->handler->handle_event(ev);

If "ev" were given the more meaningful name the sheer repetitiveness would be even more apparent:

    client->handler->handle_event(event);

You can see it here too:

948 + event_sink->handle_event(any_event);

s/handle_event/handle/ would be an improvement.

~~~~

98 + std::weak_ptr<Surface> focus_surface;

What is the purpose of this? The name isn't clear, and the usage is entirely at member function scope (so why use a member variable).

review: Needs Fixing
Revision history for this message
Robert Carr (robertcarr) wrote : Posted in a previous version of this proposal

Thanks both!

Alan: Comments addressed. Name improved to be clear at l98

Daniel: I've flipped the enum values...good point. As for the use of configure...I'm not sure of a better way to do this until a bigger focus refactoring happens. This is "next on my list"

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote : Posted in a previous version of this proposal
review: Approve (continuous-integration)
Revision history for this message
Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal

Again with:
437 + case mir_surface_attrib_focus:
438 + notify_change(attrib, value);
439 + break;

It would be easy to write reasonable test cases that now fail. Similar to:
tests/unit-tests/shell/test_surface.cpp: TEST_F(ShellSurface, states)

Because calling:
    surf.configure(mir_surface_attrib_focus, X) != X
and fails to throw an exception too.

review: Needs Fixing
Revision history for this message
Alexandros Frantzis (afrantzis) wrote : Posted in a previous version of this proposal

> As for the use of configure...I'm not sure of a better way to do this until a bigger focus refactoring happens. This is "next on my list".

If you have in mind a better way to achieve this, perhaps we should treat this MP as a spike, drop it, and move on to a refactoring that will allow implementing a better solution?

review: Abstain
Revision history for this message
Alan Griffiths (alan-griffiths) wrote : Posted in a previous version of this proposal

I assume that clog() and unclog() are intended to be paired operations - but the calls I see don't deal with exceptions being thrown (please use RAII for paired operations).

review: Needs Fixing
Revision history for this message
Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal

I really don't like RAII because it's too implicit and hidden from the reader. That said, you kind of need it for exception safety. But I would recommend against ever using exceptions too :)

Revision history for this message
Alan Griffiths (alan-griffiths) wrote : Posted in a previous version of this proposal

> I really don't like RAII because it's too implicit and hidden from the reader.
> That said, you kind of need it for exception safety. But I would recommend
> against ever using exceptions too :)

You'd recommend against using C++ then?

RAAI and exceptions have been idiomatic C++ for two decades now.

Revision history for this message
Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal

Well, for many people not using C++ or RAII has been idomatic for almost four decades ;)
</troll>

Revision history for this message
Olli Ries (ories) wrote : Posted in a previous version of this proposal

you guys... ;)

On Mon, Jun 10, 2013 at 2:28 AM, Daniel van Vugt <
<email address hidden>> wrote:

> Well, for many people not using C++ or RAII has been idomatic for almost
> four decades ;)
> </troll>
> --
>
> https://code.launchpad.net/~robertcarr/mir/client-focus-notifications/+merge/166440
> Your team Mir development team is subscribed to branch lp:mir.
>

Revision history for this message
Robert Carr (robertcarr) wrote : Posted in a previous version of this proposal

This is non-critical so I am putting it on hold for a few days so I can focus on the Unity stack.

Revision history for this message
Robert Carr (robertcarr) wrote : Posted in a previous version of this proposal

It returns! With RAII on the event sink. I encountered some intermittent failures (both related, and somewhat unrelated but exposed):

MirConnection::released -> We don't want to remove the surface from valid surfaces until here because we may still be receiving configuration events until the surface is actually released on the server side.

input_testing_server_options: It's not enough to say we are ready for input when the input_registrar returns, because the shell may have not yet granted keyboard focus (fixes 1196744)

MirSurface::release_surface: There was a deadlock going on here:

22:42 < racarr> Client calls mir_surface_release it gets to MirSurface::release_surface which acquires the recursive lock on the MirSurface
22:42 < racarr> then calls MirConnection::release_surface
22:42 < racarr> which trys to acquire the connection lock, but blocks because an RPC thread is
22:42 < racarr> has called MirConnection::handle_event
22:42 < racarr> holding the lock
22:43 < racarr> but the RPC thread while executing MirConnection::handle_event tries to call
22:43 < racarr> MirSurface::handle_event
22:43 < racarr> but the Surface is already locked from the
22:43 < racarr> original client thread

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote : Posted in a previous version of this proposal
review: Needs Fixing (continuous-integration)
Revision history for this message
Alan Griffiths (alan-griffiths) wrote : Posted in a previous version of this proposal

352 + class Impediment
353 + {
354 + public:
355 + ~Impediment()
356 + {
357 + remove_callback();
358 + }
359 + protected:
360 + friend class CloggableEventSink;
361 + Impediment(std::function<void()> const& remove) :
362 + remove_callback(remove)
363 + {
364 + }
365 + private:
366 + std::function<void()> const remove_callback;
367 + };
368 +
369 + std::unique_ptr<Impediment> clog()
370 + {
371 + std::unique_lock<std::mutex> lg(lock);
372 + clogged = true;
373 +
374 + return std::unique_ptr<Impediment>(new Impediment(
375 + std::bind(std::mem_fn(&CloggableEventSink::unclog), this)));
376 + }

This approach seems complicated to me. C.f.

template<class Cloggee>
class Clogger
{
public:
    Clogger(Cloggee& cloggee) : cloggee{cloggee} { cloggee.clog(); }
    ~Clogger() { cloggee.unclog(); }
private:
    Cloggee& cloggee;
    Clogger(Clogger const&) = delete;
    Clogger& operator=(Clogger const&) = delete;
};

Revision history for this message
Alan Griffiths (alan-griffiths) wrote : Posted in a previous version of this proposal

438 === modified file 'src/server/frontend/socket_session.cpp'
445 + // TODO: It might make sense to use a pool for these.
446 + std::vector<char> whole_message;
...
451 === modified file 'src/server/frontend/socket_session.h'
458 - std::vector<char> whole_message;

This shouldn't be needed.

Revision history for this message
Alan Griffiths (alan-griffiths) wrote : Posted in a previous version of this proposal

1321 msh::SingleVisibilityFocusMechanism focus_mechanism(mt::fake_shared(model),
1322 - std::make_shared<mtd::StubInputTargeter>());
1323 + std::make_shared<mtd::StubInputTargeter>());

Changes layout from one of our common styles to one we don't use.

Revision history for this message
Alan Griffiths (alan-griffiths) wrote : Posted in a previous version of this proposal

I'm tempted to suggest -Wno-mismatched-tags to quiet clang's curious insistence that struct X is not a synonym for class X.

I've commented on a couple of oddities above - my biggest concern being that if we really need to move "whole_message" that there is an existing race condition (and that should be fixed instead of hacking around it).

Overall, the MP looks reasonable, but is a bit big for me to feel confident I understand it all.

review: Needs Information
Revision history for this message
Robert Carr (robertcarr) wrote : Posted in a previous version of this proposal

Fixed clang compile.

I don't think the whole_message bit is required, that was introduced by me in an early iteration to attempt to fix a race that ended up being in the SessionMediator as I recall. It's been removed.

Fixed whitespace around 1321.

I'm not sure I like the Clogger class, the way it is now makes it clear how to call clog.

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote : Posted in a previous version of this proposal
review: Approve (continuous-integration)
Revision history for this message
Alan Griffiths (alan-griffiths) wrote : Posted in a previous version of this proposal

I still think that clog()/unclog() are dubious.

What should happen if clog() is called in two threads and then unclog() is called in the first?

Why are you creating a unique_ptr at all? Surely, as you don't want to follow the model used in std (e.g. unique_lock), all that you need to return is a move-only type that calls unclog() in the destructor?

~~~~

234 +protected

line noise

review: Needs Fixing
Revision history for this message
Robert Carr (robertcarr) wrote : Posted in a previous version of this proposal

Improved the clogging API and added exceptions for some exceptional conditions.

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote : Posted in a previous version of this proposal
review: Approve (continuous-integration)
Revision history for this message
Alan Griffiths (alan-griffiths) wrote : Posted in a previous version of this proposal

352 + ~CloggableEventSink()
353 + {
354 + std::unique_lock<std::mutex> lg(lock);
355 +
356 + if (clogged == true)
357 + BOOST_THROW_EXCEPTION(std::logic_error("EventSink destroyed while clogged leading to lost events"));
358 + }

See "C++ Coding Standards" Guideline 51 (this isn't the guideline I consider wrong).

review: Needs Fixing
Revision history for this message
Alan Griffiths (alan-griffiths) wrote : Posted in a previous version of this proposal

606 +};
607 +struct MockEventObserver
608 +{
609 + MOCK_METHOD1(see, void(MirEvent const*));
610 +};
611 +struct EventObservingClient : ClientConfigCommon

A bit of whitespace between classes aids readability

Revision history for this message
Robert Carr (robertcarr) wrote : Posted in a previous version of this proposal

Added some whitespace.

Removed the dtor. I thought it might be ok to throw in this case as it likely indicates a programmer error which should never happen.

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote : Posted in a previous version of this proposal
review: Needs Fixing (continuous-integration)
Revision history for this message
Alan Griffiths (alan-griffiths) wrote : Posted in a previous version of this proposal

On 15/07/13 17:34, Robert Carr wrote:
> Removed the dtor. I thought it might be ok to throw in this case as it likely indicates a programmer error which should never happen.

Better to abort?

--
Alan Griffiths +44 (0)798 9938 758
Octopull Ltd http://www.octopull.co.uk/

Revision history for this message
Alexandros Frantzis (afrantzis) wrote : Posted in a previous version of this proposal

219 +#include <initializer_list>
220 +#include <string>

Are these needed?

> 234 +protected
>
> line noise

+1. Just a nit, but there is no reason to add a protected block just for the deleted CopyAssign operations; we can put them under private.

> See "C++ Coding Standards" Guideline 51 (this isn't the guideline I consider wrong).

This also applies to

401 + ~Clog()
402 + {
403 + sink.unclog();
404 + }

since sink.unclog() can throw.

Revision history for this message
Alexandros Frantzis (afrantzis) : Posted in a previous version of this proposal
review: Needs Fixing
Revision history for this message
Alan Griffiths (alan-griffiths) wrote : Posted in a previous version of this proposal

> On 15/07/13 17:34, Robert Carr wrote:
> > Removed the dtor. I thought it might be ok to throw in this case as it
> likely indicates a programmer error which should never happen.

It is low cost to detect and leads to undefined behavior: just put an assert here.

review: Needs Fixing
Revision history for this message
Robert Carr (robertcarr) wrote : Posted in a previous version of this proposal

assert(asserts_present_in_revision_755)

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote : Posted in a previous version of this proposal
review: Needs Fixing (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote : Posted in a previous version of this proposal
review: Needs Fixing (continuous-integration)
Revision history for this message
Robert Carr (robertcarr) wrote : Posted in a previous version of this proposal

...Merged trunk!

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote : Posted in a previous version of this proposal
review: Needs Fixing (continuous-integration)
Revision history for this message
Robert Carr (robertcarr) wrote : Posted in a previous version of this proposal

Last jenkins failure was just intermittent jenkins infrastructure issue. I would like this to land one day because it conflicts a lot.

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote : Posted in a previous version of this proposal
review: Approve (continuous-integration)
Revision history for this message
Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal

Looks like my review is way out of date now. So no comment.

review: Abstain
Revision history for this message
Kevin DuBois (kdub) wrote : Posted in a previous version of this proposal

I think lp:~kdub/mir/prepare-for-display-notifications will help with this... EventSink now is a bit more decoupled and the interface can be developed against without messing with ProtobufSocketCommunicator at all.

Revision history for this message
Robert Carr (robertcarr) wrote : Posted in a previous version of this proposal

Is there a specific portion of the change that people don't want to land?

Revision history for this message
Alan Griffiths (alan-griffiths) wrote : Posted in a previous version of this proposal

My previous objection is addressed, and I don't have the headspace right now.

recursive mutexes are usually a sign of design issues, but this MP doesn't introduced a new problem.

review: Abstain
Revision history for this message
Alexandros Frantzis (afrantzis) wrote : Posted in a previous version of this proposal

Looks good overall. One question:

486 + if (current_focus)
487 + current_focus->configure(mir_surface_attrib_focus, mir_surface_unfocused);
488 + surface->configure(mir_surface_attrib_focus, mir_surface_focused);.

Is there a way for the currently focused surface to get the focus again, and receive a confusing [unfocused,focused] sequence of events? We don't seem to guard against this, at least not at this level.

review: Approve
Revision history for this message
Alan Griffiths (alan-griffiths) wrote : Posted in a previous version of this proposal

NB this needs to be re-sync'ed with trunk before top-approving.

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote : Posted in a previous version of this proposal
review: Needs Fixing (continuous-integration)
Revision history for this message
Robert Carr (robertcarr) wrote : Posted in a previous version of this proposal

Merged trunk

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote : Posted in a previous version of this proposal
review: Approve (continuous-integration)
Revision history for this message
Alan Griffiths (alan-griffiths) wrote : Posted in a previous version of this proposal

> > just curious...is this some convention ?
> > mir_surface_focus_arraysize_
> > didn't understand why it was in the surf attrib for focusstate
>
> Yes, it is a sentinel element for the array, and its value is equal to the
> array size.

Just curious - was there an array when this was written? (There isn't now, and it seems unlikely there ever was.)

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote : Posted in a previous version of this proposal
review: Needs Fixing (continuous-integration)
Revision history for this message
Robert Carr (robertcarr) wrote : Posted in a previous version of this proposal

The merge conflicts were not entirely trivial so maybe someone wants to give it a once over?

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote : Posted in a previous version of this proposal
review: Approve (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote : Posted in a previous version of this proposal
review: Needs Fixing (continuous-integration)
Revision history for this message
Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal

1. Conflicts...
Text conflict in include/server/mir/shell/default_focus_mechanism.h
Text conflict in src/server/shell/default_focus_mechanism.cpp
Text conflict in tests/unit-tests/shell/test_default_focus_mechanism.cpp
3 conflicts encountered.

2. I think the past tense "sent_event" doesn't feel right, and is a confusing function name despite being similar to those above it. Maybe "on_event_sent".

3. As already mentioned, I still don't think MirSurfaceFocusState should exist at all. Making "focus" an attribute of a surface leads to dangerous and impossible situations:
  (a) Many surfaces focused simultaneously; and
  (b) surf.configure(mir_surface_attrib_focus, X) != X

4. IPCSemaphore: Everyone knows semaphores as having "wait" and "signal" operations. I'd prefer those to the verbose method names you have. But it's not important.

5. Focus/blur should be an event on the connection, not the surface. This would resolve the ambiguities mentioned in 3.

Sorry to keep touching on the same things (3, 5), but I did first raise these concerns 2 months ago. At the time you seem to have agreed saying:
  "I'm not sure of a better way to do this until a bigger focus refactoring happens. This is "next on my list"
I think we should avoid a temporary solution that adds attributes which ultimately we agree should not exist.

review: Needs Fixing
Revision history for this message
Robert Carr (robertcarr) wrote : Posted in a previous version of this proposal

I don't understand what you mean by point 5. We need to deliver focus based on surface, i.e. you have multiple terminal windows open from the same session and they need to know whether or not to blink the cursor.

Revision history for this message
Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal

(5) is really an extension of (3). Given that (3) I don't think focus should be a per-surface attribute then one would conclude (5) that the "currently focused surface" should be stored in a single location, single variable. This makes it an attribute of some other class, like maybe the session.

Revision history for this message
Chris Halse Rogers (raof) wrote : Posted in a previous version of this proposal

High level - I concur with Daniel: having focus being a surface attribute seems dangerous. Current focus is... session local? Hm.

Focus is a per-keyboardish attribute, so it's not strictly speaking unique in a compositor session, but it's pretty close to.

===

Why are we calling the guard value of the enums *_arraysize_? That seems somewhat misleading.

===

Perhaps Clog should take a shared_ptr<CloggableEventSink> rather than a reference? It needs to keep the event sink live while the Clog is live. This is not a problem in the current code, though.

<bikeshed>I'd find "Plug" a more natural name for the Clog class; you stick a plug in your sink to clog it ☺ </bikeshed>

====

I was going to say that lttng::MPR::sent_event should be event_sent to match exception_handled above, but it turns out that above *that* is received_invocation/completed_invocation.

I think event_sent (and invocation_{received,completed}) makes more sense, but there's no consistency there at the moment.

===

812 + ~ProxyShell() noexcept(true) = default;

I *think* the default destructor is noexcept, so the noexcept(true) there is redundant?

===

949 + static int const semaphore_enable_process_shared = 1;

Why is this static? You don't need to track the value of this across entries to IPCSemaphore(), and I presume the compiler will entirely optimise it out.

===

1171 + ~MockEventSink() … {} could be ~MockEventSink() = default; ?

review: Needs Fixing
Revision history for this message
Chris Halse Rogers (raof) wrote : Posted in a previous version of this proposal

On second thoughts, maybe it's ok?

We're not storing the focus state in a surface attribute, we're storing it in the FocusStrategy and projecting that to the client as a surface state.

It's possible that there's a nicer way of projecting that data, but maybe this isn't too bad.

review: Abstain
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote : Posted in a previous version of this proposal

FAILED: Continuous integration, rev:766
No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want a jenkins rebuild you need to trigger it yourself):
https://code.launchpad.net/~robertcarr/mir/client-focus-notifications/+merge/179801/+edit-commit-message

http://jenkins.qa.ubuntu.com/job/mir-ci/1286/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/1716
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/1601
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/524/console

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

review: Needs Fixing (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote : Posted in a previous version of this proposal

FAILED: Continuous integration, rev:767
No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want a jenkins rebuild you need to trigger it yourself):
https://code.launchpad.net/~robertcarr/mir/client-focus-notifications/+merge/179801/+edit-commit-message

http://jenkins.qa.ubuntu.com/job/mir-ci/1289/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/1719
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/1604
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/527/console

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

review: Needs Fixing (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote : Posted in a previous version of this proposal

FAILED: Continuous integration, rev:768
No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want a jenkins rebuild you need to trigger it yourself):
https://code.launchpad.net/~robertcarr/mir/client-focus-notifications/+merge/179801/+edit-commit-message

http://jenkins.qa.ubuntu.com/job/mir-ci/1290/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/1720/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/1605/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/528/console

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

review: Needs Fixing (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote : Posted in a previous version of this proposal
review: Needs Fixing (continuous-integration)
Revision history for this message
Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal

BTW, "enum_max_" is not accurate. It goes one past the max value, used to size fixed arrays. Hence "arraysize_".

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote : Posted in a previous version of this proposal
review: Needs Fixing (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote : Posted in a previous version of this proposal

FAILED: Continuous integration, rev:772
No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want a jenkins rebuild you need to trigger it yourself):
https://code.launchpad.net/~robertcarr/mir/client-focus-notifications/+merge/180011/+edit-commit-message

http://jenkins.qa.ubuntu.com/job/mir-ci/1296/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/1727/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/1612/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/534/console

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

review: Needs Fixing (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote : Posted in a previous version of this proposal
review: Approve (continuous-integration)
Revision history for this message
Alan Griffiths (alan-griffiths) wrote : Posted in a previous version of this proposal

486 -<<<<<<< TREE
487 MediatingDisplayChanger(
488 std::shared_ptr<graphics::Display> const& display,
489 std::shared_ptr<compositor::Compositor> const& compositor,
490 std::shared_ptr<input::InputManager> const& input_manager,
491 std::shared_ptr<graphics::DisplayConfigurationPolicy> const& display_configuration_policy);
492 -=======
493 - explicit MediatingDisplayChanger(
494 - std::shared_ptr<graphics::Display> const& display,
495 - std::shared_ptr<compositor::Compositor> const& compositor);
496 ->>>>>>> MERGE-SOURCE

The diff is a mess. It is not a good idea to have a prerequisite branch (connect-display-request-merge-trunk) that's been rejected.

review: Needs Resubmitting
Revision history for this message
Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal

What Alan said.

review: Needs Resubmitting
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote : Posted in a previous version of this proposal
review: Needs Fixing (continuous-integration)
Revision history for this message
Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal

Would it be correct to assume this might also resolve bug 1212518?

Revision history for this message
Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal

1. Trivial text conflict in tests/unit-tests/client/test_mir_connection.cpp

2. The diff is messed up and seems to remove much of the recent multi-monitor additions. Please resolve that and resubmit.

review: Needs Resubmitting
Revision history for this message
Robert Carr (robertcarr) wrote :

Accidental reverse merge :)

Revision history for this message
Robert Carr (robertcarr) wrote :

Its not showing up in the thread history because of resubmit I guess. Fixed in r 775 (Merge trunk) and r776 (back out accidental revert).

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

FAILED: Continuous integration, rev:777
No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want a jenkins rebuild you need to trigger it yourself):
https://code.launchpad.net/~robertcarr/mir/client-focus-notifications/+merge/180903/+edit-commit-message

http://jenkins.qa.ubuntu.com/job/mir-ci/1329/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/1777
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/1662
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/567
        deb: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/567/artifact/work/output/*zip*/output.zip

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

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
Chris Halse Rogers (raof) wrote :

This looks good to me.

Doesn't break anything, and XMir still works ☺

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

376 -
377 +
...
435 -
436 +

Introduces trailing whitespace

~~~~
437 + // TODO: We rely on done->Run() sending messages synchronously.

What is to be done?

~~~~

I don't see how this fixes Bug #1196744 or Bug #1201435 (but I'm not convinced that either is still a problem anyway).

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

1101 +TEST_F(ShellSurface, focus)

This an uninformative test name. What does:

[ FAILED ] ShellSurface.focus

tell anyone?

What's wrong with "when_focus_gained_and_lost_a_surface_sends_notifications"

review: Needs Fixing
Revision history for this message
Robert Carr (robertcarr) wrote :

Fixed white space.

Updated the comment and added a pair comment in mfd::SocketMessenger::send.

Helped test name.

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
Robert Ancell (robert-ancell) wrote :

LGTM

review: Approve
Revision history for this message
kevin gunn (kgunn72) wrote :

ok, reran on a more well known machine - good results
openarena 800x600
w/ bypass 109.33 w/o bypass 103 fps
openarena 1280 x 800
w/ bypass 58.4 w/o bypass 56 fps

also, rebuilt local bypass - compared to main for egltriangle demo client
w/ bypass 264 fps
w/o bypass 173 fps

review: Approve
Revision history for this message
kevin gunn (kgunn72) wrote :

so sorry, wrong mp

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

Not had time to re-review. Abstaining to avoid blocking

review: Abstain
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
Daniel van Vugt (vanvugt) wrote :

test_client_focus_notification.cpp:125:13: error: missing initializer for member ‘MirSurfaceParameters::output_id’ [-Werror=missing-field-initializers]
             };
             ^

review: Needs Fixing
Revision history for this message
Daniel van Vugt (vanvugt) wrote :

Also, _enum_max_ is not accurate. It's not the max value but one past it. Hence the existing _arraysize_ is more appropriate.

Revision history for this message
Daniel van Vugt (vanvugt) wrote :

Summary:

1. Build failure:
test_client_focus_notification.cpp:125:13: error: missing initializer for member ‘MirSurfaceParameters::output_id’ [-Werror=missing-field-initializers]
             };
             ^

2. _enum_max_ is not accurate. It's not the max value but is in fact one past the max value. Hence the existing _arraysize_ is more appropriate.

3. mir_surface_attrib_focus should not exist; focus is not a per-surface attribute. For reasons I have mentioned several times since June. Focus is per input device (for non-pointing devices: keyboard, voice...). Though could probably be simplified to one focus per session. Something like: SurfaceID Session::focussed_surface() const;

review: Needs Fixing
Revision history for this message
Robert Carr (robertcarr) wrote :

>> 1. Build failure:

Merged trunk

>>2._enum_mx_
mir_surface_attrib_enum_max_ is very literally the maximum valid member of the mir surface attribute enumeration :p. Chris requested this change.

3.
>> mir_surface_attrib_focus should not exist; focus is not a per-surface attribute. For reasons I have mentioned several >> times since June.

You keep saying this but haven't proposed an alternative architecture.

>> Focus is per input device (for non-pointing devices: keyboard, voice...). Though could probably be simplified to >> one focus per session. Something like: SurfaceID Session::focussed_surface() const;

Actually the design docs talk about input focus as a sort of separate concept. There is certainly some notion of a surface having a focused attribute:

1. Clients need to alter their presentation based on whether they are focused, i.e. blinking cursor. It seems like this is in some sense a 'property of the surface', i.e. has some sort of input focus.
2. Which surface does the HUD act on? This has nothing to do with pointing devices.
3. Which surface does the global menu bar display from?

I think it's likely that focus will require more refinement for some multi window, multi input device, multi monitor, etc scenarios on the desktop. So far, the design documents have talked more about a hierarchy of focus though than modelling focus only as per input device. I think these requirements are unclear and trying to over generalize the system now is fruitless. On the other hand the requirements for the phone and system compositor are very clear and satisfied here.

782. By Robert Carr

Merge trunk

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
Daniel van Vugt (vanvugt) wrote :

OK, I've argued enough. I don't think this is the right answer, but of course don't have an alternative branch implemented either. Someone else can veto this one.

Revision history for this message
kevin gunn (kgunn72) wrote :

ok, we're definitely to philosophical debates

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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'include/server/mir/frontend/message_processor_report.h'
2--- include/server/mir/frontend/message_processor_report.h 2013-04-24 05:22:20 +0000
3+++ include/server/mir/frontend/message_processor_report.h 2013-08-22 22:52:58 +0000
4@@ -20,6 +20,8 @@
5 #ifndef MIR_FRONTEND_MESSAGE_PROCESSOR_REPORT_H_
6 #define MIR_FRONTEND_MESSAGE_PROCESSOR_REPORT_H_
7
8+#include "mir_toolkit/event.h"
9+
10 #include <string>
11
12 namespace mir
13@@ -42,6 +44,8 @@
14 virtual void exception_handled(void const* mediator, int id, std::exception const& error) = 0;
15
16 virtual void exception_handled(void const* mediator, std::exception const& error) = 0;
17+
18+ virtual void sent_event(void const* mediator, MirSurfaceEvent const& ev) = 0;
19
20 private:
21 MessageProcessorReport(MessageProcessorReport const&) = delete;
22
23=== modified file 'include/server/mir/frontend/null_message_processor_report.h'
24--- include/server/mir/frontend/null_message_processor_report.h 2013-04-24 05:22:20 +0000
25+++ include/server/mir/frontend/null_message_processor_report.h 2013-08-22 22:52:58 +0000
26@@ -36,6 +36,8 @@
27 void exception_handled(void const*, int, std::exception const&);
28
29 void exception_handled(void const*, std::exception const&);
30+
31+ void sent_event(void const*, MirSurfaceEvent const& e);
32 };
33 }
34 }
35
36=== modified file 'include/server/mir/frontend/shell.h'
37--- include/server/mir/frontend/shell.h 2013-08-01 07:44:17 +0000
38+++ include/server/mir/frontend/shell.h 2013-08-22 22:52:58 +0000
39@@ -44,6 +44,8 @@
40
41 virtual SurfaceId create_surface_for(std::shared_ptr<Session> const& session,
42 shell::SurfaceCreationParameters const& params) = 0;
43+
44+ virtual void handle_surface_created(std::shared_ptr<Session> const& session) = 0;
45
46 protected:
47 Shell() = default;
48
49=== modified file 'include/server/mir/logging/message_processor_report.h'
50--- include/server/mir/logging/message_processor_report.h 2013-04-24 05:22:20 +0000
51+++ include/server/mir/logging/message_processor_report.h 2013-08-22 22:52:58 +0000
52@@ -68,6 +68,8 @@
53
54 void exception_handled(void const* mediator, std::exception const& error);
55
56+ void sent_event(void const* mediator, MirSurfaceEvent const& event);
57+
58 ~MessageProcessorReport() noexcept(true);
59
60 private:
61
62=== modified file 'include/server/mir/lttng/message_processor_report.h'
63--- include/server/mir/lttng/message_processor_report.h 2013-06-03 12:15:44 +0000
64+++ include/server/mir/lttng/message_processor_report.h 2013-08-22 22:52:58 +0000
65@@ -35,6 +35,7 @@
66 void unknown_method(void const* mediator, int id, std::string const& method);
67 void exception_handled(void const* mediator, int id, std::exception const& error);
68 void exception_handled(void const* mediator, std::exception const& error);
69+ void sent_event(void const* mediator, MirSurfaceEvent const& event);
70
71 private:
72 ServerTracepointProvider tp_provider;
73
74=== modified file 'include/server/mir/shell/default_focus_mechanism.h'
75--- include/server/mir/shell/default_focus_mechanism.h 2013-08-02 04:07:15 +0000
76+++ include/server/mir/shell/default_focus_mechanism.h 2013-08-22 22:52:58 +0000
77@@ -19,14 +19,17 @@
78 #ifndef MIR_SHELL_SINGLE_VISIBILITY_FOCUS_MECHANISM_H_
79 #define MIR_SHELL_SINGLE_VISIBILITY_FOCUS_MECHANISM_H_
80
81+#include "mir/shell/focus_setter.h"
82+
83 #include <memory>
84-#include "mir/shell/focus_setter.h"
85+#include <mutex>
86
87 namespace mir
88 {
89
90 namespace shell
91 {
92+class Surface;
93 class InputTargeter;
94 class SurfaceController;
95
96@@ -46,6 +49,9 @@
97 private:
98 std::shared_ptr<InputTargeter> const input_targeter;
99 std::shared_ptr<SurfaceController> const surface_controller;
100+
101+ std::mutex surface_focus_lock;
102+ std::weak_ptr<Surface> currently_focused_surface;
103 };
104
105 }
106
107=== modified file 'include/server/mir/shell/session_manager.h'
108--- include/server/mir/shell/session_manager.h 2013-08-15 09:15:39 +0000
109+++ include/server/mir/shell/session_manager.h 2013-08-22 22:52:58 +0000
110@@ -70,6 +70,8 @@
111 void focus_next();
112 std::weak_ptr<Session> focussed_application() const;
113 void set_focus_to(std::shared_ptr<Session> const& focus);
114+
115+ void handle_surface_created(std::shared_ptr<frontend::Session> const& session);
116
117 protected:
118 SessionManager(const SessionManager&) = delete;
119
120=== modified file 'include/shared/mir_toolkit/common.h'
121--- include/shared/mir_toolkit/common.h 2013-07-09 10:51:59 +0000
122+++ include/shared/mir_toolkit/common.h 2013-08-22 22:52:58 +0000
123@@ -36,7 +36,8 @@
124 mir_surface_attrib_type,
125 mir_surface_attrib_state,
126 mir_surface_attrib_swapinterval,
127- mir_surface_attrib_arraysize_
128+ mir_surface_attrib_focus,
129+ mir_surface_attrib_enum_max_
130 } MirSurfaceAttrib;
131
132 typedef enum MirSurfaceType
133@@ -48,7 +49,7 @@
134 mir_surface_type_freestyle,
135 mir_surface_type_popover,
136 mir_surface_type_inputmethod,
137- mir_surface_type_arraysize_
138+ mir_surface_type_enum_max_
139 } MirSurfaceType;
140
141 typedef enum MirSurfaceState
142@@ -62,8 +63,16 @@
143 Omitted for now, since it's functionally a subset of vertmaximized and
144 differs only in the X coordinate. */
145 mir_surface_state_fullscreen,
146- mir_surface_state_arraysize_
147+ mir_surface_state_enum_max_
148 } MirSurfaceState;
149+
150+typedef enum MirSurfaceFocusState
151+{
152+ mir_surface_unfocused = 0,
153+ mir_surface_focused,
154+ mir_surface_focus_enum_max_
155+} MirSurfaceFocusState;
156+
157 /**@}*/
158
159 #endif
160
161=== modified file 'include/test/mir_test/event_matchers.h'
162--- include/test/mir_test/event_matchers.h 2013-04-26 16:31:48 +0000
163+++ include/test/mir_test/event_matchers.h 2013-08-22 22:52:58 +0000
164@@ -19,6 +19,8 @@
165 #ifndef MIR_TEST_EVENT_MATCHERS_H_
166 #define MIR_TEST_EVENT_MATCHERS_H_
167
168+#include "mir_toolkit/event.h"
169+
170 #include <androidfw/Input.h>
171
172 #include <gmock/gmock.h>
173@@ -64,6 +66,18 @@
174 return (coords->x == dx) && (coords->y == dy);
175 }
176
177+MATCHER_P2(SurfaceEvent, attrib, value, "")
178+{
179+ if (arg.type != mir_event_type_surface)
180+ return false;
181+ auto surface_ev = arg.surface;
182+ if (surface_ev.attrib != attrib)
183+ return false;
184+ if (surface_ev.value != value)
185+ return false;
186+ return true;
187+}
188+
189 }
190 } // namespace mir
191
192
193=== modified file 'include/test/mir_test_doubles/mock_shell.h'
194--- include/test/mir_test_doubles/mock_shell.h 2013-08-01 07:44:17 +0000
195+++ include/test/mir_test_doubles/mock_shell.h 2013-08-22 22:52:58 +0000
196@@ -39,6 +39,7 @@
197 MOCK_METHOD1(close_session, void(std::shared_ptr<frontend::Session> const&));
198
199 MOCK_METHOD2(create_surface_for, frontend::SurfaceId(std::shared_ptr<frontend::Session> const&, shell::SurfaceCreationParameters const&));
200+ MOCK_METHOD1(handle_surface_created, void(std::shared_ptr<frontend::Session> const&));
201 };
202
203 }
204
205=== modified file 'include/test/mir_test_doubles/stub_shell.h'
206--- include/test/mir_test_doubles/stub_shell.h 2013-08-01 07:44:17 +0000
207+++ include/test/mir_test_doubles/stub_shell.h 2013-08-22 22:52:58 +0000
208@@ -39,10 +39,13 @@
209 {
210 }
211 frontend::SurfaceId create_surface_for(std::shared_ptr<frontend::Session> const& /* session */,
212- shell::SurfaceCreationParameters const& /* params */)
213+ shell::SurfaceCreationParameters const& /* params */) override
214 {
215 return frontend::SurfaceId{0};
216 }
217+ void handle_surface_created(std::shared_ptr<frontend::Session> const& /* session */) override
218+ {
219+ }
220 };
221
222 }
223
224=== modified file 'include/test/mir_test_framework/input_testing_server_configuration.h'
225--- include/test/mir_test_framework/input_testing_server_configuration.h 2013-07-24 05:22:03 +0000
226+++ include/test/mir_test_framework/input_testing_server_configuration.h 2013-08-22 22:52:58 +0000
227@@ -63,14 +63,13 @@
228 void on_exit();
229
230 std::shared_ptr<mir::input::InputConfiguration> the_input_configuration() override;
231- std::shared_ptr<mir::surfaces::InputRegistrar> the_input_registrar() override;
232+ std::shared_ptr<mir::frontend::Shell> the_frontend_shell() override;
233
234 protected:
235 virtual void inject_input() = 0;
236 mir::input::android::FakeEventHub* fake_event_hub;
237
238 void wait_until_client_appears(std::string const& surface_name);
239- void wait_until_client_vanishes(std::string const& surface_name);
240
241 private:
242 std::mutex lifecycle_lock;
243@@ -81,7 +80,7 @@
244 std::thread input_injection_thread;
245
246 std::shared_ptr<mir::test::doubles::FakeEventHubInputConfiguration> input_configuration;
247- std::shared_ptr<mir::surfaces::InputRegistrar> input_registrar;
248+ std::shared_ptr<mir::frontend::Shell> frontend_shell;
249 };
250
251 }
252
253=== modified file 'src/client/mir_connection.cpp'
254--- src/client/mir_connection.cpp 2013-08-20 13:43:06 +0000
255+++ src/client/mir_connection.cpp 2013-08-22 22:52:58 +0000
256@@ -146,6 +146,10 @@
257
258 void MirConnection::released(SurfaceRelease data)
259 {
260+ {
261+ std::lock_guard<std::recursive_mutex> lock(mutex);
262+ surface_map->erase(data.surface->id());
263+ }
264 data.callback(data.surface, data.context);
265 data.handle->result_received();
266 delete data.surface;
267@@ -161,7 +165,6 @@
268 auto new_wait_handle = new MirWaitHandle;
269
270 SurfaceRelease surf_release{surface, new_wait_handle, callback, context};
271- surface_map->erase(surface->id());
272
273 mir::protobuf::SurfaceId message;
274 message.set_value(surface->id());
275
276=== modified file 'src/client/mir_surface.cpp'
277--- src/client/mir_surface.cpp 2013-08-20 13:39:31 +0000
278+++ src/client/mir_surface.cpp 2013-08-22 22:52:58 +0000
279@@ -55,7 +55,7 @@
280
281 server.create_surface(0, &message, &surface, gp::NewCallback(this, &MirSurface::created, callback, context));
282
283- for (int i = 0; i < mir_surface_attrib_arraysize_; i++)
284+ for (int i = 0; i < mir_surface_attrib_enum_max_; i++)
285 attrib_cache[i] = -1;
286 attrib_cache[mir_surface_attrib_type] = mir_surface_type_normal;
287 attrib_cache[mir_surface_attrib_state] = mir_surface_state_unknown;
288@@ -223,8 +223,6 @@
289 mir_surface_callback callback,
290 void * context)
291 {
292- std::lock_guard<std::recursive_mutex> lock(mutex);
293-
294 return connection->release_surface(this, callback, context);
295 }
296
297@@ -316,6 +314,7 @@
298 {
299 case mir_surface_attrib_type:
300 case mir_surface_attrib_state:
301+ case mir_surface_attrib_focus:
302 case mir_surface_attrib_swapinterval:
303 if (configure_result.has_ivalue())
304 attrib_cache[a] = configure_result.ivalue();
305@@ -371,7 +370,7 @@
306 if (e.type == mir_event_type_surface)
307 {
308 MirSurfaceAttrib a = e.surface.attrib;
309- if (a < mir_surface_attrib_arraysize_)
310+ if (a < mir_surface_attrib_enum_max_)
311 attrib_cache[a] = e.surface.value;
312 }
313
314
315=== modified file 'src/client/mir_surface.h'
316--- src/client/mir_surface.h 2013-08-13 10:27:45 +0000
317+++ src/client/mir_surface.h 2013-08-22 22:52:58 +0000
318@@ -123,7 +123,7 @@
319 mir::protobuf::SurfaceSetting configure_result;
320
321 // Cache of latest SurfaceSettings returned from the server
322- int attrib_cache[mir_surface_attrib_arraysize_];
323+ int attrib_cache[mir_surface_attrib_enum_max_];
324
325 std::function<void(MirEvent const*)> handle_event_callback;
326 std::shared_ptr<mir::input::receiver::InputReceiverThread> input_thread;
327
328=== modified file 'src/server/frontend/null_message_processor_report.cpp'
329--- src/server/frontend/null_message_processor_report.cpp 2013-04-24 05:22:20 +0000
330+++ src/server/frontend/null_message_processor_report.cpp 2013-08-22 22:52:58 +0000
331@@ -39,3 +39,7 @@
332 void mf::NullMessageProcessorReport::exception_handled(void const*, std::exception const&)
333 {
334 }
335+
336+void mf::NullMessageProcessorReport::sent_event(void const*, MirSurfaceEvent const&)
337+{
338+}
339
340=== modified file 'src/server/frontend/session_mediator.cpp'
341--- src/server/frontend/session_mediator.cpp 2013-08-20 13:39:31 +0000
342+++ src/server/frontend/session_mediator.cpp 2013-08-22 22:52:58 +0000
343@@ -35,17 +35,22 @@
344 #include "mir/graphics/display_configuration.h"
345 #include "mir/graphics/platform_ipc_package.h"
346 #include "mir/frontend/client_constants.h"
347+#include "mir/frontend/event_sink.h"
348+
349 #include "mir/geometry/rectangles.h"
350 #include "client_buffer_tracker.h"
351 #include "protobuf_buffer_packer.h"
352
353 #include <boost/throw_exception.hpp>
354
355+#include <mutex>
356+#include <functional>
357+
358 namespace msh = mir::shell;
359 namespace mf = mir::frontend;
360-namespace mfd = mir::frontend::detail;
361+namespace mfd=mir::frontend::detail;
362+namespace mg = mir::graphics;
363 namespace geom = mir::geometry;
364-namespace mg = mir::graphics;
365
366 mf::SessionMediator::SessionMediator(
367 std::shared_ptr<frontend::Shell> const& shell,
368@@ -117,10 +122,10 @@
369
370 if (session.get() == nullptr)
371 BOOST_THROW_EXCEPTION(std::logic_error("Invalid application session"));
372-
373+
374 report->session_create_surface_called(session->name());
375
376- auto const id = shell->create_surface_for(session,
377+ auto const id = session->create_surface(
378 msh::SurfaceCreationParameters()
379 .of_name(request->surface_name())
380 .of_size(request->width(), request->height())
381@@ -128,34 +133,34 @@
382 .of_pixel_format(static_cast<geometry::PixelFormat>(request->pixel_format()))
383 .with_output_id(graphics::DisplayConfigurationOutputId(request->output_id()))
384 );
385-
386- {
387- auto surface = session->get_surface(id);
388- response->mutable_id()->set_value(id.as_value());
389- response->set_width(surface->size().width.as_uint32_t());
390- response->set_height(surface->size().height.as_uint32_t());
391- response->set_pixel_format((int)surface->pixel_format());
392- response->set_buffer_usage(request->buffer_usage());
393-
394- if (surface->supports_input())
395- response->add_fd(surface->client_input_fd());
396-
397- client_buffer_resource = surface->advance_client_buffer();
398- auto const& id = client_buffer_resource->id();
399-
400- auto buffer = response->mutable_buffer();
401- buffer->set_buffer_id(id.as_uint32_t());
402-
403- if (!client_tracker->client_has(id))
404- {
405- auto packer = std::make_shared<mfd::ProtobufBufferPacker>(buffer);
406- graphics_platform->fill_ipc_package(packer, client_buffer_resource);
407- }
408- client_tracker->add(id);
409- }
410+ {
411+ auto surface = session->get_surface(id);
412+ response->mutable_id()->set_value(id.as_value());
413+ response->set_width(surface->size().width.as_uint32_t());
414+ response->set_height(surface->size().height.as_uint32_t());
415+ response->set_pixel_format((int)surface->pixel_format());
416+ response->set_buffer_usage(request->buffer_usage());
417+ if (surface->supports_input())
418+ response->add_fd(surface->client_input_fd());
419+ client_buffer_resource = surface->advance_client_buffer();
420+ auto const& id = client_buffer_resource->id();
421+ auto buffer = response->mutable_buffer();
422+ buffer->set_buffer_id(id.as_uint32_t());
423+ if (!client_tracker->client_has(id))
424+ {
425+ auto packer = std::make_shared<mfd::ProtobufBufferPacker>(buffer);
426+ graphics_platform->fill_ipc_package(packer, client_buffer_resource);
427+ }
428+ client_tracker->add(id);
429+ }
430 }
431
432+ // TODO: NOTE: We use the ordering here to ensure the shell acts on the surface after the surface ID is sent over the wire.
433+ // This guarantees that notifications such as, gained focus, etc, can be correctly interpreted by the client.
434+ // To achieve this order we rely on done->Run() sending messages synchronously. As documented in mfd::SocketMessenger::send.
435+ // this will require additional synchronization if mfd::SocketMessenger::send changes.
436 done->Run();
437+ shell->handle_surface_created(session);
438 }
439
440 void mf::SessionMediator::next_buffer(
441@@ -209,6 +214,7 @@
442 session->destroy_surface(id);
443 }
444
445+ // TODO: We rely on this sending responses synchronously.
446 done->Run();
447 }
448
449
450=== modified file 'src/server/frontend/socket_messenger.cpp'
451--- src/server/frontend/socket_messenger.cpp 2013-08-01 09:55:02 +0000
452+++ src/server/frontend/socket_messenger.cpp 2013-08-22 22:52:58 +0000
453@@ -57,6 +57,8 @@
454 // TODO: This should be asynchronous, but we are not making sure
455 // that a potential call to send_fds is executed _after_ this
456 // function has completed (if it would be executed asynchronously.
457+ // NOTE: we rely on this synchronous behavior as per the comment in
458+ // mf::SessionMediator::create_surface
459 ba::write(*socket, ba::buffer(whole_message));
460 }
461
462
463=== modified file 'src/server/logging/message_processor_report.cpp'
464--- src/server/logging/message_processor_report.cpp 2013-04-24 05:22:20 +0000
465+++ src/server/logging/message_processor_report.cpp 2013-08-22 22:52:58 +0000
466@@ -162,6 +162,9 @@
467 mediators.erase(mediator);
468 }
469
470-
471-
472-
473+void ml::MessageProcessorReport::sent_event(void const* mediator, MirSurfaceEvent const& event)
474+{
475+ std::ostringstream out;
476+ out << "mediator=" << mediator << ", sent event, surface id=" << event.id;
477+ log->log<Logger::debug>(out.str(), component);
478+}
479
480=== modified file 'src/server/lttng/message_processor_report.cpp'
481--- src/server/lttng/message_processor_report.cpp 2013-06-03 12:15:44 +0000
482+++ src/server/lttng/message_processor_report.cpp 2013-08-22 22:52:58 +0000
483@@ -49,3 +49,9 @@
484 void const* /*mediator*/, std::exception const& /*error*/)
485 {
486 }
487+
488+void mir::lttng::MessageProcessorReport::sent_event(
489+ void const* /*mediator*/, MirSurfaceEvent const&)
490+{
491+ // TODO
492+}
493
494=== modified file 'src/server/shell/default_focus_mechanism.cpp'
495--- src/server/shell/default_focus_mechanism.cpp 2013-08-02 04:07:15 +0000
496+++ src/server/shell/default_focus_mechanism.cpp 2013-08-22 22:52:58 +0000
497@@ -45,6 +45,13 @@
498 auto surface = focus_session->default_surface();
499 if (surface)
500 {
501+ std::lock_guard<std::mutex> lg(surface_focus_lock);
502+ auto current_focus = currently_focused_surface.lock();
503+ if (current_focus)
504+ current_focus->configure(mir_surface_attrib_focus, mir_surface_unfocused);
505+ surface->configure(mir_surface_attrib_focus, mir_surface_focused);
506+ currently_focused_surface = surface;
507+
508 surface->raise(surface_controller);
509 surface->take_input_focus(input_targeter);
510 }
511
512=== modified file 'src/server/shell/session_manager.cpp'
513--- src/server/shell/session_manager.cpp 2013-08-15 09:15:39 +0000
514+++ src/server/shell/session_manager.cpp 2013-08-22 22:52:58 +0000
515@@ -150,13 +150,24 @@
516 return focus_application;
517 }
518
519+// TODO: We use this to work around the lack of a SessionMediator-like object for internal clients.
520+// we could have an internal client mediator which acts as a factory for internal clients, taking responsibility
521+// for invoking handle_surface_created.
522 mf::SurfaceId msh::SessionManager::create_surface_for(std::shared_ptr<mf::Session> const& session,
523 msh::SurfaceCreationParameters const& params)
524 {
525 auto shell_session = std::dynamic_pointer_cast<Session>(session);
526 auto id = shell_session->create_surface(params);
527+
528+ handle_surface_created(session);
529+
530+ return id;
531+}
532+
533+void msh::SessionManager::handle_surface_created(std::shared_ptr<mf::Session> const& session)
534+{
535+ auto shell_session = std::dynamic_pointer_cast<Session>(session);
536
537 set_focus_to(shell_session);
538-
539- return id;
540 }
541+
542
543=== modified file 'src/server/shell/surface.cpp'
544--- src/server/shell/surface.cpp 2013-08-09 03:39:35 +0000
545+++ src/server/shell/surface.cpp 2013-08-22 22:52:58 +0000
546@@ -242,6 +242,9 @@
547 BOOST_THROW_EXCEPTION(std::logic_error("Invalid surface state."));
548 result = state();
549 break;
550+ case mir_surface_attrib_focus:
551+ notify_change(attrib, value);
552+ break;
553 case mir_surface_attrib_swapinterval:
554 allow_dropping = (value == 0);
555 allow_framedropping(allow_dropping);
556@@ -267,7 +270,7 @@
557 {
558 bool valid = false;
559
560- if (t >= 0 && t < mir_surface_type_arraysize_)
561+ if (t >= 0 && t < mir_surface_type_enum_max_)
562 {
563 type_value = t;
564 valid = true;
565@@ -286,7 +289,7 @@
566 bool valid = false;
567
568 if (s > mir_surface_state_unknown &&
569- s < mir_surface_state_arraysize_)
570+ s < mir_surface_state_enum_max_)
571 {
572 state_value = s;
573 valid = true;
574
575=== modified file 'tests/acceptance-tests/CMakeLists.txt'
576--- tests/acceptance-tests/CMakeLists.txt 2013-08-20 13:39:31 +0000
577+++ tests/acceptance-tests/CMakeLists.txt 2013-08-22 22:52:58 +0000
578@@ -10,6 +10,7 @@
579 test_test_framework.cpp
580 test_focus_selection.cpp
581 test_server_shutdown.cpp
582+ test_client_focus_notification.cpp
583 test_client_authorization.cpp
584 test_shell_control_of_surface_configuration.cpp
585 test_nested_mir.cpp
586
587=== added file 'tests/acceptance-tests/test_client_focus_notification.cpp'
588--- tests/acceptance-tests/test_client_focus_notification.cpp 1970-01-01 00:00:00 +0000
589+++ tests/acceptance-tests/test_client_focus_notification.cpp 2013-08-22 22:52:58 +0000
590@@ -0,0 +1,232 @@
591+/*
592+ * Copyright © 2013 Canonical Ltd.
593+ *
594+ * This program is free software: you can redistribute it and/or modify
595+ * it under the terms of the GNU General Public License version 3 as
596+ * published by the Free Software Foundation.
597+ *
598+ * This program is distributed in the hope that it will be useful,
599+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
600+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
601+ * GNU General Public License for more details.
602+ *
603+ * You should have received a copy of the GNU General Public License
604+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
605+ *
606+ * Authored by: Robert Carr <robert.carr@canonical.com>
607+ */
608+
609+#include "mir_toolkit/mir_client_library.h"
610+
611+#include "mir_test/wait_condition.h"
612+#include "mir_test/event_matchers.h"
613+
614+#include "mir_test_framework/display_server_test_fixture.h"
615+#include "mir_test_framework/cross_process_sync.h"
616+
617+#include <gtest/gtest.h>
618+#include <gmock/gmock.h>
619+
620+namespace mt = mir::test;
621+namespace mtf = mir_test_framework;
622+
623+namespace
624+{
625+ char const* const mir_test_socket = mtf::test_socket_file().c_str();
626+}
627+
628+namespace
629+{
630+struct ClientConfigCommon : TestingClientConfiguration
631+{
632+ ClientConfigCommon() :
633+ connection(0),
634+ surface(0)
635+ {
636+ }
637+ static void connection_callback(MirConnection* connection, void* context)
638+ {
639+ ClientConfigCommon* config = reinterpret_cast<ClientConfigCommon *>(context);
640+ config->connection = connection;
641+ }
642+ static void create_surface_callback(MirSurface* surface, void* context)
643+ {
644+ ClientConfigCommon* config = reinterpret_cast<ClientConfigCommon *>(context);
645+ config->surface_created(surface);
646+ }
647+ static void release_surface_callback(MirSurface* surface, void* context)
648+ {
649+ ClientConfigCommon* config = reinterpret_cast<ClientConfigCommon *>(context);
650+ config->surface_released(surface);
651+ }
652+ virtual void connected(MirConnection* new_connection)
653+ {
654+ connection = new_connection;
655+ }
656+ virtual void surface_created(MirSurface* new_surface)
657+ {
658+ surface = new_surface;
659+ }
660+ virtual void surface_released(MirSurface* /* released_surface */)
661+ {
662+ surface = nullptr;
663+ }
664+ MirConnection* connection;
665+ MirSurface* surface;
666+};
667+
668+struct MockEventObserver
669+{
670+ MOCK_METHOD1(see, void(MirEvent const*));
671+};
672+
673+struct EventObservingClient : ClientConfigCommon
674+{
675+ EventObservingClient()
676+ : observer(std::make_shared<MockEventObserver>())
677+ {
678+ }
679+
680+ static void handle_event(MirSurface* /* surface */, MirEvent const* ev, void* context)
681+ {
682+ auto client = static_cast<EventObservingClient *>(context);
683+ client->observer->see(ev);
684+ }
685+
686+ virtual void expect_events(mt::WaitCondition* /* all_events_received */) = 0;
687+
688+ void surface_created(MirSurface *surface_) override
689+ {
690+ // We need to set the event delegate from the surface_created
691+ // callback so we can block the reading of new events
692+ // until we are ready
693+ MirEventDelegate const event_delegate =
694+ {
695+ handle_event,
696+ this
697+ };
698+ surface = surface_;
699+ mir_surface_set_event_handler(surface, &event_delegate);
700+ }
701+
702+ void exec()
703+ {
704+ mt::WaitCondition all_events_received;
705+ expect_events(&all_events_received);
706+ mir_wait_for(mir_connect(mir_test_socket,
707+ __PRETTY_FUNCTION__, connection_callback, this));
708+ ASSERT_TRUE(connection != NULL);
709+ MirSurfaceParameters const request_params =
710+ {
711+ __PRETTY_FUNCTION__,
712+ surface_width, surface_height,
713+ mir_pixel_format_abgr_8888,
714+ mir_buffer_usage_hardware,
715+ mir_display_output_id_invalid
716+ };
717+ mir_wait_for(mir_connection_create_surface(connection, &request_params, create_surface_callback, this));
718+ all_events_received.wait_for_at_most_seconds(60);
719+ mir_surface_release_sync(surface);
720+ mir_connection_release(connection);
721+
722+ // The ClientConfig is not destroyed before the testing process
723+ // exits.
724+ observer.reset();
725+ }
726+ std::shared_ptr<MockEventObserver> observer;
727+ static int const surface_width = 100;
728+ static int const surface_height = 100;
729+};
730+
731+}
732+
733+TEST_F(BespokeDisplayServerTestFixture, a_surface_is_notified_of_receiving_focus)
734+{
735+ using namespace ::testing;
736+
737+ TestingServerConfiguration server_config;
738+ launch_server_process(server_config);
739+
740+ struct FocusObservingClient : public EventObservingClient
741+ {
742+ void expect_events(mt::WaitCondition* all_events_received) override
743+ {
744+ EXPECT_CALL(*observer, see(Pointee(mt::SurfaceEvent(mir_surface_attrib_focus, mir_surface_focused)))).Times(1)
745+ .WillOnce(mt::WakeUp(all_events_received));
746+ }
747+ } client_config;
748+ launch_client_process(client_config);
749+}
750+
751+namespace
752+{
753+
754+ACTION_P(SignalFence, fence)
755+{
756+ fence->try_signal_ready_for();
757+}
758+
759+}
760+
761+TEST_F(BespokeDisplayServerTestFixture, two_surfaces_are_notified_of_gaining_and_losing_focus)
762+{
763+ using namespace ::testing;
764+
765+ TestingServerConfiguration server_config;
766+ launch_server_process(server_config);
767+
768+ // We use this for synchronization to ensure the two clients
769+ // are launched in a defined order.
770+ mtf::CrossProcessSync ready_for_second_client;
771+
772+ struct FocusObservingClientOne : public EventObservingClient
773+ {
774+ mtf::CrossProcessSync ready_for_second_client;
775+ FocusObservingClientOne(mtf::CrossProcessSync const& ready_for_second_client)
776+ : ready_for_second_client(ready_for_second_client)
777+ {
778+ }
779+
780+ void expect_events(mt::WaitCondition* all_events_received) override
781+ {
782+ InSequence seq;
783+ // We should receive focus as we are created
784+ EXPECT_CALL(*observer, see(Pointee(mt::SurfaceEvent(mir_surface_attrib_focus,
785+ mir_surface_focused)))).Times(1)
786+ .WillOnce(SignalFence(&ready_for_second_client));
787+
788+ // And lose it as the second surface is created
789+ EXPECT_CALL(*observer, see(Pointee(mt::SurfaceEvent(mir_surface_attrib_focus,
790+ mir_surface_unfocused)))).Times(1);
791+ // And regain it when the second surface is closed
792+ EXPECT_CALL(*observer, see(Pointee(mt::SurfaceEvent(mir_surface_attrib_focus,
793+ mir_surface_focused)))).Times(1).WillOnce(mt::WakeUp(all_events_received));
794+ }
795+
796+ } client_one_config(ready_for_second_client);
797+ launch_client_process(client_one_config);
798+
799+ struct FocusObservingClientTwo : public EventObservingClient
800+ {
801+ mtf::CrossProcessSync ready_for_second_client;
802+ FocusObservingClientTwo(mtf::CrossProcessSync const& ready_for_second_client)
803+ : ready_for_second_client(ready_for_second_client)
804+ {
805+ }
806+ void exec() override
807+ {
808+ // We need some synchronization to ensure client two does not connect before client one.
809+ ready_for_second_client.wait_for_signal_ready_for();
810+ EventObservingClient::exec();
811+ }
812+
813+ void expect_events(mt::WaitCondition* all_events_received) override
814+ {
815+ EXPECT_CALL(*observer, see(Pointee(
816+ mt::SurfaceEvent(mir_surface_attrib_focus, mir_surface_focused))))
817+ .Times(1).WillOnce(mt::WakeUp(all_events_received));
818+ }
819+ } client_two_config(ready_for_second_client);
820+
821+ launch_client_process(client_two_config);
822+}
823
824=== modified file 'tests/acceptance-tests/test_client_input.cpp'
825--- tests/acceptance-tests/test_client_input.cpp 2013-08-20 13:39:31 +0000
826+++ tests/acceptance-tests/test_client_input.cpp 2013-08-22 22:52:58 +0000
827@@ -154,8 +154,10 @@
828
829 static void handle_input(MirSurface* /* surface */, MirEvent const* ev, void* context)
830 {
831+ if (ev->type == mir_event_type_surface)
832+ return;
833+
834 auto client = static_cast<InputClient *>(context);
835-
836 client->handler->handle_input(ev);
837 }
838
839
840=== modified file 'tests/acceptance-tests/test_focus_selection.cpp'
841--- tests/acceptance-tests/test_focus_selection.cpp 2013-08-20 13:39:31 +0000
842+++ tests/acceptance-tests/test_focus_selection.cpp 2013-08-22 22:52:58 +0000
843@@ -38,6 +38,7 @@
844 namespace mi = mir::input;
845 namespace mt = mir::test;
846 namespace mtd = mt::doubles;
847+namespace mt = mir::test;
848 namespace mtf = mir_test_framework;
849
850 namespace
851
852=== modified file 'tests/acceptance-tests/test_shell_control_of_surface_configuration.cpp'
853--- tests/acceptance-tests/test_shell_control_of_surface_configuration.cpp 2013-08-20 13:39:31 +0000
854+++ tests/acceptance-tests/test_shell_control_of_surface_configuration.cpp 2013-08-22 22:52:58 +0000
855@@ -82,6 +82,9 @@
856 void exec() override
857 {
858 using namespace ::testing;
859+
860+ EXPECT_CALL(mock_configurator, select_attribute_value(_, mir_surface_attrib_focus, _)).Times(AnyNumber());
861+ EXPECT_CALL(mock_configurator, attribute_set(_, mir_surface_attrib_focus, _)).Times(AnyNumber());
862
863 ON_CALL(mock_configurator, select_attribute_value(_, _, _)).WillByDefault(Return(mir_surface_type_freestyle));
864 EXPECT_CALL(mock_configurator, select_attribute_value(_, mir_surface_attrib_type, Eq(mir_surface_type_freestyle))).Times(1);
865@@ -117,6 +120,10 @@
866 void exec() override
867 {
868 using namespace ::testing;
869+
870+ EXPECT_CALL(mock_configurator, select_attribute_value(_, mir_surface_attrib_focus, _)).Times(AnyNumber());
871+ EXPECT_CALL(mock_configurator, attribute_set(_, mir_surface_attrib_focus, _)).Times(AnyNumber());
872+
873 EXPECT_CALL(mock_configurator, select_attribute_value(_, mir_surface_attrib_type, Eq(mir_surface_type_freestyle))).Times(1)
874 .WillOnce(Return(mir_surface_type_normal));
875 EXPECT_CALL(mock_configurator, attribute_set(_, mir_surface_attrib_type, Eq(mir_surface_type_normal))).Times(1);
876
877=== modified file 'tests/mir_test_framework/input_testing_server_options.cpp'
878--- tests/mir_test_framework/input_testing_server_options.cpp 2013-08-07 07:18:03 +0000
879+++ tests/mir_test_framework/input_testing_server_options.cpp 2013-08-22 22:52:58 +0000
880@@ -21,6 +21,9 @@
881 #include "mir/input/input_channel.h"
882 #include "mir/surfaces/input_registrar.h"
883 #include "mir/input/surface.h"
884+#include "mir/shell/surface_creation_parameters.h"
885+#include "mir/frontend/shell.h"
886+#include "mir/frontend/session.h"
887 #include "mir/input/composite_event_filter.h"
888
889 #include "mir_test/fake_event_hub.h"
890@@ -34,6 +37,8 @@
891 namespace mtf = mir_test_framework;
892
893 namespace ms = mir::surfaces;
894+namespace msh = mir::shell;
895+namespace mf = mir::frontend;
896 namespace mg = mir::graphics;
897 namespace mi = mir::input;
898 namespace mia = mi::android;
899@@ -48,7 +53,6 @@
900 virtual ~SurfaceReadinessListener() = default;
901
902 virtual void channel_ready_for_input(std::string const& channel_name) = 0;
903- virtual void channel_finished_for_input(std::string const& channel_name) = 0;
904
905 protected:
906 SurfaceReadinessListener() = default;
907@@ -56,36 +60,43 @@
908 SurfaceReadinessListener& operator=(SurfaceReadinessListener const&) = delete;
909 };
910
911-class ProxyInputRegistrar : public ms::InputRegistrar
912+class ProxyShell : public mf::Shell
913 {
914 public:
915- ProxyInputRegistrar(std::shared_ptr<ms::InputRegistrar> const underlying_registrar,
916- std::shared_ptr<SurfaceReadinessListener> const listener)
917- : underlying_registrar(underlying_registrar),
918+ ProxyShell(std::shared_ptr<mf::Shell> const& underlying_shell,
919+ std::shared_ptr<SurfaceReadinessListener> const listener)
920+ : underlying_shell(underlying_shell),
921 listener(listener)
922 {
923 }
924
925- ~ProxyInputRegistrar() noexcept(true) = default;
926-
927- void input_channel_opened(std::shared_ptr<mi::InputChannel> const& opened_channel,
928- std::shared_ptr<mi::Surface> const& surface,
929- mi::InputReceptionMode input_mode)
930- {
931- outstanding_channels[opened_channel] = surface->name();
932- underlying_registrar->input_channel_opened(opened_channel, surface, input_mode);
933- listener->channel_ready_for_input(surface->name());
934- }
935- void input_channel_closed(std::shared_ptr<mi::InputChannel> const& closed_channel)
936- {
937- auto channel = outstanding_channels[closed_channel];
938- underlying_registrar->input_channel_closed(closed_channel);
939- listener->channel_finished_for_input(channel);
940+ ~ProxyShell() noexcept(true) = default;
941+
942+ mf::SurfaceId create_surface_for(std::shared_ptr<mf::Session> const& session,
943+ msh::SurfaceCreationParameters const& params)
944+ {
945+ return underlying_shell->create_surface_for(session, params);
946+ }
947+
948+ std::shared_ptr<mf::Session> open_session(std::string const& name,
949+ std::shared_ptr<mf::EventSink> const& sink)
950+ {
951+ return underlying_shell->open_session(name, sink);
952+ }
953+
954+ void close_session(std::shared_ptr<mf::Session> const& session)
955+ {
956+ underlying_shell->close_session(session);
957+ }
958+
959+ void handle_surface_created(std::shared_ptr<mf::Session> const& session)
960+ {
961+ underlying_shell->handle_surface_created(session);
962+ listener->channel_ready_for_input(session->name());
963 }
964
965 private:
966- std::map<std::shared_ptr<mi::InputChannel>, std::string> outstanding_channels;
967- std::shared_ptr<ms::InputRegistrar> const underlying_registrar;
968+ std::shared_ptr<mf::Shell> const underlying_shell;
969 std::shared_ptr<SurfaceReadinessListener> const listener;
970 };
971
972@@ -126,7 +137,7 @@
973 return input_configuration;
974 }
975
976-std::shared_ptr<ms::InputRegistrar> mtf::InputTestingServerConfiguration::the_input_registrar()
977+std::shared_ptr<mf::Shell> mtf::InputTestingServerConfiguration::the_frontend_shell()
978 {
979 struct LifecycleTracker : public SurfaceReadinessListener
980 {
981@@ -145,27 +156,20 @@
982 lifecycle_condition.notify_all();
983 }
984
985- void channel_finished_for_input(std::string const& channel_name)
986- {
987- std::unique_lock<std::mutex> lg(lifecycle_lock);
988- client_lifecycles[channel_name] = mtf::ClientLifecycleState::vanished;
989- lifecycle_condition.notify_all();
990- }
991 std::mutex &lifecycle_lock;
992 std::condition_variable &lifecycle_condition;
993 std::map<std::string, mtf::ClientLifecycleState> &client_lifecycles;
994 };
995
996- if (!input_registrar)
997+ if (!frontend_shell)
998 {
999- auto registrar_listener = std::make_shared<LifecycleTracker>(lifecycle_lock,
1000+ auto readiness_listener = std::make_shared<LifecycleTracker>(lifecycle_lock,
1001 lifecycle_condition,
1002 client_lifecycles);
1003- input_registrar = std::make_shared<ProxyInputRegistrar>(the_input_configuration()->the_input_registrar(),
1004- registrar_listener);
1005+ frontend_shell = std::make_shared<ProxyShell>(DefaultServerConfiguration::the_frontend_shell(), readiness_listener);
1006 }
1007
1008- return input_registrar;
1009+ return frontend_shell;
1010 }
1011
1012 void mtf::InputTestingServerConfiguration::wait_until_client_appears(std::string const& channel_name)
1013@@ -185,22 +189,3 @@
1014 BOOST_THROW_EXCEPTION(std::runtime_error("Timed out waiting for client (" + channel_name + ") to appear"));
1015 }
1016 }
1017-
1018-void mtf::InputTestingServerConfiguration::wait_until_client_vanishes(std::string const& channel_name)
1019-{
1020- std::unique_lock<std::mutex> lg(lifecycle_lock);
1021-
1022- std::chrono::seconds timeout(60);
1023- auto end_time = std::chrono::system_clock::now() + timeout;
1024-
1025-
1026- if (client_lifecycles[channel_name] == appeared)
1027- {
1028- BOOST_THROW_EXCEPTION(std::runtime_error("Waiting for a client (" + channel_name + ") to vanish but it has already appeared"));
1029- }
1030- while (client_lifecycles[channel_name] != vanished)
1031- {
1032- if (lifecycle_condition.wait_until(lg, end_time) == std::cv_status::timeout)
1033- BOOST_THROW_EXCEPTION(std::runtime_error("Timed out waiting for client (" + channel_name + ") to vanish"));
1034- }
1035-}
1036
1037=== modified file 'tests/unit-tests/shell/test_default_focus_mechanism.cpp'
1038--- tests/unit-tests/shell/test_default_focus_mechanism.cpp 2013-08-09 22:48:27 +0000
1039+++ tests/unit-tests/shell/test_default_focus_mechanism.cpp 2013-08-22 22:52:58 +0000
1040@@ -67,6 +67,32 @@
1041 focus_mechanism.set_focus_to(mt::fake_shared(app1));
1042 }
1043
1044+TEST(DefaultFocusMechanism, mechanism_notifies_default_surface_of_focus_changes)
1045+{
1046+ using namespace ::testing;
1047+
1048+ NiceMock<mtd::MockShellSession> app1, app2;
1049+ mtd::MockSurface mock_surface1(&app1, std::make_shared<mtd::StubSurfaceBuilder>());
1050+ mtd::MockSurface mock_surface2(&app2, std::make_shared<mtd::StubSurfaceBuilder>());
1051+
1052+ ON_CALL(app1, default_surface()).WillByDefault(Return(mt::fake_shared(mock_surface1)));
1053+ ON_CALL(app2, default_surface()).WillByDefault(Return(mt::fake_shared(mock_surface2)));
1054+
1055+
1056+ {
1057+ InSequence seq;
1058+ EXPECT_CALL(mock_surface1, configure(mir_surface_attrib_focus, mir_surface_focused)).Times(1);
1059+ EXPECT_CALL(mock_surface1, configure(mir_surface_attrib_focus, mir_surface_unfocused)).Times(1);
1060+ EXPECT_CALL(mock_surface2, configure(mir_surface_attrib_focus, mir_surface_focused)).Times(1);
1061+ }
1062+
1063+ msh::DefaultFocusMechanism focus_mechanism(std::make_shared<mtd::StubInputTargeter>(),
1064+ std::make_shared<mtd::StubSurfaceController>());
1065+
1066+ focus_mechanism.set_focus_to(mt::fake_shared(app1));
1067+ focus_mechanism.set_focus_to(mt::fake_shared(app2));
1068+}
1069+
1070 TEST(DefaultFocusMechanism, sets_input_focus)
1071 {
1072 using namespace ::testing;
1073
1074=== modified file 'tests/unit-tests/shell/test_surface.cpp'
1075--- tests/unit-tests/shell/test_surface.cpp 2013-08-09 03:39:35 +0000
1076+++ tests/unit-tests/shell/test_surface.cpp 2013-08-22 22:52:58 +0000
1077@@ -21,6 +21,8 @@
1078 #include "mir/shell/surface_creation_parameters.h"
1079 #include "mir/surfaces/surface_stack_model.h"
1080 #include "mir/shell/surface_builder.h"
1081+#include "mir/frontend/event_sink.h"
1082+#include "mir/graphics/display_configuration.h"
1083
1084 #include "mir_test_doubles/stub_surface_controller.h"
1085 #include "mir_test_doubles/mock_surface_controller.h"
1086@@ -35,6 +37,7 @@
1087 #include "mir_test_doubles/null_surface_configurator.h"
1088 #include "mir_test_doubles/mock_surface_configurator.h"
1089 #include "mir_test/fake_shared.h"
1090+#include "mir_test/event_matchers.h"
1091
1092 #include <stdexcept>
1093 #include <gmock/gmock.h>
1094@@ -52,6 +55,14 @@
1095
1096 namespace
1097 {
1098+
1099+struct MockEventSink : public mf::EventSink
1100+{
1101+ ~MockEventSink() noexcept(true) {}
1102+ MOCK_METHOD1(handle_event, void(MirEvent const&));
1103+ MOCK_METHOD1(handle_display_config_change, void(mg::DisplayConfiguration const&));
1104+};
1105+
1106 class StubSurfaceBuilder : public msh::SurfaceBuilder
1107 {
1108 public:
1109@@ -454,6 +465,29 @@
1110 EXPECT_EQ(mir_surface_state_fullscreen, surf.state());
1111 }
1112
1113+TEST_F(ShellSurface, sends_focus_notifications_when_focus_gained_and_lost)
1114+{
1115+ using namespace testing;
1116+
1117+ MockEventSink sink;
1118+
1119+ {
1120+ InSequence seq;
1121+ EXPECT_CALL(sink, handle_event(mt::SurfaceEvent(mir_surface_attrib_focus, mir_surface_focused)))
1122+ .Times(1);
1123+ EXPECT_CALL(sink, handle_event(mt::SurfaceEvent(mir_surface_attrib_focus, mir_surface_unfocused)))
1124+ .Times(1);
1125+ }
1126+
1127+ msh::Surface surf(
1128+ nullptr,
1129+ mt::fake_shared(surface_builder), std::make_shared<mtd::NullSurfaceConfigurator>(),
1130+ msh::a_surface(), stub_id, mt::fake_shared(sink));
1131+
1132+ surf.configure(mir_surface_attrib_focus, mir_surface_focused);
1133+ surf.configure(mir_surface_attrib_focus, mir_surface_unfocused);
1134+}
1135+
1136 TEST_F(ShellSurface, configurator_selects_attribute_values)
1137 {
1138 using namespace testing;

Subscribers

People subscribed via source and target branches