After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 677215 - Trigger the tray with downward pressure, not the hot corner
Trigger the tray with downward pressure, not the hot corner
Status: RESOLVED FIXED
Product: gnome-shell
Classification: Core
Component: message-tray
3.3.x
Other Linux
: Normal normal
: ---
Assigned To: gnome-shell-maint
gnome-shell-maint
: 687386 687445 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2012-05-31 17:46 UTC by Allan Day
Modified: 2013-02-11 18:25 UTC
See Also:
GNOME target: 3.8
GNOME version: ---


Attachments
display: Require XFixes 5.0 (3.01 KB, patch)
2012-07-30 22:18 UTC, Jasper St. Pierre (not reading bugmail)
reviewed Details | Review
barrier: Add a new Meta wrapper for pointer barriers (8.85 KB, patch)
2012-07-30 22:18 UTC, Jasper St. Pierre (not reading bugmail)
reviewed Details | Review
barrier: Add signals for when a barrier gets hit (8.93 KB, patch)
2012-07-30 22:18 UTC, Jasper St. Pierre (not reading bugmail)
none Details | Review
barrier: Add signals for when a barrier gets hit (8.93 KB, patch)
2012-07-30 22:20 UTC, Jasper St. Pierre (not reading bugmail)
reviewed Details | Review
barrier: Update to current XFixes 6.0 revision (1.63 KB, patch)
2012-07-30 22:20 UTC, Jasper St. Pierre (not reading bugmail)
needs-work Details | Review
layout: Port to the new mutter-based barrier wrappers (8.27 KB, patch)
2012-07-30 22:21 UTC, Jasper St. Pierre (not reading bugmail)
accepted-commit_now Details | Review
messageTray: Trigger the tray by downward pressure (6.56 KB, patch)
2012-07-30 22:21 UTC, Jasper St. Pierre (not reading bugmail)
accepted-commit_now Details | Review
display: Require XFixes 5.0 (3.01 KB, patch)
2012-12-14 07:32 UTC, Jasper St. Pierre (not reading bugmail)
none Details | Review
barrier: Add a new Meta wrapper for pointer barriers (10.46 KB, patch)
2012-12-14 07:34 UTC, Jasper St. Pierre (not reading bugmail)
none Details | Review
barrier: Add signals for barrier events (9.35 KB, patch)
2012-12-14 07:37 UTC, Jasper St. Pierre (not reading bugmail)
none Details | Review
layout: Port to the new mutter-based barrier wrappers (6.79 KB, patch)
2012-12-14 07:38 UTC, Jasper St. Pierre (not reading bugmail)
none Details | Review
layout: Trigger the message tray by downward pressure (4.72 KB, patch)
2012-12-14 07:39 UTC, Jasper St. Pierre (not reading bugmail)
none Details | Review
layout: Don't show the tray if the user is just skirting the barrier (4.00 KB, patch)
2012-12-14 07:39 UTC, Jasper St. Pierre (not reading bugmail)
none Details | Review
messageTray: Remove dwell to open (4.32 KB, patch)
2012-12-14 07:39 UTC, Jasper St. Pierre (not reading bugmail)
none Details | Review
layout: Remove an unused variable (788 bytes, patch)
2013-01-16 22:21 UTC, Jasper St. Pierre (not reading bugmail)
none Details | Review
layout: Extend the panel barriers all the way to the top of the screen (2.01 KB, patch)
2013-01-16 22:21 UTC, Jasper St. Pierre (not reading bugmail)
none Details | Review
display: Require XFixes 5.0 (3.16 KB, patch)
2013-01-23 22:23 UTC, Jasper St. Pierre (not reading bugmail)
accepted-commit_now Details | Review
barrier: Add a new Meta wrapper for pointer barriers (10.26 KB, patch)
2013-01-23 22:23 UTC, Jasper St. Pierre (not reading bugmail)
needs-work Details | Review
barrier: Add support for new barrier features in XInput 2.3 (14.29 KB, patch)
2013-01-23 22:23 UTC, Jasper St. Pierre (not reading bugmail)
needs-work Details | Review
layout: Remove an unused variable (788 bytes, patch)
2013-01-24 20:57 UTC, Jasper St. Pierre (not reading bugmail)
committed Details | Review
layout: Extend the panel barriers all the way to the top of the screen (2.01 KB, patch)
2013-01-24 20:58 UTC, Jasper St. Pierre (not reading bugmail)
none Details | Review
layout: Port to the new mutter-based barrier wrappers (6.95 KB, patch)
2013-01-24 20:58 UTC, Jasper St. Pierre (not reading bugmail)
accepted-commit_now Details | Review
layout: Trigger the message tray by downward pressure (5.62 KB, patch)
2013-01-24 20:58 UTC, Jasper St. Pierre (not reading bugmail)
needs-work Details | Review
layout: Ignore cursor movement along the barrier (2.11 KB, patch)
2013-01-24 20:58 UTC, Jasper St. Pierre (not reading bugmail)
accepted-commit_now Details | Review
messageTray: Remove dwell to open (4.77 KB, patch)
2013-01-24 20:58 UTC, Jasper St. Pierre (not reading bugmail)
committed Details | Review
display: Require XFixes 5.0 (3.16 KB, patch)
2013-02-01 18:03 UTC, Jasper St. Pierre (not reading bugmail)
committed Details | Review
barrier# Attachment to Bug 677215 - Trigger the tray with downward pressure, not the hot corner (10.52 KB, patch)
2013-02-01 18:04 UTC, Jasper St. Pierre (not reading bugmail)
needs-work Details | Review
barrier: Add support for new barrier features in XInput 2.3 (14.77 KB, patch)
2013-02-01 18:15 UTC, Jasper St. Pierre (not reading bugmail)
needs-work Details | Review
layout: Port to the new mutter-based barrier wrappers (6.98 KB, patch)
2013-02-01 18:15 UTC, Jasper St. Pierre (not reading bugmail)
accepted-commit_now Details | Review
layout: Trigger the message tray by downward pressure (6.07 KB, patch)
2013-02-01 18:15 UTC, Jasper St. Pierre (not reading bugmail)
needs-work Details | Review
layout: Ignore cursor movement along the barrier (2.81 KB, patch)
2013-02-01 18:16 UTC, Jasper St. Pierre (not reading bugmail)
committed Details | Review
messageTray: Remove tray dwelling if we have new barrier features (1.90 KB, patch)
2013-02-01 18:16 UTC, Jasper St. Pierre (not reading bugmail)
needs-work Details | Review
barrier: Add a new Meta wrapper for pointer barriers (9.96 KB, patch)
2013-02-06 22:10 UTC, Jasper St. Pierre (not reading bugmail)
committed Details | Review
barrier: Add support for new barrier features in XInput 2.3 (15.43 KB, patch)
2013-02-06 22:10 UTC, Jasper St. Pierre (not reading bugmail)
committed Details | Review
display: Rename window_ids to xids (5.06 KB, patch)
2013-02-06 22:10 UTC, Jasper St. Pierre (not reading bugmail)
committed Details | Review
messageTray: Remove tray dwelling if we have new barrier features (1.94 KB, patch)
2013-02-06 22:11 UTC, Jasper St. Pierre (not reading bugmail)
committed Details | Review
layout: Port to the new mutter-based barrier wrappers (7.01 KB, patch)
2013-02-06 22:11 UTC, Jasper St. Pierre (not reading bugmail)
committed Details | Review
layout: Trigger the message tray by downward pressure (6.84 KB, patch)
2013-02-06 22:11 UTC, Jasper St. Pierre (not reading bugmail)
committed Details | Review
barrier: fix fallback for unsupported servers (1.51 KB, patch)
2013-02-08 23:34 UTC, darkxst
none Details | Review
barrier: fix fallback for unsupported servers (1.51 KB, patch)
2013-02-08 23:38 UTC, darkxst
none Details | Review
barrier: fix fallback for unsupported servers (991 bytes, patch)
2013-02-08 23:57 UTC, darkxst
committed Details | Review

