Created attachment 279447 [details] [review]
Add ShellTouchDisplay

This object listens for raw events from all attached touchscreens, and
displays these onscreen, for showing/recording purposes.

Created attachment 279448 [details] [review]
windowManager: Set up keybinding for touch display

It is bound to Ctrl+Alt+Shift+t, which toggles the feature on and off.

Review of attachment 279447 [details] [review]:

::: src/shell-touch-display.c
@@ +256,3 @@
+  clutter_transition_set_to_value (transition, to);
+
+  g_signal_connect (transition, "completed",

you should just assign the transition to the actor using clutter_actor_add_transition() instead of setting the animatable.

@@ +257,3 @@
+
+  g_signal_connect (transition, "completed",
+                    G_CALLBACK (g_object_unref), NULL);

you can use ClutterTransition:remove-on-complete instead of connecting to the completed signal to unref() the Transition instance.

@@ +265,3 @@
+
+static void
+actor_animate_begin (ClutterActor *actor)

this whole function and animate_end() are questionable. you're using half of the explicit transitions API, but the implicit transitions API would be enough.

actor_animate_begin() could be replaced by:

  // set initial state
  clutter_actor_set_scale (actor, 0, 0);

  // set final state
  clutter_actor_save_easing_state (actor);
  clutter_actor_set_easing_mode (actor, CLUTTER_EASE_IN);
  clutter_actor_set_easing_duration (actor, 100);
  clutter_actor_set_scale (actor, 1, 1);
  clutter_actor_restore_easing_state (actor);

and actor_animate_end():

  // set initial state
  clutter_actor_set_opacity (actor, 255);

  // set final state
  clutter_actor_save_easing_state (actor);
  clutter_actor_set_easing_mode (actor, CLUTTER_EASE_IN);
  clutter_actor_set_easing_duration (actor, 100);
  clutter_actor_set_opacity (actor, 0);
  clutter_actor_restore_easing_state (actor);

without involving explicit transitions.

Review of attachment 279447 [details] [review]:

Some additional comments:

::: src/shell-touch-display.c
@@ +350,3 @@
+
+  info = g_slice_new0 (TouchInfo);
+  info->actor = clutter_actor_new ();

Our usual approach is to delegate the actual UI to JS/St - maybe we should do that here as well? Kind of related, did you test the interaction with the magnifier? The touch points obviously won't be scaled there (not sure they should?), but are they actually in the correct positions for all the different settings? I do remember some problems with the mouse cursor there ...

::: src/shell-touch-display.h
@@ +11,3 @@
+ * @short_description: Record from a #ClutterStage
+ *
+ * The #ShellRecorder object is used to display on-screen touch events

Left-overs from copy-paste :-)

Review of attachment 279448 [details] [review]:

Sure. Should we also have a developer setting to control the initial state (for devices without hardware keyboard)?

Review of attachment 279448 [details] [review]:

TBH, I really think we should not be installing enabled global keybindings for debug features like this. Enabled by default keybindings is something we should only add for design driven features (like we did with super+a). Please remember that users can define their own global shortcuts in Settings and applications might also be using them so enabling this by default risks breaking user setups.

(In reply to comment #6)
> Please remember that users can define their own global shortcuts in Settings
> and applications might also be using them so enabling this by default risks
> breaking user setups.

True, though the default looks sufficiently obscure - not unlike the screencast shortcut that we enabled quite some time before actually exposing it in Settings.

Created attachment 281627 [details] [review]
Add ShellTouchDisplay

This object listens for raw events from all attached touchscreens, and
displays these onscreen, for showing/recording purposes.

Thanks emmanuele for the tips :), the patch now uses implicit transitions.

(In reply to comment #4)
> Review of attachment 279447 [details] [review]:
> 
> Some additional comments:
> 
> ::: src/shell-touch-display.c
> @@ +350,3 @@
> +
> +  info = g_slice_new0 (TouchInfo);
> +  info->actor = clutter_actor_new ();
> 
> Our usual approach is to delegate the actual UI to JS/St - maybe we should do
> that here as well? 

JS is out of question because of XInput, raw events, etc... I thought about
lifting at least the ui parts somewhere else, but an api like add_spot(),
move_spot(), etc... looks a bit odd :), I also often think of St as a toolkit
subset for the shell and plugins, this would be r arely useful out of this
usecase.

> Kind of related, did you test the interaction with the
> magnifier? 

Good point, spots appear under the finger as expected, but the pointer warping
makes things very unintuitive (you touch somewhere on the screen, which moves
elsewhere below you because the pointer moved). Something could be done to
scroll the zoomed are differently on touch, I wonder though how often will both
be mixed for us to rush it :).

> ::: src/shell-touch-display.h
> @@ +11,3 @@
> + * @short_description: Record from a #ClutterStage
> + *
> + * The #ShellRecorder object is used to display on-screen touch events
> 
> Left-overs from copy-paste :-)

Oops.

(In reply to comment #6)
> Review of attachment 279448 [details] [review]:
> 
> TBH, I really think we should not be installing enabled global keybindings for
> debug features like this. Enabled by default keybindings is something we should
> only add for design driven features (like we did with super+a). Please remember
> that users can define their own global shortcuts in Settings and applications
> might also be using them so enabling this by default risks breaking user
> setups.

I see your point, which one has a precedence? I would hope it'd be pretty weird
to clobber a user keybinding.

(In reply to comment #9)
> > Our usual approach is to delegate the actual UI to JS/St - maybe we should do
> > that here as well? 
> 
> JS is out of question because of XInput, raw events, etc... I thought about
> lifting at least the ui parts somewhere else

That's all I was referring to, I didn't suggest moving *everything* into JS


> but an api like add_spot(), move_spot(), etc... looks a bit odd :)

Fair enough :)

(I had assumed a signal-based API, but admittedly ShellTouchDisplay::touch-point-added, ::touch-point-moved etc. isn't really better ...)

Alternatively you could simply get rid of the custom drawing code by using an StWidget instead of ClutterActor and add some appropriate style-class / CSS ...

*** Bug 749289 has been marked as a duplicate of this bug. ***

GNOME is going to shut down bugzilla.gnome.org in favor of  gitlab.gnome.org.
As part of that, we are mass-closing older open tickets in bugzilla.gnome.org
which have not seen updates for a longer time (resources are unfortunately
quite limited so not every ticket can get handled).

If you can still reproduce the situation described in this ticket in a recent
and supported software version, then please follow
  https://wiki.gnome.org/GettingInTouch/BugReportingGuidelines
and create a new ticket at
  https://gitlab.gnome.org/GNOME/gnome-shell/-/issues/

Thank you for your understanding and your help.