Mir

Merge lp://qastaging/~vanvugt/mir/rename-api into lp://qastaging/mir

rename-api
Merge into development-branch

Proposed by Daniel van Vugt on 2015-03-13

Status:

Merged

Approved by:

Daniel van Vugt on 2015-03-30

Approved revision:

no longer in the source branch.

Merged at revision:

2437

Proposed branch:

lp://qastaging/~vanvugt/mir/rename-api

Merge into:

lp://qastaging/mir

Diff against target:

696 lines (+307/-14)

26 files modified

examples/fingerpaint.c (+1/-0)
examples/progressbar.c (+5/-0)
include/client/mir_toolkit/mir_surface.h (+8/-0)
include/server/mir/frontend/surface.h (+14/-0)
include/server/mir/scene/null_surface_observer.h (+1/-0)
include/server/mir/scene/surface_observer.h (+1/-0)
src/client/mir_surface.cpp (+36/-0)
src/client/mir_surface.h (+8/-0)
src/client/mir_surface_api.cpp (+42/-0)
src/client/symbols.map (+2/-1)
src/protobuf/mir_protobuf.proto (+18/-0)
src/server/frontend/protobuf_message_processor.cpp (+4/-0)
src/server/frontend/session_mediator.cpp (+28/-0)
src/server/frontend/session_mediator.h (+5/-0)
src/server/scene/basic_surface.cpp (+20/-0)
src/server/scene/basic_surface.h (+4/-1)
src/server/scene/legacy_surface_change_notification.cpp (+5/-0)
src/server/scene/legacy_surface_change_notification.h (+1/-0)
src/server/scene/null_surface_observer.cpp (+1/-0)
src/server/symbols.map (+1/-0)
tests/acceptance-tests/test_client_surfaces.cpp (+24/-0)
tests/include/mir_test_doubles/mock_frontend_surface.h (+1/-0)
tests/include/mir_test_doubles/stub_scene_surface.h (+1/-0)
tests/unit-tests/scene/test_basic_surface.cpp (+24/-12)
tests/unit-tests/scene/test_legacy_scene_change_notification.cpp (+17/-0)
tests/unit-tests/scene/test_surface_stack.cpp (+35/-0)

To merge this branch:

bzr merge lp://qastaging/~vanvugt/mir/rename-api

Related bugs:

Bug #1420573: [enhancement] Missing client function for surface resizing	Medium	Fix Released
Bug #1422522: [enhancement] Add surface morph client API	Medium	Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
PS Jenkins bot (community)	continuous-integration		Approve on 2015-03-30
Alberto Aguirre (community)			Approve on 2015-03-26
Robert Carr (community)			Approve on 2015-03-25
Alan Griffiths		2015-03-13	Needs Fixing on 2015-03-16
Chris Halse Rogers			Needs Fixing on 2015-03-16
Review via email: mp+252864@code.qastaging.launchpad.net

Commit message

Introduce a client function for surface renaming (changing window title).

More importantly this lays some foundation for multi-attribute surface
modifications or "morphing". Client-initiated resizing is included in that
and will also benefit from the framework here.

Future work will route such modifications through the shell. But names
don't ever need censoring, and it's better to avoid touching "shell" right
now while Alan's redesigning it still.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-03-13:

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

review: Approve (continuous-integration)

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2015-03-13:

*Needs discussion*

If we want to "batch" requests (as I understand the plan) then the client API ought to be more like the surface spec stuff:

    auto const spec = mir_surface_get_spec_sync(surface);
    mir_surface_spec_set_name(spec, "New name");
    ...
    mir_surface_spec_update(spec);
    mir_surface_spec_release(spec);

~~~~

If I'm wrong about the plan then...

42 +MirWaitHandle* mir_surface_rename(MirSurface* surf, char const* name);

Shouldn't this be mir_surface_set_name() for consistency with mir_surface_set_state(), mir_surface_set_preferred_orientation() etc.

~~~~

Also, while I'd not considered of this before, is there any point at all in the wait handle for any of these? There's no guarantee that how server will respond to such a request so waiting for the request to be handled seems pointless at best.

review: Needs Information

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2015-03-13:

> *Needs discussion*
>
> If we want to "batch" requests (as I understand the plan) then the client API
> ought to be more like the surface spec stuff:
>
> auto const spec = mir_surface_get_spec_sync(surface);
> mir_surface_spec_set_name(spec, "New name");
> ...
> mir_surface_spec_update(spec);
> mir_surface_spec_release(spec);

Or:

     auto const spec = mir_surface_get_change_spec_sync(surface);
     mir_surface_spec_set_name(spec, "New name");
     ...
     mir_surface_apply_change_spec(surface, spec);
     mir_surface_spec_release(spec);

Revision history for this message

Alberto Aguirre (albaguirre) wrote on 2015-03-13:

> Or:
>
> auto const spec = mir_surface_get_change_spec_sync(surface);
> mir_surface_spec_set_name(spec, "New name");
> ...
> mir_surface_apply_change_spec(surface, spec);
> mir_surface_spec_release(spec);

+1 I thought that was the plan too.

review: Needs Information

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2015-03-14:

> > Or:
> >
> > auto const spec = mir_surface_get_change_spec_sync(surface);
> > mir_surface_spec_set_name(spec, "New name");
> > ...
> > mir_surface_apply_change_spec(surface, spec);
> > mir_surface_spec_release(spec);
>
> +1 I thought that was the plan too.