Description Allan Day 2012-05-31 17:46:18 UTC
There's been plenty of issues with the message tray hot corner (see bug 665819). It also won't work on touch screen devices, and doesn't have a strong conceptual connection with the tray as something that occupies the whole of the bottom screen edge.

The new design [1] includes a new way to open the message tray - by exerting downward pressure on the bottom screen edge with the pointer. In the future, we could add a touchscreen gesture which mirrors this action.

[1] https://live.gnome.org/GnomeShell/Design/Guidelines/MessageTray/#A3.6_Refresh_Proposal
Comment 1 Jasper St. Pierre (not reading bugmail) 2012-05-31 17:53:40 UTC
This will require a new X server feature like pointer barriers.
Comment 2 Allan Day 2012-05-31 19:35:12 UTC
(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. :)
Comment 3 Jasper St. Pierre (not reading bugmail) 2012-07-08 20:27:47 UTC
(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.
Comment 4 Matthias Clasen 2012-07-10 12:40:41 UTC
http://git.gnome.org/browse/mutter/log/?h=wip/xinput2 ?
Comment 5 Jasper St. Pierre (not reading bugmail) 2012-07-10 17:38:02 UTC
Well yes, but that's from a while ago. It hasn't been rebased. There's probably lots of merge conflicts everywhere. :)
Comment 6 Matthias Clasen 2012-07-10 23:05:21 UTC
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
Comment 7 Matthias Clasen 2012-07-10 23:26:42 UTC
for reference, here's the 'barrier thresholds' api that was discussed:

http://patchwork.freedesktop.org/patch/10402/
Comment 8 Debarshi Ray 2012-07-18 15:30:44 UTC
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
Comment 9 Jasper St. Pierre (not reading bugmail) 2012-07-30 22:18:16 UTC
Created attachment 219936 [details] [review]
display: Require XFixes 5.0
Comment 10 Jasper St. Pierre (not reading bugmail) 2012-07-30 22:18:23 UTC
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.
Comment 11 Jasper St. Pierre (not reading bugmail) 2012-07-30 22:18:40 UTC
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.
Comment 12 Jasper St. Pierre (not reading bugmail) 2012-07-30 22:20:22 UTC
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.
Comment 13 Jasper St. Pierre (not reading bugmail) 2012-07-30 22:20:29 UTC
Created attachment 219940 [details] [review]
barrier: Update to current XFixes 6.0 revision
Comment 14 Jasper St. Pierre (not reading bugmail) 2012-07-30 22:21:30 UTC
Created attachment 219941 [details] [review]
layout: Port to the new mutter-based barrier wrappers

... and remove our old ones.
Comment 15 Jasper St. Pierre (not reading bugmail) 2012-07-30 22:21:34 UTC
Created attachment 219942 [details] [review]
messageTray: Trigger the tray by downward pressure
Comment 16 Jasper St. Pierre (not reading bugmail) 2012-07-30 22:29:45 UTC
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.
Comment 17 Giovanni Campagna 2012-08-03 17:33:04 UTC
Review of attachment 219936 [details] [review]:

Meh... It's not that with XFixes 5.0 but no XDamage, XComposite or XShape Mutter works well...
Comment 18 Jasper St. Pierre (not reading bugmail) 2012-08-03 17:35:18 UTC
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.
Comment 19 Giovanni Campagna 2012-08-03 17:35:22 UTC
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.
Comment 20 Jasper St. Pierre (not reading bugmail) 2012-08-03 17:36:07 UTC
Review of attachment 219936 [details] [review]:

Oh, and if Florian/Owen say so, I'd gladly hard-depend on all of those.
Comment 21 Giovanni Campagna 2012-08-03 17:38:58 UTC
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.
Comment 22 Giovanni Campagna 2012-08-03 17:39:55 UTC
Review of attachment 219940 [details] [review]:

Squash.

(And kill the GenericEvent cookie stuff in the internal API, if not needed)
Comment 23 Giovanni Campagna 2012-08-03 17:40:57 UTC
Review of attachment 219941 [details] [review]:

Seems a nice cleanup.
Comment 24 Giovanni Campagna 2012-08-03 17:42:01 UTC
Review of attachment 219942 [details] [review]:

Ok, assuming it works.
Comment 25 Jasper St. Pierre (not reading bugmail) 2012-08-03 17:42:19 UTC
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 ().
Comment 26 Jasper St. Pierre (not reading bugmail) 2012-08-03 17:44:15 UTC
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 :)
Comment 27 Debarshi Ray 2012-08-07 17:20:29 UTC
(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
Comment 28 Allan Day 2012-11-02 10:18:27 UTC
*** Bug 687386 has been marked as a duplicate of this bug. ***
Comment 29 Florian Müllner 2012-11-02 14:44:13 UTC
*** Bug 687445 has been marked as a duplicate of this bug. ***
Comment 30 Jasper St. Pierre (not reading bugmail) 2012-12-14 07:32:39 UTC
Created attachment 231542 [details] [review]
display: Require XFixes 5.0

Rebased; not sure if there are any major changes.
Comment 31 Jasper St. Pierre (not reading bugmail) 2012-12-14 07:34:30 UTC
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.
Comment 32 Jasper St. Pierre (not reading bugmail) 2012-12-14 07:37:07 UTC
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.
Comment 33 Jasper St. Pierre (not reading bugmail) 2012-12-14 07:38:48 UTC
Created attachment 231545 [details] [review]
layout: Port to the new mutter-based barrier wrappers

... and remove our old ones.
Comment 34 Jasper St. Pierre (not reading bugmail) 2012-12-14 07:39:03 UTC
Created attachment 231546 [details] [review]
layout: Trigger the message tray by downward pressure
Comment 35 Jasper St. Pierre (not reading bugmail) 2012-12-14 07:39:22 UTC
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.
Comment 36 Jasper St. Pierre (not reading bugmail) 2012-12-14 07:39:43 UTC
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.
Comment 37 Jasper St. Pierre (not reading bugmail) 2013-01-16 22:21:17 UTC
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.
Comment 38 Jasper St. Pierre (not reading bugmail) 2013-01-16 22:21:23 UTC
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.
Comment 39 Jasper St. Pierre (not reading bugmail) 2013-01-23 22:23:30 UTC
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.
Comment 40 Jasper St. Pierre (not reading bugmail) 2013-01-23 22:23:37 UTC
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.
Comment 41 Jasper St. Pierre (not reading bugmail) 2013-01-23 22:23:42 UTC
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.
Comment 42 Jasper St. Pierre (not reading bugmail) 2013-01-24 20:57:56 UTC
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.
Comment 43 Jasper St. Pierre (not reading bugmail) 2013-01-24 20:58:01 UTC
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.
Comment 44 Jasper St. Pierre (not reading bugmail) 2013-01-24 20:58:05 UTC
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.
Comment 45 Jasper St. Pierre (not reading bugmail) 2013-01-24 20:58:09 UTC
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.
Comment 46 Jasper St. Pierre (not reading bugmail) 2013-01-24 20:58:13 UTC
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.
Comment 47 Jasper St. Pierre (not reading bugmail) 2013-01-24 20:58:17 UTC
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 48 Owen Taylor 2013-01-28 18:42:17 UTC
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'
Comment 49 Owen Taylor 2013-01-28 19:19:03 UTC
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.)
Comment 50 Owen Taylor 2013-01-28 19:25:04 UTC
Review of attachment 234260 [details] [review]:

