This will require a new X server feature like pointer barriers.

(In reply to comment #1)
> This will require a new X server feature like pointer barriers.

Until we have the ability to measure 'pressure' we can require that the pointer is against the bottom of the screen for a certain length of time (we could start with about a second). We can probably add other heuristics, such as:

 * Angle of travel is between 45 and 135 degrees
 * Pointer doesn't travel horizontally along the screen edge while in contact with it
 * Pointer is moving reasonably fast when it comes into contact with the screen edge

Each of these will need to be tweaked, obviously. :)

(In reply to comment #1)
> This will require a new X server feature like pointer barriers.

Just going to say that we *could* do this with XI2 raw pointer events, but that would require porting mutter/gnome-shell to XI2. I wonder where Carlos Garnacho is, these days.

http://git.gnome.org/browse/mutter/log/?h=wip/xinput2 ?

Well yes, but that's from a while ago. It hasn't been rebased. There's probably lots of merge conflicts everywhere. :)

from a brief discussion with Peter:

<whot> mclasen: pong. let me get a coffee and we can start
<whot> mclasen: we've to pointer barrier thresholds in the works for the next server version. they can fix some of those issues (e.g. the "strong intent"). they also make the barriers send events, so the 2 s should be good
<whot> mclasen: unfortunately, you're looking at 1.14 here for the implementation
<mclasen> whot: failing that, I guess we'll be limited to an oldschool height-1-inputonly window, like wms used to put up for edge resistance ?
<whot> yeah, pretty much. dont' think raw events is a good idea here

for reference, here's the 'barrier thresholds' api that was discussed:

http://patchwork.freedesktop.org/patch/10402/

I have started working on this in the following two branches:
http://git.gnome.org/browse/gnome-shell/log/?h=wip/message-tray
http://git.gnome.org/browse/mutter/log/?h=wip/message-tray

Created attachment 219936 [details] [review]
display: Require XFixes 5.0

Created attachment 219937 [details] [review]
barrier: Add a new Meta wrapper for pointer barriers

Currently, we have a few function wrappers in the shell for pointer
barriers. If we want to implement pressure on a barrier, we need to
connect to some form of signal. In that case, we need to make a more
sophisticated wrapper for a pointer barrier. Add one, and stick it
in mutter.

Created attachment 219938 [details] [review]
barrier: Add signals for when a barrier gets hit

We want to know when a barrier gets hit so that we can implement
message tray pressure.

WIP: This requires XFixes 6.0
     Protocol may change before landing.

Created attachment 219939 [details] [review]
barrier: Add signals for when a barrier gets hit

We want to know when a barrier gets hit so that we can implement
message tray pressure.

WIP: This requires XFixes 6.0
     Protocol may change before landing.

Created attachment 219940 [details] [review]
barrier: Update to current XFixes 6.0 revision

Created attachment 219941 [details] [review]
layout: Port to the new mutter-based barrier wrappers

... and remove our old ones.

Created attachment 219942 [details] [review]
messageTray: Trigger the tray by downward pressure

I have a lot of patches to the X server and other things:

    http://github.com/magcius/fixesproto
    http://github.com/magcius/libXfixes
    http://github.com/magcius/xserver

They may or may not work at various points in time.

For now, I'd like some feedback on the new Meta.Barrier API. Stare at the mutter patch (barrier: Add a new Meta wrapper for pointer barriers) and the gnome-shell patch (layout: Port to the new mutter-based barrier wrappers) and tell me what you think.

It feels a bit clunky to me.

Review of attachment 219936 [details] [review]:

Meh... It's not that with XFixes 5.0 but no XDamage, XComposite or XShape Mutter works well...

Review of attachment 219936 [details] [review]:

If we want to force damage/composite/shape, we can do that. But right now we require libXfixes (we don't have guards like HAVE_XFIXES like we do for the other three), so might as well require it on the server as well.

Review of attachment 219937 [details] [review]:

::: src/core/barrier.c
@@ +209,3 @@
+  if (priv->display == NULL)
+    {
+      g_warning ("Trying to activate barrier without an associated display");

deactivate.

And really, MetaDisplay is the first thing mutter creates on startup, and it is a singleton.
I don't see much value in passing it around.

Review of attachment 219936 [details] [review]:

Oh, and if Florian/Owen say so, I'd gladly hard-depend on all of those.

Review of attachment 219939 [details] [review]:

::: src/core/barrier.c
@@ +168,3 @@
 
+  obj_signals[HIT] =
+    g_signal_new ("hit",

crossing-event?

(Bonus point if you synth a ClutterEvent instead of a new struct)

@@ +251,3 @@
+  event->ref_count = 1;
+  event->event_id = details->event_id;
+  event->dt = xevent->dt;

No timestamp?
This really needs to be fixed before the protocol goes stable.

Review of attachment 219940 [details] [review]:

Squash.

(And kill the GenericEvent cookie stuff in the internal API, if not needed)

Review of attachment 219941 [details] [review]:

Seems a nice cleanup.

Review of attachment 219942 [details] [review]:

Ok, assuming it works.

Review of attachment 219937 [details] [review]:

::: src/core/barrier.c
@@ +209,3 @@
+  if (priv->display == NULL)
+    {
+      g_warning ("Trying to activate barrier without an associated display");

Because everything else passes it around. There are very few things that actually call meta_get_display ().

Review of attachment 219942 [details] [review]:

::: js/ui/layout.js
@@ +1025,3 @@
+    _getVelocity: function(event, barrier) {
+        if (barrier.x1 === barrier.x2)
+            return Math.abs(event.dx);

If you're curious, here is the bug that caused the tray to open when swiping across the bottom. There's a horziontal barrier, and we were returning the X velocity.

I feel like such a moron :)

(In reply to comment #8)
> I have started working on this in the following two branches:
> http://git.gnome.org/browse/gnome-shell/log/?h=wip/message-tray
> http://git.gnome.org/browse/mutter/log/?h=wip/message-tray

I filed a separate bug for my patches:
https://bugzilla.gnome.org/show_bug.cgi?id=681392

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

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

Created attachment 231542 [details] [review]
display: Require XFixes 5.0

Rebased; not sure if there are any major changes.

Created attachment 231543 [details] [review]
barrier: Add a new Meta wrapper for pointer barriers

Currently, we have a few function wrappers in the shell for pointer
barriers. If we want to implement pressure on a barrier, we need to
connect to some form of signal. In that case, we need to make a more
sophisticated wrapper for a pointer barrier. Add one, and stick it
in mutter.



---

If this is a bit too experiemental, we can rebrand and put this in
the Shell code instead. I stuck it in here because it's supre easy to
integrate with Grand Central Station, but we can just use the shell
plugin filter instead, I guess.

Created attachment 231544 [details] [review]
barrier: Add signals for barrier events

We want to know when a barrier gets hit so that we can implement
message tray pressure.



This is the part that requires the custom X server. It should be landing
extremely soon, and will almost definitely be in 1.14. Current patches are
in:

  http://cgit.freedesktop.org/~whot/xserver/log/?h=barriers
  http://cgit.freedesktop.org/~whot/inputproto/log/?h=barriers
  http://cgit.freedesktop.org/~whot/libXi/log/?h=barriers

I'll try to make actual RPMs for Fedora people to install soon.

The new barrier features might get backed out of the server if we don't
have a client implementation being tested, so let's try and land this soon.

Created attachment 231545 [details] [review]
layout: Port to the new mutter-based barrier wrappers

... and remove our old ones.

Created attachment 231546 [details] [review]
layout: Trigger the message tray by downward pressure

Created attachment 231547 [details] [review]
layout: Don't show the tray if the user is just skirting the barrier

If the user is just pushing their mouse along the barrier's edge, rather
than pushing down onto the barrier, we shouldn't show the tray. Do this
by keeping track of the distance traveled perpendicular to the barrier,
and releasing the accumulated pressure if we pass a threshold.

Created attachment 231548 [details] [review]
messageTray: Remove dwell to open

This is now covered by pointer barriers.



This we may want to keep for jhbuilders until the new server has
been widespread enough.

Created attachment 233632 [details] [review]
layout: Remove an unused variable

The tray barrier was removed when the hot corner to activate the message
tray was removed.

Created attachment 233633 [details] [review]
layout: Extend the panel barriers all the way to the top of the screen

If the user has a horizontal layout where the primary monitor is lower
than additional monitors, the pointer barrier may not properly constrain
cursor movement. This is a known bug in the implementation of Xorg, but
but difficulties in implementation mean that Xorg maintainers suggest
that pointer barrier clients properly construct pointer barriers with
expected semantics if RANDR cursor constrainment is not implemented.

While this may have issues with certain vertical monitor layouts, other
known issues with gnome-shell's top panel bar exist in those situations,
and are not expected to be fixed, which means that incorrect geometry in
this situation is safer.

Created attachment 234260 [details] [review]
display: Require XFixes 5.0

We want to put barrier wrappers in mutter, which requre XFixes 5.0.
XFixes 5.0 was released in March, 2011, which should be old enough
to mandate support for.

Created attachment 234261 [details] [review]
barrier: Add a new Meta wrapper for pointer barriers

Currently, we have a few function wrappers in the shell for pointer
barriers. If we want to implement interactive features on barriers,
we need some sort of signal to be notified of the interactivity.
In that case, we need to make a more sophisticated object-based wrapper
for a pointer barrier. Add one, and stick it in mutter.

Created attachment 234262 [details] [review]
barrier: Add support for new barrier features in XInput 2.3

XInput 2.3 adds support for "barrier events", which let us know when
a pointer barrier has been hit, and when the pointer has stopped
hitting the barrier, and lets us "release" the barrier, temporarily
letting the pointer pass through the barrier. These features can be
combined to allow for certain pointer gestures, such as "pushing"
against the bottom of the screen, or stopping the pointer on monitor
edges while dragging slowly for increased edge precision.

This commit should allow graceful fallback if servers with
XInput 2.3 aren't supported.

Created attachment 234333 [details] [review]
layout: Remove an unused variable

The tray barrier was removed when the hot corner to activate the message
tray was removed.

Created attachment 234334 [details] [review]
layout: Extend the panel barriers all the way to the top of the screen

If the user has a horizontal layout where the primary monitor is lower
than additional monitors, the pointer barrier may not properly constrain
cursor movement. This is a known bug in the implementation of Xorg, but
but difficulties in implementation mean that Xorg maintainers suggest
that pointer barrier clients properly construct pointer barriers with
expected semantics if RANDR cursor constrainment is not implemented.

While this may have issues with certain vertical monitor layouts, other
known issues with gnome-shell's top panel bar exist in those situations,
and are not expected to be fixed, which means that incorrect geometry in
this situation is safer.

Created attachment 234335 [details] [review]
layout: Port to the new mutter-based barrier wrappers

As pressure barriers need a signalling mechanism to provide
information about when and where they are hit, an object which
provides a signal is a more appropriate abstraction for a pointer
barrier than a functional ID-based approach. Mutter has gained
pointer barrier wrappers, so use its objects instead of ours.

Created attachment 234336 [details] [review]
layout: Trigger the message tray by downward pressure

A pressure barrier is a barrier that activates after the user pushes
against the bottom of the screen in a short time. Implement this using
the new XInput 2.3 features that provide extended information about
pointer barriers, and use it so that pushing against the bottom of
the screen edge brings up the message tray.

Created attachment 234337 [details] [review]
layout: Ignore cursor movement along the barrier

If a user slides slowly along the axis of the barrier, we don't
want to activate the barrier. While a properly set timeout should
account for most of these cases, it's not much more code to throw
out events where the user is "slipping", and ensures that we'll
never show the message tray accidentally.

Created attachment 234338 [details] [review]
messageTray: Remove dwell to open

This functionality is now provided by pressure barriers. Users which
do not have XInput 2.3 support can use <Super>M or the overview to
view the message tray.

Comment on attachment 231544 [details] [review]
barrier: Add signals for barrier events

Seems to be an obsolete version of 'barrier: Add support for new barrier features in XInput 2.3'

Review of attachment 234262 [details] [review]:

Basically looks good to me, some minor stuff in detail.

::: src/core/barrier.c
@@ +363,3 @@
+
+  event->released = (xevent->flags & XIBarrierPointerReleased);
+  event->grabbed = (xevent->flags & XIBarrierDeviceIsGrabbed);

!= 0 ..., without the != 0, this is a trap if someone does 'unsigned int released : 1'

@@ +401,3 @@
+  g_return_val_if_fail (event->ref_count > 0, NULL);
+
+  g_atomic_int_inc ((volatile int *)&event->ref_count);

If you are using atomic operations for the refcounting, then the ref_count needs to be declared volatile in the struct, or at least that's my understanding.

::: src/core/display-private.h
@@ +293,3 @@
   int xinput_opcode;
+#ifdef HAVE_XI23
+  gboolean has_xinput_23;

stay consistent with the use of bitfields elsewhere in the structure. Group with other bitfields so there's some point to it.

::: src/core/display.c
@@ +818,3 @@
+  the_display->barriers = g_hash_table_new_full (g_direct_hash,
+                                                 g_direct_equal,
+                                                 NULL, g_object_unref);

Be consistent with display->window_ids in how X ids are hashed.

::: src/meta/barrier.h
@@ +51,3 @@
 } MetaBarrierDirection;
 
+struct _MetaBarrierEvent {

it's good practice to use the gtk-doc annotations here

 /*< private >*/
 int ref_count;

 /*< public >*/
 ... rest of stuff...

@@ +52,3 @@
 
+struct _MetaBarrierEvent {
+  int ref_count;

should be 'volatile guint ref_count'

@@ +61,3 @@
+  double dy;
+  gboolean released;
+  gboolean grabbed;

Write a struct doc-comment for this structure, the fields are not at all obvious. (Feel free to reference the XInput specification with an URL for the details of anything two complicated to explain in a sentence or two.)

Review of attachment 234260 [details] [review]:

I'm OK with this

Review of attachment 234261 [details] [review]:

Mostly OK, some details

::: src/core/barrier.c
@@ +14,3 @@
+G_DEFINE_TYPE (MetaBarrier, meta_barrier, G_TYPE_OBJECT)
+
+#define GET_PRIVATE(o) (G_TYPE_INSTANCE_GET_PRIVATE ((o), META_TYPE_BARRIER, MetaBarrierPrivate))

It doesn't make sense to define a macro you are going to use in one place

@@ +133,3 @@
+meta_barrier_is_active (MetaBarrier *barrier)
+{
+  return barrier->priv->barrier > 0;

!= 0 would be clearer

@@ +142,3 @@
+  Display *dpy;
+  Window root;
+

I think it would be useful to have a

 g_return_if_fail (x1 == x2 || y1 == y2)

here to replace a BadValue with a meaningful error message.

@@ +145,3 @@
+  if (priv->display == NULL)
+    {
+      g_warning ("Trying to activate barrier without an associated display.");

I have this feeling that maybe this patch has left-overs - if I do:

 new Meta.Barrier()

I get "trying to activate barrier without an associated display" instead of "a display must be provided when creating a barrier" or something. And is_active() might be better as is_destroyed() for correspondence to the public API, if it is needed at all.

@@ +152,3 @@
+  root = DefaultRootWindow (dpy);
+
+  if (meta_barrier_is_active (barrier))

how could this be the case? maybe inline this all into _constructed?

@@ +182,3 @@
+   * MetaBarrier:display:
+   *
+   * The display to construct the pointer barrier on.

Not sure why you are using doc comments and leaving empty nick / description for the paramspecs

@@ +196,3 @@
+  obj_props[PROP_X1] =
+    g_param_spec_int ("x1", "", "",
+                      -1, 0xFFFF, 0,

range here should be G_MINSHORT G_MAXSHORT (and same for the rest of the properties)

@@ +254,3 @@
+  if (priv->display == NULL)
+    {
+      g_warning ("Trying to activate barrier without an associated display");

cut-and-paste, but I don't think a warning is needed - already warned at construction just silently return

::: src/meta/barrier.h
@@ +14,3 @@
+#define META_BARRIER_CLASS(klass)    (G_TYPE_CHECK_CLASS_CAST ((klass),  META_TYPE_BARRIER, MetaBarrierClass))
+#define META_IS_BARRIER(obj)         (G_TYPE_CHECK_INSTANCE_TYPE ((obj), META_TYPE_BARRIER))
+#define META_IS_BARRIER_CLASS(klass) (G_TYPE_CHECK_CLASS_TYPE ((klass), META_TYPE_BARRIER))

alignment

@@ +37,3 @@
+gboolean meta_barrier_is_active (MetaBarrier *barrier);
+void meta_barrier_destroy (MetaBarrier *barrier);
+void meta_barrier_release (MetaBarrier *barrier,

In the wrong patch, with wrong prototype - the 'new barrier features' patch overwrites this

@@ +42,3 @@
+/* Keep in sync with XFixes */
+typedef enum {
+  META_BARRIER_DIRECTION_POSITIVE_X = (1L << 0),

parens and the L suffix are unnecessary here, and don't correspond to elsewhere in Mutter. (If the L had an effect and forced the enum to be long, that would be bad, but I think it has no effect at all.)

Review of attachment 234333 [details] [review]:

Yep

Review of attachment 234334 [details] [review]:

So, with the problems with the primary monitor on bottom, we need to do one or the other of:

 A) Prevent these layouts from being established in System Settings. If you drag the primary monitor to the bottom, I guess the panel pops to the top monitor.
 B) Fix things as well as possible so that we just have "doesn't work ideally" rather than broken. Probably that means using a releasable barrier at the top of the screen, allowing dragging a maximized window up through that barrier, and not setting a strut and just letting menus lap over the panel. So people can try out how things it works, and decide against it unless they *really* want it.

Can you get designer input on that? That informs what this patch should do. Even if we follow course A) though, I'm reluctant to say that this patch is OK unless we're *actually* doing it this cycle. Piling the breakage higher and deeper because it was already broken isn't going to get us to quality.

Review of attachment 234335 [details] [review]:

OK

Review of attachment 234336 [details] [review]:

Mostly looks OK to me - will need to get it into the hands of designers for final tweeking.

::: js/ui/layout.js
@@ +136,3 @@
         this.addChrome(this.trayBox);
+        this.trayBox.connect('allocation-changed',
+                             Lang.bind(this, this._updateTrayBarrier));

any reason not to just do this in monitorsChanged? less we can do during allocation, the better.

@@ +297,3 @@
+        if (this._trayPressure) {
+            this._trayPressure.destroy();
+            this._trayPressure = null;

You, however, leak this._trayBarrier

@@ +1204,3 @@
+    },
+
+    _getVelocity: function(event) {

velocity is distance over time :-)

@@ +1222,3 @@
+        } else if (event.time > this._beginningTime + this._timeout) {
+            this._reset();
+            this._beginningTime = event.time;

There's a slight deviation from expected behavior here - consider what happens if the user:

 swipes the trackpad to move the pointer down to the bottom of the screen
 after a few seconds, swipes down again meaning to trigger the tray

then because there is a pending barrier hit that times out, the second swipe is arbitrarily reset in the middle. I don't know if this causes a problem at all, but if it does cause a problem, then it's going to rarely cause a subtle confusion to users in a way that we'll never explicitly notice and track down. A more fully correct algorithm would be to:

 - During the barrier hit keep an array of all events with a time less than 'timeout' before the last received event
 - When removing old events, reduce currentPressure

the main downside of this approach is that we might be keeping as much as a few hundred barrier events alive in some cases, but my feeling that's OK.

Review of attachment 234337 [details] [review]:

If it feels right, it is right...

Review of attachment 234338 [details] [review]:

As discussed earlier, I'm not OK with this. While people may trigger the current tray accidentally and get annoyed sometimes, forcing people to learn a different set of gestures, and then go back when they upgrade their X doesn't make sense. Conditionalize on the accessor you added in Mutter.

For reference, as I wondered if "Can you get designer input on that?" got anything, there was this:

> <aday> owen, what's the issue, exactly? https://bugzilla.gnome.org/show_bug.cgi?id=677215#c53
> <owen> aday: right now, if you try to drag a window up from the bottom monitor, *through* the panel, it maximizes and stays stuck on the bottom monitor
> <owen> we get frequent dups of this
> <owen> aday: that's definitely fixable, but we will be left with some non-idealness a) activities hotcorner will be harder to hit - though releasable barriers can help b) maximization gesture makes less sense - releasable barriers can maybe help? 
> <owen> aday: so the question is - do we consider that the panel is always at a screen edge, anything else is not going to work well, and we make System Settings enforce that or do we make it work "as well as possible" and let users decide if they like that better than the alternative
> <owen> aday: this is in the case where you have your laptop and an external monitor directly above it on an arm
> <aday> so it's a question of whether we allow vertically stacked displays with the primary on top?
> <owen> (Oh, the other non-idealness, which is technical but hard to fix, is that we can only support screen-edge reserved areas, so GTK+ menus will go over the panel)
> <owen> (without fairly major window spec changes)
> <Jasper> aday, primary on the bottom
> <Jasper> aday, basically, take all the monitors and remove the bevels and space between them and you have a giant shape
> <Jasper> aday, is the panel allowed anywhere but the edge of a shape?

In one of the reviews, Owen say: will need to get this in the designers hands for final tweaking.
Indeed, it would be good to get this merged sooner, rather than later - how about 3.7.5 (next week) ?

Created attachment 235010 [details] [review]
display: Require XFixes 5.0

We want to put barrier wrappers in mutter, which requre XFixes 5.0.
XFixes 5.0 was released in March, 2011, which should be old enough
to mandate support for.

Created attachment 235011 [details] [review]
barrier# Attachment to Bug 677215 - Trigger the tray with downward pressure, not the hot corner

barrier: Add a new Meta wrapper for pointer barriers

Currently, we have a few function wrappers in the shell for pointer
barriers. If we want to implement interactive features on barriers,
we need some sort of signal to be notified of the interactivity.
In that case, we need to make a more sophisticated object-based wrapper
for a pointer barrier. Add one, and stick it in mutter.



This should address all review comments.

Review of attachment 234261 [details] [review]:

::: src/meta/barrier.h
@@ +42,3 @@
+/* Keep in sync with XFixes */
+typedef enum {
+  META_BARRIER_DIRECTION_POSITIVE_X = (1L << 0),

Note that I stole this from Xfixes:

#define BarrierPositiveX		    (1L << 0)
#define BarrierPositiveY		    (1L << 1)
#define BarrierNegativeX		    (1L << 2)
#define BarrierNegativeY		    (1L << 3)

Comment on attachment 235010 [details] [review]
display: Require XFixes 5.0

Attachment 235010 [details] pushed as 7105555 - display: Require XFixes 5.0

Comment on attachment 234333 [details] [review]
layout: Remove an unused variable

Attachment 234333 [details] pushed as 7798da8 - layout: Remove an unused variable

Created attachment 235013 [details] [review]
barrier: Add support for new barrier features in XInput 2.3

XInput 2.3 adds support for "barrier events", which let us know when
a pointer barrier has been hit, and when the pointer has stopped
hitting the barrier, and lets us "release" the barrier, temporarily
letting the pointer pass through the barrier. These features can be
combined to allow for certain pointer gestures, such as "pushing"
against the bottom of the screen, or stopping the pointer on monitor
edges while dragging slowly for increased edge precision.

This commit should allow graceful fallback if servers with
XInput 2.3 aren't supported.

Created attachment 235014 [details] [review]
layout: Port to the new mutter-based barrier wrappers

As pressure barriers need a signalling mechanism to provide
information about when and where they are hit, an object which
provides a signal is a more appropriate abstraction for a pointer
barrier than a functional ID-based approach. Mutter has gained
pointer barrier wrappers, so use its objects instead of ours.

Created attachment 235015 [details] [review]
layout: Trigger the message tray by downward pressure

A pressure barrier is a barrier that activates after the user pushes
against the bottom of the screen in a short time. Implement this using
the new XInput 2.3 features that provide extended information about
pointer barriers, and use it so that pushing against the bottom of
the screen edge brings up the message tray.

Created attachment 235017 [details] [review]
layout: Ignore cursor movement along the barrier

If a user slides slowly along the axis of the barrier, we don't
want to activate the barrier. While a properly set timeout should
account for most of these cases, it's not much more code to throw
out events where the user is "slipping", and ensures that we'll
never show the message tray accidentally.

Created attachment 235018 [details] [review]
messageTray: Remove tray dwelling if we have new barrier features

If we have the new barrier features, then we should not activate the
old tray dwelling mechanism.

Review of attachment 235013 [details] [review]:

Mostly right, one detail, some doc suggestions, and an optional change to how we track the barrier IDs

::: src/core/display-private.h
@@ +291,3 @@
   int xinput_opcode;
+#ifdef HAVE_XI23
+  gboolean has_xinput_23;

My comment:

> stay consistent with the use of bitfields elsewhere in the structure. Group with other 
> bitfields so there's some point to it.

was maybe missed?

::: src/core/display.c
@@ +594,3 @@
+  the_display->barrier_ids = g_hash_table_new_full (meta_unsigned_long_hash,
+                                                    meta_unsigned_long_equal,
+                                                    NULL, g_object_unref);

This is OK - I noted what I did for sync alarms was to put them in the window ID table

+#ifdef HAVE_XSYNC
+/* We store sync alarms in the window ID hash table, because they are
+ * just more types of XIDs in the same global space, but we have
+ * typesafe functions to register/unregister for readability.
+ */
+
+MetaWindow*
+meta_display_lookup_sync_alarm (MetaDisplay *display,
+                                XSyncAlarm   alarm)
+{
+  return g_hash_table_lookup (display->window_ids, &alarm);
+}

(etc.)

In patch "Move sync alarms to be per-window and permanent". Would be some advantage to being consistent with that.

::: src/meta/barrier.h
@@ +52,3 @@
+/**
+ * MetaBarrierEvent:
+ * @event_id: The event ID as returned from the server

Undescriptive - something like "A unique integer ID identifying a consecutive series of motions at or along the barrier."

@@ +54,3 @@
+ * @event_id: The event ID as returned from the server
+ * @dt: Server time, in milliseconds, since the event sent
+ * for this barrier

Missing word "last"

@@ +62,3 @@
+ * of X movement past the barrier, in screen coordinates
+ * @released: A boolean flag indicating if a client has
+ * released the barrier

A boolean flag, %TRUE if this event is generated by the pointer leaving the barrier as a result of a client calling XIBarrierReleasePointer() (will be set only for ::leave signals)

@@ +64,3 @@
+ * released the barrier
+ * @grabbed: A boolean flag indicating if the pointer has
+ * been grabbed while hitting this barrier.

According to the spec, the flag is set if the pointer was grabbed when *this* event was generated.

Review of attachment 235011 [details] [review]:

Just a few tiny things, feel free to commit after fixing.

::: src/core/barrier.c
@@ +174,3 @@
+   * MetaBarrier:display:
+   *
+   * The display to construct the pointer barrier on.

There's no point in duplicating betwen param spec and doc comment, delete the doc comments

@@ +192,3 @@
+                      "X1",
+                      "The first X coordinate of the barrier",
+                      -1, G_MAXSHORT, -1,

This range doesn't make sense ... the minimum either needs to be 0 or G_MINSHORT depending on whether you are going based upon what is useful or what the protocol allows. If there is some reason why -1 is the only useful negative value, you need to document it.

(In reply to comment #71)
> Review of attachment 235011 [details] [review]:
> @@ +192,3 @@
> +                      "X1",
> +                      "The first X coordinate of the barrier",
> +                      -1, G_MAXSHORT, -1,
> 
> This range doesn't make sense ... the minimum either needs to be 0 or
> G_MINSHORT depending on whether you are going based upon what is useful or what
> the protocol allows. If there is some reason why -1 is the only useful negative
> value, you need to document it.

See: http://cgit.freedesktop.org/xorg/xserver/commit/?id=707b4dc61f18960611409ef5ad8947be189f7296

Passing -1 means that the barrier extends infinitely in space. This is only really useful for X2/Y2.

The protocol technically allows any negative value, but I think it's more useful to have to explicitly pass -1.

(In reply to comment #70)
> Review of attachment 235013 [details] [review]:
> ::: src/core/display-private.h
> @@ +291,3 @@
>    int xinput_opcode;
> +#ifdef HAVE_XI23
> +  gboolean has_xinput_23;
> 
> My comment:
> 
> > stay consistent with the use of bitfields elsewhere in the structure. Group with other 
> > bitfields so there's some point to it.
> 
> was maybe missed?

It was, yes. Will fix.

> ::: src/core/display.c
> @@ +594,3 @@
> +  the_display->barrier_ids = g_hash_table_new_full (meta_unsigned_long_hash,
> +                                                    meta_unsigned_long_equal,
> +                                                    NULL, g_object_unref);

They're reffed in the table, but maybe we could remove that and the meta_barrier_finalize removes them from the HT?

The issue with that is that having a non-rooted barrier (let myBarrier;) could end up being destroyed by the GC at any time, but in the above case you always leak it. Hm...

Review of attachment 235015 [details] [review]:

Don't quite understand how you interpreted my suggestion for an algorithm.

::: js/ui/layout.js
@@ +1141,3 @@
+    },
+
+    _getVelocity: function(event) {

No, really, don't call a function that returns a distance getVelocity :-)

@@ +1149,3 @@
+
+    _isBarrierEventTooOld: function(event) {
+        return event.time > this._beginningTime + this._timeout;

This test doesn't make sense to me. I don't think there should be a beginningTime - there should be a latestTime

@@ +1173,3 @@
+        // If we've received an old event, make sure not
+        // to start the timer.
+        if (this._isBarrierEventTooOld(event))

a "too old" event is going to be one that is too much before this event, so how can we call isBarrierEventTooOld() on it? (maybe I'm just not understanding what you are doing with beginningTime)

Review of attachment 235018 [details] [review]:

::: js/ui/messageTray.js
@@ +1631,3 @@
     },
 
+    _setupTrayDwellIfNeeded: function() {

Never called!

(In reply to comment #73)

> > ::: src/core/display.c
> > @@ +594,3 @@
> > +  the_display->barrier_ids = g_hash_table_new_full (meta_unsigned_long_hash,
> > +                                                    meta_unsigned_long_equal,
> > +                                                    NULL, g_object_unref);
> 
> They're reffed in the table, but maybe we could remove that and the
> meta_barrier_finalize removes them from the HT?
> 
> The issue with that is that having a non-rooted barrier (let myBarrier;) could
> end up being destroyed by the GC at any time, but in the above case you always
> leak it. Hm...

The simplest is that the extra reference should be associated with the existence of the X ID

 * Create a barrier - the X ID is allocated, the ID is inserted into the XID table, we ref the object.

 * Call destroy() on a barrier - *if the barrier is not already destroyed*, the X ID is freed, we unref the object.

What I'm saying is that if we only remove the barrier from the hash table in one place, we don't need to hash table to do the g_object_unref() - we can just do it ourselves when we remove the object from the hash table.

I think it's fine if '{ let barrier = new Meta.Barrier() }' leaks the barrier.

As you say, the alternative is that he hash table is weak and doesn't hold a reference, and we destroy() in finalize() if not already destroyed, but I dislike making X calls with user-visible effects out of finalize.

Created attachment 235335 [details] [review]
barrier: Add a new Meta wrapper for pointer barriers

Currently, we have a few function wrappers in the shell for pointer
barriers. If we want to implement interactive features on barriers,
we need some sort of signal to be notified of the interactivity.
In that case, we need to make a more sophisticated object-based wrapper
for a pointer barrier. Add one, and stick it in mutter.

Created attachment 235336 [details] [review]
barrier: Add support for new barrier features in XInput 2.3

XInput 2.3 adds support for "barrier events", which let us know when
a pointer barrier has been hit, and when the pointer has stopped
hitting the barrier, and lets us "release" the barrier, temporarily
letting the pointer pass through the barrier. These features can be
combined to allow for certain pointer gestures, such as "pushing"
against the bottom of the screen, or stopping the pointer on monitor
edges while dragging slowly for increased edge precision.

This commit should allow graceful fallback if servers with
XInput 2.3 aren't supported.

Created attachment 235337 [details] [review]
display: Rename window_ids to xids

As the hash table no longer stores only window IDs, we should rename it so
that we make sure to check if something is actually a window before using it
as a window.

Created attachment 235338 [details] [review]
messageTray: Remove tray dwelling if we have new barrier features

If we have the new barrier features, then we should not activate the
old tray dwelling mechanism.

Created attachment 235339 [details] [review]
layout: Port to the new mutter-based barrier wrappers

As pressure barriers need a signalling mechanism to provide
information about when and where they are hit, an object which
provides a signal is a more appropriate abstraction for a pointer
barrier than a functional ID-based approach. Mutter has gained
pointer barrier wrappers, so use its objects instead of ours.

Created attachment 235340 [details] [review]
layout: Trigger the message tray by downward pressure

A pressure barrier is a barrier that activates after the user pushes
against the bottom of the screen in a short time. Implement this using
the new XInput 2.3 features that provide extended information about
pointer barriers, and use it so that pushing against the bottom of
the screen edge brings up the message tray.

Review of attachment 235336 [details] [review]:

Good to commit (not *completely* sure I like having multiple files of the code manipulating display->window_ids, but it is OK)

Review of attachment 235337 [details] [review]:

OK

Review of attachment 235338 [details] [review]:

OK

Review of attachment 235340 [details] [review]:

::: js/ui/layout.js
@@ +1188,3 @@
+    _onBarrierHit: function(barrier, event) {
+        if (this._isBarrierEventTooOld(event))
+            return;

I don't understand the presence of this - everything else makes sense now.

Review of attachment 235340 [details] [review]:

::: js/ui/layout.js
@@ +1188,3 @@
+    _onBarrierHit: function(barrier, event) {
+        if (this._isBarrierEventTooOld(event))
+            return;

If the server is lagging, it could send us a very old event very late, which we should throw out.

Did I get that wrong?

Review of attachment 235335 [details] [review]:

Looks good

::: src/core/barrier.c
@@ +182,3 @@
+                      "X1",
+                      "The first X coordinate of the barrier",
+                      0, G_MAXSHORT, 0,

I would have been fine with the -1, especially if there was a "-1 for infinite extent" or something in the doc comment. But this is OK too - we can add another patch later.

(In reply to comment #87)
> Review of attachment 235340 [details] [review]:
> 
> ::: js/ui/layout.js
> @@ +1188,3 @@
> +    _onBarrierHit: function(barrier, event) {
> +        if (this._isBarrierEventTooOld(event))
> +            return;
> 
> If the server is lagging, it could send us a very old event very late, which we
> should throw out.
> 
> Did I get that wrong?

The X server is always going to send events in sequence - it would be unexpected to get a time going backwards. This would basically be an X server bug or someone doing XSendEvent(). If we do have a time that appears in the past, I think we should just proceed as if it's the latest event - we'll throw everything out and start over on reset() anyways.

Attachment 235335 [details] pushed as 8b21df9 - barrier: Add a new Meta wrapper for pointer barriers
Attachment 235336 [details] pushed as 57c31a5 - barrier: Add support for new barrier features in XInput 2.3
Attachment 235337 [details] pushed as d8f569e - display: Rename window_ids to xids

Attachment 235338 [details] pushed as e614b99 - messageTray: Remove tray dwelling if we have new barrier features
Attachment 235339 [details] pushed as c0279df - layout: Port to the new mutter-based barrier wrappers
Attachment 235340 [details] pushed as fb63f48 - layout: Trigger the message tray by downward pressure


Pushed after removing the suggested hunk:

<Jasper> owen, I was imagining the server sending events in order, but much later than the current time, but now I realize that would require a round-trip to actually compare against the current time

For now, let's consider this complete. After I show this to Jon in person, there will likely be a follow-up bug where we tweak thresholds and heuristics.

Created attachment 235556 [details] [review]
barrier: fix fallback for unsupported servers

add missing ifdef, and change XInput2.3 detection, Ubuntu has XI
1.6.99 with the XInput2.3 patches reverted, since Xserver has not
been updated yet.

Created attachment 235557 [details] [review]
barrier: fix fallback for unsupported servers

add missing ifdef, and change XInput2.3 detection, Ubuntu has XI
1.6.99 with the XInput2.3 patches reverted, since Xserver has not
been updated yet.

Created attachment 235560 [details] [review]
barrier: fix fallback for unsupported servers

add missing ifdef HAVE_XI23.

Comment on attachment 235560 [details] [review]
barrier: fix fallback for unsupported servers

OK'ed on IRC by Jasper
Attachment 235560 [details] pushed as b3c572b - barrier: fix fallback for unsupported servers

It would be nice to have some sort of visual indication on how much pressure is applied, similar to Android.