Merge lp:~alan-griffiths/mir/drag-and-drop-II into lp:mir
| Status: | Superseded |
|---|---|
| Proposed branch: | lp:~alan-griffiths/mir/drag-and-drop-II |
| Merge into: | lp:mir |
| Diff against target: |
2548 lines (+1425/-27) 75 files modified
examples/server_example_basic_window_manager.cpp (+8/-0) examples/server_example_basic_window_manager.h (+5/-0) include/client/mir/events/event_builders.h (+2/-0) include/client/mir_toolkit/extensions/drag_and_drop.h (+81/-0) include/server/mir/scene/null_surface_observer.h (+1/-0) include/server/mir/scene/surface.h (+1/-0) include/server/mir/scene/surface_observer.h (+2/-0) include/server/mir/shell/abstract_shell.h (+8/-0) include/server/mir/shell/focus_controller.h (+5/-0) include/server/mir/shell/input_targeter.h (+5/-0) include/server/mir/shell/shell.h (+4/-0) include/server/mir/shell/shell_wrapper.h (+8/-0) include/server/mir/shell/system_compositor_window_manager.h (+5/-0) include/server/mir/shell/window_manager.h (+5/-0) include/test/mir/test/doubles/mock_window_manager.h (+1/-0) include/test/mir/test/doubles/stub_surface.h (+1/-0) include/test/mir_test_framework/observant_shell.h (+8/-0) src/capnproto/mir_event.capnp (+5/-1) src/client/CMakeLists.txt (+2/-0) src/client/drag_and_drop.cpp (+85/-0) src/client/drag_and_drop.h (+32/-0) src/client/events/event_builders.cpp (+25/-0) src/client/mir_blob.cpp (+1/-8) src/client/mir_connection.cpp (+4/-0) src/client/mir_surface.cpp (+32/-0) src/client/mir_surface.h (+4/-0) src/client/rpc/mir_display_server.cpp (+9/-0) src/client/rpc/mir_display_server.h (+4/-0) src/client/symbols.map (+2/-0) src/common/events/pointer_event.cpp (+41/-3) src/common/events/surface_event.cpp (+39/-1) src/common/symbols.map (+4/-0) src/include/common/mir/events/pointer_event.h (+6/-1) src/include/common/mir/events/surface_event.h (+6/-1) src/include/common/mir/protobuf/display_server.h (+4/-0) src/include/common/mir_blob.h (+32/-0) src/include/server/mir/frontend/shell.h (+5/-0) src/include/server/mir/scene/surface_event_source.h (+1/-0) src/include/server/mir/scene/surface_observers.h (+1/-0) src/include/server/mir/shell/basic_window_manager.h (+15/-0) src/include/server/mir/shell/canonical_window_manager.h (+4/-0) src/protobuf/mir_protobuf.proto (+5/-0) src/protobuf/symbols.map (+26/-0) src/server/frontend/protobuf_message_processor.cpp (+4/-0) src/server/frontend/session_mediator.cpp (+20/-1) src/server/frontend/session_mediator.h (+4/-0) src/server/frontend/shell_wrapper.cpp (+8/-0) src/server/frontend/shell_wrapper.h (+5/-0) src/server/input/null_input_targeter.h (+3/-0) src/server/input/surface_input_dispatcher.cpp (+42/-8) src/server/input/surface_input_dispatcher.h (+5/-1) src/server/scene/basic_surface.cpp (+11/-0) src/server/scene/basic_surface.h (+1/-0) src/server/scene/legacy_surface_change_notification.cpp (+4/-0) src/server/scene/legacy_surface_change_notification.h (+1/-0) src/server/scene/null_surface_observer.cpp (+1/-0) src/server/scene/surface_event_source.cpp (+5/-0) src/server/shell/abstract_shell.cpp (+17/-0) src/server/shell/basic_window_manager.cpp (+21/-0) src/server/shell/canonical_window_manager.cpp (+13/-0) src/server/shell/frontend_shell.cpp (+10/-0) src/server/shell/frontend_shell.h (+5/-0) src/server/shell/shell_wrapper.cpp (+18/-0) src/server/shell/system_compositor_window_manager.cpp (+7/-0) src/server/symbols.map (+10/-0) tests/acceptance-tests/CMakeLists.txt (+2/-0) tests/acceptance-tests/drag_and_drop.cpp (+624/-0) tests/acceptance-tests/test_client_cursor_api.cpp (+1/-0) tests/include/mir/test/doubles/mock_input_targeter.h (+3/-0) tests/include/mir/test/doubles/mock_shell.h (+3/-0) tests/include/mir/test/doubles/stub_display_server.h (+4/-0) tests/include/mir/test/doubles/stub_input_targeter.h (+3/-0) tests/include/mir/test/doubles/stub_scene_surface.h (+1/-0) tests/mir_test_framework/observant_shell.cpp (+21/-2) tests/mir_test_framework/stub_surface.cpp (+4/-0) |
| To merge this branch: | bzr merge lp:~alan-griffiths/mir/drag-and-drop-II |
| Related bugs: |
| Reviewer | Review Type | Date Requested | Status |
|---|---|---|---|
| Alan Griffiths | Needs Information on 2017-03-16 | ||
| Andreas Pokorny (community) | Needs Information on 2017-03-16 | ||
| Chris Halse Rogers | Needs Information on 2017-03-15 | ||
| Brandon Schaefer (community) | Approve on 2017-03-15 | ||
| Cemil Azizoglu (community) | Needs Information on 2017-03-15 | ||
| Mir CI Bot | continuous-integration | 2017-03-14 | Approve on 2017-03-15 |
| Kevin DuBois (community) | Approve on 2017-03-14 | ||
|
Review via email:
|
|||
This proposal supersedes a proposal from 2017-03-13.
This proposal has been superseded by a proposal from 2017-03-17.
Commit Message
Initial support for Drag and Drop: tests (and implementation for) starting drag and releasing on target surface
Description of the Change
Initial support for Drag and Drop: tests (and implementation for) starting drag and releasing on target surface
One more episode to follow for handling the cancellation from content hub.
| Alan Griffiths (alan-griffiths) wrote : | # |
18:39:25 [ RUN ] DragAndDrop.
...
18:39:25 /<<BUILDDIR>
18:39:25 Value of: have_cookie.
18:39:25 Expected: is equal to true
18:39:25 Actual: false (of type bool)
18:39:25 ==11713== Invalid read of size 8
The memory errors and FD leaks that follow result from following the resulting nullptr.
| Alan Griffiths (alan-griffiths) wrote : | # |
> 18:39:25 [ RUN ]
> DragAndDrop.
> ...
> 18:39:25 /<<BUILDDIR>
> tests/drag_
> 18:39:25 Value of: have_cookie.
> 18:39:25 Expected: is equal to true
> 18:39:25 Actual: false (of type bool)
> 18:39:25 ==11713== Invalid read of size 8
>
> The memory errors and FD leaks that follow result from following the resulting
> nullptr.
Hmm wrong paste buffer, but still I don't see how this can happen:
18:35:34 11: [ RUN ] DragAndDrop.
...
18:37:06 11: /<<BUILDDIR>
18:37:06 11: Value of: have_cookie.
18:37:06 11: Expected: is equal to true
18:37:06 11: Actual: false (of type bool)
18:37:06 11: ==11713== Invalid read of size 8
Basically, the click isn't seen by the window. And on this branch the window has definitely received {mir_window_
| Mir CI Bot (mir-ci-bot) wrote : | # |
FAILED: Continuous integration, rev:4088
https:/
Executed test runs:
FAILURE: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
FAILURE: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
Click here to trigger a rebuild:
https:/
| Mir CI Bot (mir-ci-bot) wrote : | # |
PASSED: Continuous integration, rev:4089
https:/
Executed test runs:
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
Click here to trigger a rebuild:
https:/
| Mir CI Bot (mir-ci-bot) wrote : | # |
PASSED: Continuous integration, rev:4090
https:/
Executed test runs:
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
Click here to trigger a rebuild:
https:/
| Mir CI Bot (mir-ci-bot) wrote : | # |
FAILED: Continuous integration, rev:4091
https:/
Executed test runs:
FAILURE: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
FAILURE: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
Click here to trigger a rebuild:
https:/
| Alan Griffiths (alan-griffiths) wrote : | # |
Failure is unrelated (lp:1660889)
So that's 3 CI runs with the new tests passing. (And soak testing them locally.)
- 4092. By Alan Griffiths on 2017-03-14
-
Correct test names
| Mir CI Bot (mir-ci-bot) wrote : | # |
PASSED: Continuous integration, rev:4092
https:/
Executed test runs:
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
Click here to trigger a rebuild:
https:/
| Kevin DuBois (kdub) wrote : | # |
l773 could use comment here as well (like the other comment noting how CapPnP has problems)
+ //std::
+// We miss the "mouseover" occasionally (with valgrind and heavy stress about 1/20).
1995 +// But it isn't essential for the test and we've probably waited long enough
1996 +// for the mouse-down needed by the test to reach the window.
1997 +// EXPECT_
does that need a bug?
both those minor, lgtm overall
- 4093. By Alan Griffiths on 2017-03-15
-
Better comment
- 4094. By Alan Griffiths on 2017-03-15
-
merge :parent
| Alan Griffiths (alan-griffiths) wrote : | # |
> l773 could use comment here as well (like the other comment noting how CapPnP
> has problems)
> + //std::
> back_inserter(
Fixed.
> +// We miss the "mouseover" occasionally (with valgrind and heavy stress about
> 1/20).
> 1995 +// But it isn't essential for the test and we've probably waited long
> enough
> 1996 +// for the mouse-down needed by the test to reach the window.
> 1997 +// EXPECT_
> Eq(true));
> does that need a bug?
Possibly. It would appear there is a lack of consistency between the window management "model" that says the surface is ready and has focus (hence the mir_window_
I doubt we'll see an issue in "real life".
| Mir CI Bot (mir-ci-bot) wrote : | # |
PASSED: Continuous integration, rev:4094
https:/
Executed test runs:
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
Click here to trigger a rebuild:
https:/
| Cemil Azizoglu (cemil-azizoglu) wrote : | # |
Why is this an extension? Because it requires an outside entity like a content hub?
-------
Is this needed?
130 + mir_surface_
-------
| Alan Griffiths (alan-griffiths) wrote : | # |
> Why is this an extension? Because it requires an outside entity like a content
> hub?
So that we can evolve the API without breaking ABI
> Is this needed?
>
> 130 + mir_surface_
> -------
It doesn't have to be done this way, but the mir_surface_attribs and mir_window_attribs enum members have to be kept in step somehow. This seemed the clearest approach.
| Kevin DuBois (kdub) wrote : | # |
> > Why is this an extension? Because it requires an outside entity like a
> content
> > hub?
>
> So that we can evolve the API without breaking ABI
+1 to evolving client api this way, keeps 1.0 stable, and can be incorporated to main api once the work is done and there are no more warts.
| Chris Halse Rogers (raof) wrote : | # |
Needs info:
I'm not sure that overriding the events is appropriate - as I understand it, this will result in Mir sending pointer_
It looks like this will cause clients to behave incorrectly unless their event handler checks each pointer event for a drag handle, and treats pointer events with drag handles as not pointer events?
For example: given a client B that doesn't check pointer events for drag handles, if you initiate a drag and then move the pointer over B's window, this will act like you're just moving the pointer normally over B's window, and B will highlight buttons, perform on-hover actions, and so on? And then if you release the drag over a button in B's window, B will respond as if you mouse-button-
Nit: The locking in
+void MirSurface:
appears unnecessary? All of the things it accesses are stack variables (cookie) or written-
Is the locking for the appeasement of ThreadSanitizer?
| Andreas Pokorny (andreas-pokorny) wrote : | # |
> Needs info:
> I'm not sure that overriding the events is appropriate - as I understand it,
> this will result in Mir sending pointer_
> events in cases where we currently don't?
Yes leaving the boundaries of a window even when buttons are pressed will cause the source window to receive a pointer leave and potentially crossed windows to receive pointer_enter and motion events even though pointer buttons are still pressed.
>
> It looks like this will cause clients to behave incorrectly unless their event
> handler checks each pointer event for a drag handle, and treats pointer events
> with drag handles as not pointer events?
I doubt ui toolkits unaware of the drag handle will treat it as a click sequence, so there is wrong behavior but not really severe.
We cannot safely expand enums with extensions - so we either expand the MirPointerAction enum in an unsafe and problematic way (i.e. we have values like mir_pointer_
| Andreas Pokorny (andreas-pokorny) wrote : | # |
Now looking again - why do we need a surface attribute for drag and drop?
Are downstreams using values like: mir_window_attribs?
I think we should remove those.
Besides the enum changes and window attribute - lgtm
| Alan Griffiths (alan-griffiths) wrote : | # |
> > Needs info:
> > I'm not sure that overriding the events is appropriate - as I understand it,
> > this will result in Mir sending pointer_
> > events in cases where we currently don't?
>
> Yes leaving the boundaries of a window even when buttons are pressed will
> cause the source window to receive a pointer leave and potentially crossed
> windows to receive pointer_enter and motion events even though pointer buttons
> are still pressed.
>
> >
> > It looks like this will cause clients to behave incorrectly unless their
> event
> > handler checks each pointer event for a drag handle, and treats pointer
> events
> > with drag handles as not pointer events?
>
> I doubt ui toolkits unaware of the drag handle will treat it as a click
> sequence, so there is wrong behavior but not really severe.
The Nuremberg defence: https:/
| Alan Griffiths (alan-griffiths) wrote : | # |
> Now looking again - why do we need a surface attribute for drag and drop?
>
> Are downstreams using values like: mir_window_attribs?
>
> I think we should remove those.
>
> Besides the enum changes and window attribute - lgtm
We needed some event to carry the drag handle. Extending the window event seemed the path of least resistance. But we've hard-coded the existence of an attribute value pair into the API.
I can think of two alternatives:
/1/ Use window events as the transport, but filter out those with a D&D handle (and pass the handle to an entirely separate callback registered through the extension API).
/2/ Introduce a new drag-and-drop event to the transport.
If we were to introduce a new drag-and-drop event to carry the initial handle then we could also use it for leave/enter/
OTOH /1/ seems simpler.
*Seeking guidance* on /1/ or /2/
| Andreas Pokorny (andreas-pokorny) wrote : | # |
Which route will allow us to easily expand drag and drop to touch guestures?
| Alan Griffiths (alan-griffiths) wrote : | # |
> Which route will allow us to easily expand drag and drop to touch guestures?
I imagine the client-side processing needs to be different (i.e. different visual effects) so it might be easier to "piggy back" on the existing input events. But without a design for D&D touch I can't be sure.
/me wonders if Unity7 has prior art.
| Chris Halse Rogers (raof) wrote : | # |
I'd be reasonably happy with (1).
If people don't like throwing in an ad-hoc callback for each extension, an alternative would be to assign event types *in* the extension, like so:
struct DnDv1
{
int (*dnd_event_
MirDndEvent const* (*dnd_event_
MirBlob* (*dnd_event_
MirDndAction (*dnd_event_
}
…
DnDv1* dnd = mir_drag_
…
<in event handler>
if (mir_event_
{
MirDndEvent const* dnd_event = dnd->dnd_
if (dnd->dnd_
{
…
}
}
I prefer this to one-callback-
| Alan Griffiths (alan-griffiths) wrote : | # |
> I'd be reasonably happy with (1).
>
> If people don't like throwing in an ad-hoc callback for each extension, an
> alternative would be to assign event types *in* the extension, like so:
...
> if (mir_event_
Perhaps:
- if (mir_event_
+ if (dnd && dnd->is_
| Chris Halse Rogers (raof) wrote : | # |
Also happy with that :)
- 4095. By Alan Griffiths on 2017-03-17
- 4096. By Alan Griffiths on 2017-03-17
-
merge :parent
- 4097. By Alan Griffiths on 2017-03-17
-
Rework avoid a fake WindowEvent attribute

FAILED: Continuous integration, rev:4086 /mir-jenkins. ubuntu. com/job/ mir-ci/ 3142/ /mir-jenkins. ubuntu. com/job/ build-mir/ 4216/console /mir-jenkins. ubuntu. com/job/ build-0- fetch/4303 /mir-jenkins. ubuntu. com/job/ build-1- sourcepkg/ release= vivid+overlay/ 4293 /mir-jenkins. ubuntu. com/job/ build-1- sourcepkg/ release= xenial+ overlay/ 4293 /mir-jenkins. ubuntu. com/job/ build-1- sourcepkg/ release= zesty/4293 /mir-jenkins. ubuntu. com/job/ build-2- binpkg- mir/arch= amd64,compiler= clang,platform= mesa,release= zesty/4243 /mir-jenkins. ubuntu. com/job/ build-2- binpkg- mir/arch= amd64,compiler= clang,platform= mesa,release= zesty/4243/ artifact/ output/ *zip*/output. zip /mir-jenkins. ubuntu. com/job/ build-2- binpkg- mir/arch= amd64,compiler= gcc,platform= mesa,release= xenial+ overlay/ 4243 /mir-jenkins. ubuntu. com/job/ build-2- binpkg- mir/arch= amd64,compiler= gcc,platform= mesa,release= xenial+ overlay/ 4243/artifact/ output/ *zip*/output. zip /mir-jenkins. ubuntu. com/job/ build-2- binpkg- mir/arch= amd64,compiler= gcc,platform= mesa,release= zesty/4243/ console /mir-jenkins. ubuntu. com/job/ build-2- binpkg- mir/arch= cross-armhf, compiler= gcc,platform= android, release= vivid+overlay/ 4243 /mir-jenkins. ubuntu. com/job/ build-2- binpkg- mir/arch= cross-armhf, compiler= gcc,platform= android, release= vivid+overlay/ 4243/artifact/ output/ *zip*/output. zip /mir-jenkins. ubuntu. com/job/ build-2- binpkg- mir/arch= i386,compiler= gcc,platform= android, release= vivid+overlay/ 4243 /mir-jenkins. ubuntu. com/job/ build-2- binpkg- mir/arch= i386,compiler= gcc,platform= android, release= vivid+overlay/ 4243/artifact/ output/ *zip*/output. zip /mir-jenkins. ubuntu. com/job/ build-2- binpkg- mir/arch= i386,compiler= gcc,platform= mesa,release= xenial+ overlay/ 4243 /mir-jenkins. ubuntu. com/job/ build-2- binpkg- mir/arch= i386,compiler= gcc,platform= mesa,release= xenial+ overlay/ 4243/artifact/ output/ *zip*/output. zip
https:/
Executed test runs:
FAILURE: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
FAILURE: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
Click here to trigger a rebuild: /mir-jenkins. ubuntu. com/job/ mir-ci/ 3142/rebuild
https:/