I'm OK with this
Comment 51 Owen Taylor 2013-01-28 19:44:56 UTC
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.)
Comment 52 Owen Taylor 2013-01-28 19:51:11 UTC
Review of attachment 234333 [details] [review]:

Yep
Comment 53 Owen Taylor 2013-01-28 20:07:07 UTC
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.
Comment 54 Owen Taylor 2013-01-28 20:18:41 UTC
Review of attachment 234335 [details] [review]:

OK
Comment 55 Owen Taylor 2013-01-28 20:45:38 UTC
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.
Comment 56 Owen Taylor 2013-01-28 20:47:24 UTC
Review of attachment 234337 [details] [review]:

If it feels right, it is right...
Comment 57 Owen Taylor 2013-01-28 20:51:46 UTC
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.
Comment 58 Frederic Peters 2013-01-28 21:08:36 UTC
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?
Comment 59 Matthias Clasen 2013-02-01 11:27:07 UTC
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) ?
Comment 60 Jasper St. Pierre (not reading bugmail) 2013-02-01 18:03:42 UTC
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.
Comment 61 Jasper St. Pierre (not reading bugmail) 2013-02-01 18:04:09 UTC
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.
Comment 62 Jasper St. Pierre (not reading bugmail) 2013-02-01 18:11:50 UTC
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 63 Jasper St. Pierre (not reading bugmail) 2013-02-01 18:14:32 UTC
Comment on attachment 235010 [details] [review]
display: Require XFixes 5.0

