GNOME Bugzilla – Bug 705911
Add MetaCursorTracker, a new helper for tracking the cursor sprite
Last modified: 2013-08-19 14:15:13 UTC
Under X, we need to use XFixes to watch the cursor changing, while on wayland, we're in charge of setting and painting the cursor. MetaCursorTracker provides the abstraction layer for gnome-shell, which can thus drop ShellXFixesCursor. In the future, it may grow the ability to watch for pointer position too, especially if CursorEvents are added to the next version of XInput2, and thus it would also replace the PointerWatcher we use for gnome-shell's magnifier.
Created attachment 251490 [details] [review] Add MetaCursorTracker, a new helper for tracking the cursor sprite
Created attachment 251492 [details] [review] Replace ShellXFixesCursor with MetaCursorTracker Mutter now includes an object with the same purpose and functionality as ShellXFixesCursor, so we can replace our XFixes code with it and work under wayland too.
Review of attachment 251490 [details] [review]: ::: src/core/meta-cursor-tracker-private.h @@ +2,3 @@ + +/* + * Copyright (C) 2013 Red Hat, Inc. Remove (C) ::: src/core/meta-cursor-tracker.c @@ +85,3 @@ +meta_cursor_tracker_init (MetaCursorTracker *self) +{ + // (JS) Best (?) that can be assumed since XFixes doesn't provide a way of I assume this comment was copied from gnome-shell and (JS) means Joseph Scheuhmamer. Could you rewrite it using C89-style /* */ comments? @@ +147,3 @@ + self->screen = screen; + + self->using_x11 = TRUE; Any reason for this instead of meta_is_wayland_compositor() ? @@ +176,3 @@ + self = make_x11_cursor_tracker (screen); + + return screen->cursor_tracker = self; yuck. screen->cursor_tracker = self; return self; @@ +343,3 @@ + if (!tracker->using_x11) + { + /* FIXME! */ ??? I assume this is about not wanting to use the default cursor under Wayland? @@ +419,3 @@ + return; + + /* FIXME: try to use a DRM cursor when possible */ Is Cogl going to add wrappers for drmModeSetCursor / drmModeGetPlane, or are we going to do this ourselves? ::: src/core/screen.c @@ +1641,3 @@ return; screen->current_cursor = cursor; Hm, should current_cursor be in the cursor tracker? @@ +1649,3 @@ { + meta_cursor_tracker_set_root_cursor (screen->cursor_tracker, + screen->current_cursor); Is this method even worth it anymore? @@ +3867,3 @@ +{ + /* Go through our helpers and see if they want this event. + Currently, only MetaCursorTracker. Useless comment. ::: src/wayland/meta-wayland-seat.c @@ +98,3 @@ + CoglContext *context = clutter_backend_get_cogl_context (backend); + struct wl_resource *buffer = seat->sprite->buffer_ref.buffer->resource; + CoglTexture2D *texture; Why did these have to be moved out? ::: src/wayland/meta-wayland.c @@ +1305,1 @@ + meta_cursor_tracker_queue_redraw (seat->cursor_tracker, stage); update_position / revert_root should queue redraws in their implementation, as we don't want to do that in the KMS case.
Review of attachment 251492 [details] [review]: Should we update the recorder and screenshot code as well? ::: src/shell-util.c @@ +440,3 @@ + + sprite = meta_cursor_tracker_get_sprite (tracker); + clutter_texture_set_cogl_texture (texture, sprite); Why not do this from JS? CoglTexture is introspectable as a Boxed.
Created attachment 251572 [details] [review] Add MetaCursorTracker, a new helper for tracking the cursor sprite (x11 version) Under X, we need to use XFixes to watch the cursor changing, while on wayland, we're in charge of setting and painting the cursor. MetaCursorTracker provides the abstraction layer for gnome-shell, which can thus drop ShellXFixesCursor. In the future, it may grow the ability to watch for pointer position too, especially if CursorEvents are added to the next version of XInput2, and thus it would also replace the PointerWatcher we use for gnome-shell's magnifier. Ok, so this is the cherry-picked x11-only version for use in master
(In reply to comment #3) > ::: src/core/screen.c > @@ +1641,3 @@ > return; > > screen->current_cursor = cursor; > > Hm, should current_cursor be in the cursor tracker? I don't know, I don't want to change core code too much... > @@ +1649,3 @@ > { > + meta_cursor_tracker_set_root_cursor (screen->cursor_tracker, > + screen->current_cursor); > > Is this method even worth it anymore? Uhm, the cursor tracker is an implementation detail of MetaScreen, so this provides good encapsulation. > ::: src/wayland/meta-wayland.c > @@ +1305,1 @@ > + meta_cursor_tracker_queue_redraw (seat->cursor_tracker, stage); > > update_position / revert_root should queue redraws in their implementation, as > we don't want to do that in the KMS case. But you need a stage to queue_redraw, that's why it's a separate method. Also, it's easy to make the method noop for hardware cursors.
Created attachment 251574 [details] [review] Add MetaCursorTracker, a new helper for tracking the cursor sprite Under X, we need to use XFixes to watch the cursor changing, while on wayland, we're in charge of setting and painting the cursor. MetaCursorTracker provides the abstraction layer for gnome-shell, which can thus drop ShellXFixesCursor. In the future, it may grow the ability to watch for pointer position too, especially if CursorEvents are added to the next version of XInput2, and thus it would also replace the PointerWatcher we use for gnome-shell's magnifier.
Created attachment 251581 [details] [review] ShellRecorder: update to use MetaCursorTracker Replace more direct XFixes usage with a the appropriate abstraction API from mutter, which is guaranteed to work in wayland too. It doesn't yet replace pointer position tracking, although probably it should. Also, because now we're using Mutter API, we lose the standalone test case.
Created attachment 251582 [details] [review] ShellScreenshot: replace XFixes usage with MetaCursorTracker More X11 code going away from gnome-shell, and more wayland compatibility.
Review of attachment 251582 [details] [review]: OK.
Review of attachment 251581 [details] [review]: OK. ::: src/shell-recorder.c @@ +411,3 @@ + cogl_texture_get_data (texture, CLUTTER_CAIRO_FORMAT_ARGB32, stride, data); + + /* FIXME: cairo-gl? */ What about it?
Review of attachment 251574 [details] [review]: I'd split this into two parts so it's easier to merge. I'd add the X version, then "add wayland support"
Review of attachment 251572 [details] [review]: OK, with one minor nit. ::: src/core/meta-cursor-tracker.c @@ +72,3 @@ +meta_cursor_tracker_init (MetaCursorTracker *self) +{ + // (JS) Best (?) that can be assumed since XFixes doesn't provide a way of remove / rewrite comment please
Comment on attachment 251572 [details] [review] Add MetaCursorTracker, a new helper for tracking the cursor sprite (x11 version) Attachment 251572 [details] pushed as 7d1e149 - Add MetaCursorTracker, a new helper for tracking the cursor sprite
Comment on attachment 251574 [details] [review] Add MetaCursorTracker, a new helper for tracking the cursor sprite Attachment 251574 [details] pushed as 2ae7454 - Add MetaCursorTracker, a new helper for tracking the cursor sprite
Attachment 251492 [details] pushed as 2b8414a - Replace ShellXFixesCursor with MetaCursorTracker Attachment 251581 [details] pushed as d52c95a - ShellRecorder: update to use MetaCursorTracker Attachment 251582 [details] pushed as d272b0c - ShellScreenshot: replace XFixes usage with MetaCursorTracker