Mir

Merge lp://qastaging/~mir-team/mir/animated-cursors into lp://qastaging/mir

animated-cursors
Merge into development-branch

Proposed by Robert Carr on 2015-02-25

Status:

Merged

Approved by:

Daniel van Vugt on 2015-03-26

Approved revision:

no longer in the source branch.

Merged at revision:

2428

Proposed branch:

lp://qastaging/~mir-team/mir/animated-cursors

Merge into:

lp://qastaging/mir

Diff against target:

3326 lines (+1541/-197)

71 files modified

examples/CMakeLists.txt (+7/-0)
examples/animated_cursor_demo_client.c (+90/-0)
include/client/mir_toolkit/mir_buffer_stream.h (+89/-9)
include/client/mir_toolkit/mir_cursor_configuration.h (+14/-0)
include/common/mir/frontend/buffer_stream_id.h (+33/-0)
include/platform/mir/graphics/buffer.h (+2/-0)
include/server/mir/frontend/buffer_stream.h (+66/-0)
include/server/mir/frontend/session.h (+7/-0)
include/server/mir/frontend/surface.h (+6/-1)
include/server/mir/scene/session.h (+5/-0)
include/server/mir/scene/surface.h (+2/-2)
src/client/buffer_stream.cpp (+85/-1)
src/client/buffer_stream.h (+19/-0)
src/client/client_buffer_stream.h (+12/-1)
src/client/client_buffer_stream_factory.h (+8/-0)
src/client/cursor_configuration.h (+12/-0)
src/client/default_client_buffer_stream_factory.cpp (+7/-0)
src/client/default_client_buffer_stream_factory.h (+4/-0)
src/client/mir_buffer_stream_api.cpp (+91/-10)
src/client/mir_cursor_api.cpp (+24/-2)
src/client/mir_surface.cpp (+14/-2)
src/client/rpc/mir_protobuf_rpc_channel.cpp (+6/-0)
src/client/symbols.map (+5/-0)
src/platforms/android/server/buffer.cpp (+23/-0)
src/platforms/android/server/buffer.h (+1/-0)
src/platforms/mesa/server/gbm_buffer.cpp (+5/-0)
src/platforms/mesa/server/gbm_buffer.h (+1/-0)
src/platforms/mesa/server/shm_buffer.cpp (+5/-0)
src/platforms/mesa/server/shm_buffer.h (+1/-0)
src/protobuf/mir_protobuf.proto (+15/-1)
src/server/compositor/temporary_buffers.cpp (+5/-0)
src/server/compositor/temporary_buffers.h (+1/-0)
src/server/frontend/CMakeLists.txt (+1/-1)
src/server/frontend/buffer_stream_tracker.cpp (+16/-12)
src/server/frontend/buffer_stream_tracker.h (+19/-18)
src/server/frontend/protobuf_message_processor.cpp (+14/-0)
src/server/frontend/protobuf_message_processor.h (+1/-0)
src/server/frontend/session_mediator.cpp (+117/-15)
src/server/frontend/session_mediator.h (+15/-4)
src/server/scene/CMakeLists.txt (+1/-0)
src/server/scene/application_session.cpp (+47/-8)
src/server/scene/application_session.h (+10/-1)
src/server/scene/basic_surface.cpp (+128/-2)
src/server/scene/basic_surface.h (+8/-0)
src/server/scene/default_configuration.cpp (+1/-0)
src/server/scene/session_manager.cpp (+3/-1)
src/server/scene/session_manager.h (+3/-1)
src/server/scene/surfaceless_buffer_stream.cpp (+84/-0)
src/server/scene/surfaceless_buffer_stream.h (+59/-0)
tests/acceptance-tests/throwback/test_client_cursor_api.cpp (+51/-1)
tests/include/mir_test_doubles/mock_buffer.h (+1/-0)
tests/include/mir_test_doubles/mock_client_buffer_stream.h (+5/-0)
tests/include/mir_test_doubles/mock_client_buffer_stream_factory.h (+3/-0)
tests/include/mir_test_doubles/mock_frontend_surface.h (+6/-0)
tests/include/mir_test_doubles/mock_scene_session.h (+5/-0)
tests/include/mir_test_doubles/stub_buffer.h (+11/-0)
tests/include/mir_test_doubles/stub_buffer_stream_factory.h (+39/-0)
tests/include/mir_test_doubles/stub_client_buffer_stream_factory.h (+10/-2)
tests/include/mir_test_doubles/stub_scene_session.h (+13/-0)
tests/include/mir_test_doubles/stub_scene_surface.h (+2/-0)
tests/include/mir_test_doubles/stub_session.h (+12/-0)
tests/integration-tests/frontend/test_session_mediator_report.cpp (+1/-1)
tests/integration-tests/test_default_window_manager.cpp (+3/-0)
tests/integration-tests/test_session.cpp (+2/-0)
tests/unit-tests/frontend/stress_protobuf_communicator.cpp (+1/-1)
tests/unit-tests/frontend/test_client_buffer_tracker.cpp (+85/-85)
tests/unit-tests/frontend/test_session_mediator.cpp (+13/-9)
tests/unit-tests/scene/test_abstract_shell.cpp (+2/-0)
tests/unit-tests/scene/test_application_session.cpp (+77/-6)
tests/unit-tests/scene/test_session_manager.cpp (+5/-0)
tests/unit-tests/shell/test_default_window_manager.cpp (+2/-0)