Attachment 235010 [details] pushed as 7105555 - display: Require XFixes 5.0
Comment 64 Jasper St. Pierre (not reading bugmail) 2013-02-01 18:14:54 UTC
Comment on attachment 234333 [details] [review]
layout: Remove an unused variable

Attachment 234333 [details] pushed as 7798da8 - layout: Remove an unused variable
Comment 65 Jasper St. Pierre (not reading bugmail) 2013-02-01 18:15:16 UTC
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.
Comment 66 Jasper St. Pierre (not reading bugmail) 2013-02-01 18:15:40 UTC
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.
Comment 67 Jasper St. Pierre (not reading bugmail) 2013-02-01 18:15:54 UTC
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.
Comment 68 Jasper St. Pierre (not reading bugmail) 2013-02-01 18:16:09 UTC
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.
Comment 69 Jasper St. Pierre (not reading bugmail) 2013-02-01 18:16:18 UTC
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.
Comment 70 Owen Taylor 2013-02-05 19:33:34 UTC
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.
Comment 71 Owen Taylor 2013-02-05 19:46:43 UTC
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.
Comment 72 Jasper St. Pierre (not reading bugmail) 2013-02-05 19:57:46 UTC
(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.
Comment 73 Jasper St. Pierre (not reading bugmail) 2013-02-05 20:00:20 UTC
(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...
Comment 74 Owen Taylor 2013-02-05 20:00:30 UTC
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)
Comment 75 Owen Taylor 2013-02-05 20:02:18 UTC
Review of attachment 235018 [details] [review]:

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

Never called!
Comment 76 Owen Taylor 2013-02-05 22:22:43 UTC
(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.
Comment 77 Jasper St. Pierre (not reading bugmail) 2013-02-06 22:10:25 UTC
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.
Comment 78 Jasper St. Pierre (not reading bugmail) 2013-02-06 22:10:38 UTC
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.
Comment 79 Jasper St. Pierre (not reading bugmail) 2013-02-06 22:10:45 UTC
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.
Comment 80 Jasper St. Pierre (not reading bugmail) 2013-02-06 22:11:23 UTC
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.
Comment 81 Jasper St. Pierre (not reading bugmail) 2013-02-06 22:11:27 UTC
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.
Comment 82 Jasper St. Pierre (not reading bugmail) 2013-02-06 22:11:32 UTC
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.
Comment 83 Owen Taylor 2013-02-08 18:02:50 UTC
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)
Comment 84 Owen Taylor 2013-02-08 18:04:22 UTC
Review of attachment 235337 [details] [review]:

OK
Comment 85 Owen Taylor 2013-02-08 18:05:17 UTC
Review of attachment 235338 [details] [review]:

OK
Comment 86 Owen Taylor 2013-02-08 18:10:22 UTC
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.
Comment 87 Jasper St. Pierre (not reading bugmail) 2013-02-08 18:15:33 UTC
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?
Comment 88 Owen Taylor 2013-02-08 18:52:03 UTC
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.
Comment 89 Owen Taylor 2013-02-08 18:56:50 UTC
(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.
Comment 90 Jasper St. Pierre (not reading bugmail) 2013-02-08 19:26:33 UTC
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
Comment 91 Jasper St. Pierre (not reading bugmail) 2013-02-08 19:29:25 UTC
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.
Comment 92 darkxst 2013-02-08 23:34:11 UTC
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.
Comment 93 darkxst 2013-02-08 23:38:55 UTC
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.
Comment 94 darkxst 2013-02-08 23:57:18 UTC
Created attachment 235560 [details] [review]
barrier: fix fallback for unsupported servers

add missing ifdef HAVE_XI23.
Comment 95 darkxst 2013-02-09 00:28:02 UTC
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
Comment 96 Marco Scannadinari 2013-02-11 18:25:43 UTC
It would be nice to have some sort of visual indication on how much pressure is applied, similar to Android.