Not just me then

review: Needs Fixing

Revision history for this message

Chris Halse Rogers (raof) wrote on 2015-03-16:

In general I wonder if this wouldn't be better implemented at the IPC layer as a transaction system rather than as another request that adds another duplicate to the attribute sending messages?

It would seem to generalise better, but might be too much Architecture Abstronaught. I genuinely don't know.

In addition to Alan & Alberto's needs-fixing:

182 - spec->surface_name = name;
183 + spec->surface_name = name ? name : "";

I'm mildly opposed to losing the distinction between unset and set-but-empty. I don't have any compelling use-case for the distinction in this case, but I'd like us to be consistent.

> Also, while I'd not considered of this before, is there any point at all in the wait handle for any of these?
> There's no guarantee that how server will respond to such a request so waiting for the request to be handled
> seems pointless at best.

The only case I can think that they're useful for is in detecting when the server is *not* going to respect a client request - if the wait-handle has fired and the state hasn't changed to what you expect then the server is (presumably) not going to honour the request.

This would seem to be better served by an explicit response mechanism, though.

The only unique thing I'm needs-fixing on is the distinction between nullptr-and-"".

review: Needs Fixing

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2015-03-16:

(1) Heh...

   auto const spec = mir_surface_get_change_spec_sync(surface);
   mir_surface_spec_set_name(spec, "New name");
   ...
   mir_surface_apply_change_spec(surface, spec);
   mir_surface_spec_release(spec);

which is funny, because that's what the old version of this branch looked like. Then I decided it was unnecessary to introduce the batching API all-at-once. I can reintroduce that if you really want, but it's unnecessary for just this renaming proposal (which of course never needs to be batched and can never fail).

(2) "rename" vs "set_name"... I lean towards whatever the English language provides. Rename is a more concise verb that you will find in a dictionary, and a verb is how a function name should end if you can. Slightly more preferable to set_X. Although we should consider "set_title" to match existing toolkits too.

(3) Regarding NULL name... There is no distinction between unset and set-but-empty for name. That's intentional. Real world-apps sometimes don't bother to name some windows. And we really don't care if that means the name is empty or missing. There's no value in treating those as different things.

(4) Wait handle; ironically that's a leftover from the old prototype (1). Rename indeed doesn't need it, but it's there for completeness.

The suggestions so far are indeed things I've thought about already. Effectively it's just rearranging deck chairs on the Titanic (with a brighter outcome in mind). I'm happy to rearrange them. It doesn't affect the result...

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-03-16:

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

review: Approve (continuous-integration)

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2015-03-16:

36 +/**
37 + * Change the title (name) of a surface.
38 + * \param [in] surface The surface to rename
39 + * \param [in] name The new name
40 + * \returns When the change has completed
41 + */
42 +MirWaitHandle* mir_surface_set_title(MirSurface* surf, char const* name);

> > auto const spec = mir_surface_get_change_spec_sync(surface);
> > mir_surface_spec_set_name(spec, "New name");
> > ...
> > mir_surface_apply_change_spec(surface, spec);
> > mir_surface_spec_release(spec);
>
> +1 I thought that was the plan too.

review: Needs Fixing

Revision history for this message

Robert Carr (robertcarr) wrote on 2015-03-16:

269 +// If and when we break our protocol backward-compatibility, this could be
270 +// merged with SurfaceParameters...

Should be no problem to break protocol still?

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2015-03-17:

Alan: I've intentionally kept that bit private for now, until we need it. So there's no need to agree on function names yet. But I've proposed different names. Please see: src/client/mir_surface_api.cpp

I expect mir_surface_set_title/name to be a handy short-cut, so worth publishing.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-03-17:

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

review: Approve (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-03-20:

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

review: Approve (continuous-integration)

Revision history for this message

Robert Carr (robertcarr) wrote on 2015-03-23:

341:
>> + response->clear_error();

Why?

review: Needs Information

Revision history for this message

Chris Halse Rogers (raof) wrote on 2015-03-23:

> (3) Regarding NULL name... There is no distinction between unset and set-but-
> empty for name. That's intentional. Real world-apps sometimes don't bother to
> name some windows. And we really don't care if that means the name is empty or
> missing. There's no value in treating those as different things.

Putting it another way, why add code explicitly to conflate nullptr with ""? If an application doesn't name its windows it's not going to call set_name(nullptr), either. If an application wants to explicitly not name its window, well, set_name("") is *shorter* than set_name(nullptr). I don't see what value conflating null with "" gives, and I see minor harm to it.

An example where set-but-empty versus unset matters would be a shell that provides a default window name if unset (not entirely dissimilar to what U8 does now). If the name isn't set the shell just takes the name from the .desktop file; if the name is set the shell uses that name.

More generally, other attributes have more obvious set-but-empty vs unset semantics; I'd like the API to be consistent.

Revision history for this message

Chris Halse Rogers (raof) wrote on 2015-03-23:

Otherwise fine; I prefer the _set_title naming.

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2015-03-25:

Robert:
Clear error is just like good behaviour in initializing variables. Making things more explicit. I had a better reason at the time but can't see it now...

Chris:
Yeah the nullptr thing came out of a discussion with Alan a while back. It seemed important at the time.

Happy to drop both features.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-03-25: