Add local position property to touch and pointer events #2785
Conversation
wmww
force-pushed
the
events-local-positions
branch
from
bb4f3e2
to
9b35c09
Compare
Codecov Report
@@ Coverage Diff @@
## main #2785 +/- ##
==========================================
- Coverage 78.36% 78.35% -0.02%
==========================================
Files 1052 1053 +1
Lines 71877 71905 +28
==========================================
+ Hits 56327 56338 +11
- Misses 15550 15567 +17
... and 6 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
| @@ -84,6 +84,20 @@ void MirTouchEvent::set_position(size_t index, geom::PointF position) | |||
| contacts[index].position = position; | |||
| } | |||
|
|
|||
| std::optional<geom::PointF> MirTouchEvent::local_position(size_t index) const | |||
There was a problem hiding this comment.
Nit - we've been spelling this auto MirTouchEvent::local_position(size_t index) const -> std::optional<geom::PointF>
| EXPECT_CALL(*surface, consume(mt::TouchEvent(0,0))).Times(1); | ||
| EXPECT_CALL(*surface, consume(mt::TouchUpEvent(0,0))).Times(1); | ||
| EXPECT_CALL(*surface, consume(mt::TouchEvent(1,1))).Times(1); | ||
| EXPECT_CALL(*surface, consume(mt::TouchUpEvent(1,1))).Times(1); |
There was a problem hiding this comment.
Are these changes correct? It looks to me like we're creating a 1×1 surface at (1,1), and then touching at (1,1), which should be (0,0) in surface coordinates?
There was a problem hiding this comment.
Ah, right. No, because we're now sending absolute coordinates through to consume(), but we only send the local coordinates contained within the event through to the wl_surface?
src/miroil/eventdispatch.cpp
Outdated
|
|
||
| void miroil::dispatch_input_event(const miral::Window& window, const MirInputEvent* event) | ||
| { | ||
| if (auto surface = std::shared_ptr<mir::scene::Surface>(window)) | ||
| { | ||
| surface->consume(mir::events::clone_event(*event)); | ||
| std::shared_ptr<MirEvent> const clone = mir::events::clone_event(*event); | ||
| // Unclear if this is the right thing to do |
There was a problem hiding this comment.
We should probably work out whether or not this is the right thing to do before landing, yes?
There was a problem hiding this comment.
The really correct thing to do would be to allow miroil to create input events with local and global positions. Should probably introduce methods into it's event builder that allow that. Also should change this to be more correct until it updates.
There was a problem hiding this comment.
Hmm, adding that api is just introducing more areas for possible confusion. Should probably wait until they need it.
|
Oh! Elegant solution to the clumsy event positions transformers: allow mapping all positions in an event with one method. |
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
|
Oops, sorry didn't see that |
…positions"" This reverts commit c61aa41.
For #1792 we'll want to pass events from the frontend to the window manager. In order to meaningfully use them events will need to preserve global position in addition to local position.