To merge this branch:

bzr merge lp://qastaging/~mir-team/mir/animated-cursors

High

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
Daniel van Vugt			Abstain on 2015-03-26
PS Jenkins bot (community)	continuous-integration		Approve on 2015-03-25
Andreas Pokorny (community)			Approve on 2015-03-20
Alan Griffiths			Abstain on 2015-03-19
Chris Halse Rogers		2015-02-25	Approve on 2015-03-02
Kevin DuBois (community)			Needs Information on 2015-02-26
Review via email: mp+251018@code.qastaging.launchpad.net

Commit message

Add support for specifying cursors via buffer stream.

Description of the change

Add support for specifying cursors via buffer stream.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-02-26:

FAILED: Continuous integration, rev:2348
http://jenkins.qa.ubuntu.com/job/mir-ci/3067/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/1418/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/1417/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/1372/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/1064/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/1372/console

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

review: Needs Fixing (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-02-26:

FAILED: Continuous integration, rev:2349
http://jenkins.qa.ubuntu.com/job/mir-ci/3068/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/1419/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/1418/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/1373/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/1065/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/1373/console

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

review: Needs Fixing (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-02-26:

FAILED: Continuous integration, rev:2351
http://jenkins.qa.ubuntu.com/job/mir-ci/3069/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/1420
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/1419/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/1374/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/1066/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/1374/console

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

review: Needs Fixing (continuous-integration)

Revision history for this message

Chris Halse Rogers (raof) wrote on 2015-02-26:

144+MirBufferStream* mir_connection_create_buffer_stream(MirConnection *connection,

Oops, you meant to return MirWaitHandle; also, you're missing a callback on completion? Oh, no, you just haven't updated the prototype :).

1109- optional SurfaceId id = 1;
1110+ optional BufferStreamId id = 1;

I *think* this breaks protocol, but that should still be ok. You might need to be careful in CI, though?

I'm somewhat disappointed that we can't pull a vsyncish signal out of this and not need the client-side animation throttling, but that's probably for a later MP.

Otherwise looks sensible to me.

review: Needs Fixing

Revision history for this message

Robert Carr (robertcarr) wrote on 2015-02-26:

>> 144+MirBufferStream* mir_connection_create_buffer_stream(MirConnection *connection,

>> Oops, you meant to return MirWaitHandle; also, you're missing a callback on completion? Oh, no, >> you just haven't updated the prototype :).

whoops lol...cleaned up

Yeah hopefully vsyncish signal in the future.

Revision history for this message

Kevin DuBois (kdub) wrote on 2015-02-26:

needs info:
130 + * Create a new buffer stream unattached to a surface.

I guess, outside of cursors, is this a universal concept? I guess I'm just a bit leery of saying "this is a surface, with a bufferstream" and "this is a bufferstream", because it seems difficult to understand, if you're only looking at the API.

needs fixings:
216 + * If the configuration successfully applied buffers from the stream will be used
needs a verb
220 + * \param [in] hotspot_y The y-coordinate displacement within the image to place on the cursor.
typo

review: Needs Information

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-02-26:

FAILED: Continuous integration, rev:2353
http://jenkins.qa.ubuntu.com/job/mir-ci/3078/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/1437/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/1436/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/1391/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/1075/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/1391/console

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

review: Needs Fixing (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-02-27:

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

review: Approve (continuous-integration)

Revision history for this message

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

>> needs info:
>> 130 + * Create a new buffer stream unattached to a surface.
>>
>. I guess, outside of cursors, is this a universal concept? I guess I'm just a bit leery of saying
>> "this is a surface, with a bufferstream" and "this is a bufferstream", because it seems difficult to >> understand, if you're only looking at the API.

I think it is a universal concept...BufferStreams also being useful at least for:

1. Decorations Next
2. Subwindowing/nested compositors

Maybe the new text of the comment is a little better?

/**
* Create a new buffer stream. BufferStreams provide a way
* to exchange buffers with the server without directly
* posting them to a surface.
* For example, the resulting buffer stream may be used
* with mir_cursor_configuration_from_buffer_stream in order to post images to the system
* cursor.
*

>> needs fixings:
>> 216 + * If the configuration successfully applied buffers from the stream will be used
>> needs a verb
>> 220 + * \param [in] hotspot_y The y-coordinate displacement within the image to place on the
>> cursor. typo

Fixed! Thanks.

Revision history for this message

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

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

review: Needs Fixing (continuous-integration)

Revision history for this message

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

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

review: Approve (continuous-integration)

Revision history for this message

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

Re: BufferStreams. I'm vaguely in favour of making their construction independent of surface construction at all times (so surface_spec_for_* would take a pre-existing BufferStream).

Now LGTM.

review: Approve

Revision history for this message

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

I'm also vaguely in favor of that (at some point)

On Mon, Mar 2, 2015 at 3:47 PM, Chris Halse Rogers <email address hidden>
wrote:

> Review: Approve
>
> Re: BufferStreams. I'm vaguely in favour of making their construction
> independent of surface construction at all times (so surface_spec_for_*
> would take a pre-existing BufferStream).
>
> Now LGTM.
> --
> https://code.launchpad.net/~mir-team/mir/animated-cursors/+merge/251018
> Your team Mir development team is subscribed to branch lp:mir.
>

Revision history for this message

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

*Alan Rambling/Discussion*

131 + * to exchange buffers with the server without directly
132 + * posting them to a surface.

I'm starting to see that posting to a surface is actually a wrong abstraction.

Exchanging buffers via a buffer stream seems more general and only requires a way to associate a surface|cursor|... with a buffer stream.

If that is the way we're heading then we shouldn't be talking about the wrong approach in "without directly posting them to a surface". Rather "posting them to a surface" is just a convenient shortcut for getting the buffer stream and using that.

Revision history for this message

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

>> I'm starting to see that posting to a surface is actually a wrong abstraction.

>> Exchanging buffers via a buffer stream seems more general and only requires a way to associate a
>> surface|cursor|... with a buffer stream.

I think that's sort of what I was trying to articulate with the comment...but so far it's been confusing in two forms so I've just removed the reference to surfaces.

Revision history for this message

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

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

review: Approve (continuous-integration)

Revision history for this message

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

Fixed conflicts

Revision history for this message

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

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

review: Approve (continuous-integration)

Revision history for this message

Kevin DuBois (kdub) wrote on 2015-03-10:

nit:
221 + * \param [in] hotspot_x The x-coordinate displacement within the image to place on the cursor.
222 + * \param [in] hotspot_y The y-coordinate displacement within the image to place on the cursor.

maybe, "the displacement relative to the upper-left corner that cursor clicks are recorded", or simply, "the cursor's hotspot". Also, I seem to recall that half-pixel hotspots are sometimes useful, would float be better?

Revision history for this message

Kevin DuBois (kdub) wrote on 2015-03-10:

> >> needs info:
> >> 130 + * Create a new buffer stream unattached to a surface.
> >>
> >. I guess, outside of cursors, is this a universal concept? I guess I'm just
> a bit leery of saying
> >> "this is a surface, with a bufferstream" and "this is a bufferstream",
> because it seems difficult to >> understand, if you're only looking at the
> API.
>
> I think it is a universal concept...BufferStreams also being useful at least
> for:
>
> 1. Decorations Next
> 2. Subwindowing/nested compositors
>
> Maybe the new text of the comment is a little better?

Alright, so following the discussion, I agree with the point that a bufferstream is some sort of useful universal concept, although, I think that "a surface is just a particular type of bufferstream" is worth clarifying through some sort of association in the client api.

Revision history for this message

Kevin DuBois (kdub) wrote on 2015-03-10:

291 === added file 'include/server/mir/frontend/buffer_stream.h'
Not sure this should be in the public api... but not sure the mf::Surface should be either.

Also though, we have a mc::BufferStream, could the new interface be unified with mc::BufferStream?
http://bazaar.launchpad.net/~mir-team/mir/animated-cursors/view/head:/src/include/server/mir/compositor/buffer_stream.h

Revision history for this message

Kevin DuBois (kdub) wrote on 2015-03-10:

(just for clarity) the needs-info is non-blocking, but if it lands like it is, IMO, it would be best to unify the bufferstream concept in the client api before the next release.

Revision history for this message

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

Thanks kevin :)

>>or simply, "the cursor's hotspot"

I've adopted this language

>> I think that "a surface is just a particular type of bufferstream" is worth clarifying through
>> some sort of association in the client api.

I think a surface is not a particular type of buffer stream but rather a a surface may be associated with a buffer stream.

The current client API implements this, with the additional point that a surface comes with a preassociated buffer stream.

It could be possible to require the surface constructor to take a preconstructed buffer stream...but I don't think there is any sort of use case driving it. It seems like the current setup may even be more appropriate given that you can't really decouple buffer size and surface state.

Revision history for this message

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

>> 91 === added file 'include/server/mir/frontend/buffer_stream.h'
>> Not sure this should be in the public api... but not sure the mf::Surface should be either.

I'm not sure off the top of my head about actual downstream usage but at least Session depends on frontend::Surface being public.

>> Also though, we have a mc::BufferStream, could the new interface be unified with mc::BufferStream?

Maybe though I didn't want to shove the observer methods on mc::BufferStream and wanted to follow the convention of defining interfaces in the namespace they are used (as much as I could)

Revision history for this message

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

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

review: Approve (continuous-integration)

Revision history for this message

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

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

review: Approve (continuous-integration)

Revision history for this message

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

Hi :)

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/3229/rebuild

review: Approve (continuous-integration)

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/3233/rebuild

review: Approve (continuous-integration)

Revision history for this message

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

154 + * Create a new buffer stream unattached to a surface and wair for the result. The resulting buffer stream may be used

s/wair/wait/

PS I though we tried to stick to 80 chars in the toolkit headers?

review: Needs Fixing

Revision history for this message

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

418 +class Surface : public BufferStream

Surfaces *are* buffer streams? I was thinking "a surface has a buffer stream".

Revision history for this message

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

2207 +// TODO: Locking?

needs doing?

~~~~

2293 +class SurfacelessBufferStream : public frontend::BufferStream

The name "SurfacelessBufferStream" suggests there's a classification error in the design as it defines the type in terms of what it lacks, not what it does.

Revision history for this message

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

Thanks Alan :)

>> Toolkit header Doc comments

Addressed

>> Surfaces *are* buffer streams? I was thinking "a surface has a buffer stream".

Maybe...I'm open to this, but it doesn't change my mental model substantially.

>> 2207 +// TODO: Locking?

>> needs doing?

Deleted and added a comment explaining why it doesn't.

>> The name "SurfacelessBufferStream" suggests there's a classification error in the design as >> it defines the type in terms of what it lacks, not what it does.

The original name was SimpleBufferStream but I thought that was less useful. I guess this name is intended to say that what it does is 'provide a buffer stream which can function without a surface' in contrast to the current understanding of buffer streams...maybe this isn't useful though. I have another idea StandaloneBufferStream but it seems to fall prey to the same criticism (though featuring positive phrasing...).

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/3253/rebuild

review: Approve (continuous-integration)

Revision history for this message

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

*Needs Discussion*

I would expect a buffer stream to be something owned by surfaces, cursors, decorations. etc.

Given that I'm not convinced by the type hierarchy:

Surface /IsA/ BufferStream Why now /HasA/?
SurfacelessBufferStream /IsA/ BufferStream What is special about /not/ having a surface?

Are we going to have:

Cursor /IsA/ BufferStream?
Decoration /IsA/ BufferStream?

What about DecorationlessBufferStream and CursorlessBufferStream?

I don't feel I understand the intended roles well enough to suggest something better.

review: Needs Information

Revision history for this message

Andreas Pokorny (andreas-pokorny) wrote on 2015-03-18:

Am I msising something: mir_buffer_stream_is_valid will throw bad_cast for anything that isnt MirSurface. Additionally MirScreencast is not yet derived from BufferStream?

review: Needs Information

Revision history for this message

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

>> I would expect a buffer stream to be something owned by surfaces, cursors, decorations. etc.

I just expect that there would be a way to interact with the buffers owned by these resources through the buffer stream interface.

>> Surface /IsA/ BufferStream Why now /HasA/?

On the client side it was just an easy way to share implementation code between Surface ScreenCast and BufferStream. Of course surface could still implement buffer stream and maintain forwarding methods but it was possible to share the whole implementation. On the server side surface and surfaceless buffer stream can't share the whole implementation because of the lack of acquire_compositor_buffer on surfaceless buffer stream (and the assosciated semantic where the stream itself holds buffers). Just contrast ms::BasicSurface::swap_buffers and SurfacelessBufferStream.

>> SurfacelessBufferStream /IsA/ BufferStream What is special about /not/ having a surface?

Its rather, the existing BufferStream has special bits about being a surface, aka SurfaceBufferStream. In the global picture though it makes more sense to me to let that remain as "BufferStream" though.

I think maybe the biggest misnomer is mir::scene::SurfacelessBufferStream, but the BufferStream doesn't appear in the scene! Perhaps it should be mf::BufferStream(Impl?)?

>> Cursor /IsA/ BufferStream?
>> Decoration /IsA/ BufferStream?

I can't predict this now.

Revision history for this message

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

>> Am I msising something: mir_buffer_stream_is_valid will throw bad_cast for anything that isnt >> MirSurface.

No because it calls dynamic_cast on pointer typer which will return nullptr instead of throw bad_cast (like on reference types).

>> Additionally MirScreencast is not yet derived from BufferStream?

Yes it has a buffer stream accessor.

Revision history for this message

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

> Its rather, the existing BufferStream has special bits about being a surface,
> aka SurfaceBufferStream.

That sounds like SurfaceBufferStream /IsA/ BufferStream and the abstractions need fixing.

> In the global picture though it makes more sense to
> me to let that remain as "BufferStream" though.
>
> I think maybe the biggest misnomer is mir::scene::SurfacelessBufferStream, but
> the BufferStream doesn't appear in the scene! Perhaps it should be
> mf::BufferStream(Impl?)?

It would still make more sense to me that the scene contains a variety of objects some of which have associated buffer streams. But I don't think the scene needs to know anything more than "there is a type BufferStream". Having surface derive from BufferStream makes that impossible.

Of course, that may well require more change to existing code than what is proposed. I've not investigated in enough depth.

review: Abstain

Revision history for this message

Andreas Pokorny (andreas-pokorny) wrote on 2015-03-19:

> > Its rather, the existing BufferStream has special bits about being a
> surface,
> > aka SurfaceBufferStream.
>
> That sounds like SurfaceBufferStream /IsA/ BufferStream and the abstractions
> need fixing.
>
> > In the global picture though it makes more sense to
> > me to let that remain as "BufferStream" though.
> >
> > I think maybe the biggest misnomer is mir::scene::SurfacelessBufferStream,
> but
> > the BufferStream doesn't appear in the scene! Perhaps it should be
> > mf::BufferStream(Impl?)?
>
> It would still make more sense to me that the scene contains a variety of
> objects some of which have associated buffer streams. But I don't think the
> scene needs to know anything more than "there is a type BufferStream". Having
> surface derive from BufferStream makes that impossible.

I could also imagine a scene in which surface is only a thing attached to a scene element.
So you could say that a scene does not need to know anything more than 'there is a type surface'. Of course if surface stays our defining scene element your point is perfectly valid. I believe we will iterate on that topic again.

Revision history for this message

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

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/3285/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/3290/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Andreas Pokorny (andreas-pokorny) wrote on 2015-03-20:

I would prefer HasA over IsA on the server side, but I see us reworking surface anyhow.. I believe we can add this as is, and change later..

review: Approve

Revision history for this message

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

FAILED: Continuous integration, rev:2370
http://jenkins.qa.ubuntu.com/job/mir-ci/3306/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/1747/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/1746/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/1700/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/1303/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/1700/console

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

review: Needs Fixing (continuous-integration)

Revision history for this message

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

FAILED: Continuous integration, rev:2372
http://jenkins.qa.ubuntu.com/job/mir-ci/3323/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/1773
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/1772
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/1724/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/1320/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/1724/console

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

review: Needs Fixing (continuous-integration)

Revision history for this message

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

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

review: Approve (continuous-integration)

Revision history for this message

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

(1) MIR_CLIENT_8.4 is already for series 0.13. Don't need MIR_CLIENT_8.5.

1038 +++ src/client/symbols.map 2015-03-24 20:44:00 +0000
1043 +MIR_CLIENT_8.5 {
1044 + mir_connection_create_buffer_stream_sync;
1045 + mir_cursor_configuration_from_buffer_stream;
1046 +} MIR_CLIENT_8.4;
1047 +

review: Needs Fixing

Revision history for this message

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

Thanks Daniel, symbols file fixed.

Revision history for this message

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

Added some missing symbols too

Revision history for this message

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

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

review: Approve (continuous-integration)

Revision history for this message

Daniel van Vugt (vanvugt) on 2015-03-26:

review: Abstain

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk

The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches

to all changes:

Emanuele Antonio Faraone

Gerry Boland

Mir development team