Bug 21613

Please attach a photo of that touchpad, I'd like to see the physical layout.
Also, what laptop brand and models are affected by this?

If you click on the Gallery you will see the photos of the Dell mini 10 laptop:
http://www.dell.com/content/products/productdetails.aspx/laptop-inspiron-10?c=us&cs=19&l=en&s=dhs

To be more specific, the model is Dell Mini 10 v.

Any updates, comments, etc. on this?

CC'ing henrik and christoph for extra input.

As mentioned on IRC, I'm hesitant to put in properties in for broken hardware. Properties need to be maintained and we have too many already IMO.

Please attach the evtest output when you press each button once.

I am not completely opposed to the idea, but I have a feeling it can be implemented simpler with existing logic. There are already parameters to restrict the movement area to an arbitrary rectangular part of the trackpad. Any chance it could be used to solve your problem?

Henrik
 

Created attachment 26722 [details]
evtest output for the left button

Created attachment 26723 [details]
evtest output for the right button

@Henrik
If you're referring to "BottomEdge", that's different from the physical bottom edge and seems to influence only horizontal edge scrolling (instead of movement in general).

If you're referring to something else, can you tell me what those parameters are, please?

@Peter
I have attached the output as you requested.

> If you're referring to "BottomEdge", that's different from the physical bottom
> edge and seems to influence only horizontal edge scrolling (instead of movement
> in general).

Yes, it is currently affecting edge scrolling, but if it could also affect movement
in general, would it not solve your problem?

What if a user/vendor wants to enable and use horizontal edge scrolling (so that scrolling works in an area above the blacklisted one) and disable the area with the buttons (with MovementBottomEdge) at the same time?

In my opinion this is another use case to take into consideration.

What do you think?

I don't think we can use the edges for this feature. The edges have too much
semantic behaviour that we shouldn't break.

A move that starts inside and crosses an edge does not generate scroll events.
If edge scrolling is disabled, the edge settings are effectively ignored. Plus
there is the circular scrolling and corner-coasting interaction that also
complicates things.

This bug seems to affect all Dell Minis, so it's not just a single machine's
problem.

The patch you submitted before is a good start, but I don't want this to be
just on the bottom edge. Sooner or later it will affect other edges too so we
might as well get it right now.

I propose a "Synaptics Area" optional property that allows for all 4
directions to be limited to a given coordinate value.

The semantics get a bit interesting though:
- if a finger moves from the valid to the blacklisted area, does it continue
  in this area or are all coordinates clipped?
- if a finger starts a move in the blacklisted area, does it generate events?
- If a finger taps in the blacklisted area, does it generate events?

(In reply to comment #11)
> This bug seems to affect all Dell Minis, so it's not just a single machine's
> problem.
> 
> The patch you submitted before is a good start, but I don't want this to be
> just on the bottom edge. Sooner or later it will affect other edges too so we
> might as well get it right now.
> 
> I propose a "Synaptics Area" optional property that allows for all 4
> directions to be limited to a given coordinate value.
>

I think your idea is more future-proof than mine and I look forward to working on it.
 
> The semantics get a bit interesting though:
> - if a finger moves from the valid to the blacklisted area, does it continue
>   in this area or are all coordinates clipped?

I think the coordinates should be clipped for the reasons listed below.

> - if a finger starts a move in the blacklisted area, does it generate events?

Yes but, in my opinion, the driver should ignore them as the movement could be caused by (as in the case which my patch covers) an attempt to click or to perform some other action that specific area was meant for by manufacturer.

> - If a finger taps in the blacklisted area, does it generate events?
> 

I'm not sure about this. It could make sense to disable finger taps (and finger taps in the corners?) too as we don't know what the blacklisted area is intended to do.

Of course better ideas are welcome.

clipping IMO doesn't work. given that your valid range goes from 0 - N, any coordinate beyond N would result in N. so you still get the fake motion event for the buttons.

ignoring events is more interesting but leads to jumps as the last coordinate when you move back into the valid area. not sure what the usability impact of this is.

(In reply to comment #13)
> clipping IMO doesn't work. given that your valid range goes from 0 - N, any
> coordinate beyond N would result in N. so you still get the fake motion event
> for the buttons.
> 
> ignoring events is more interesting but leads to jumps as the last coordinate
> when you move back into the valid area. not sure what the usability impact of
> this is.
> 

What I meant to say is that, for example, if the event takes place in a point below the bottom edge defined by the Synaptics Area property, we can do something similar to what my current patch does:

1) The driver updates hw->y to its real value only if the event didn't take place in the blacklisted area.

2) The driver updates hw->x only if the y event didn't take place in the blacklisted area.

I didn't notice cursor jumps when testing the case that you suggested. Furthermore you would get a jump only if hw->y and hw->x were updated when touching the blacklisted area.

As soon as you've got anything to test, I've got the Dell Mini 10 hardware with F11/rawhide. It's driving me crazy too ;)

(In reply to comment #14)
> What I meant to say is that, for example, if the event takes place in a point
> below the bottom edge defined by the Synaptics Area property, we can do
> something similar to what my current patch does:
> 
> 1) The driver updates hw->y to its real value only if the event didn't take
> place in the blacklisted area.
> 
> 2) The driver updates hw->x only if the y event didn't take place in the
> blacklisted area.
> 
> I didn't notice cursor jumps when testing the case that you suggested.
> Furthermore you would get a jump only if hw->y and hw->x were updated when
> touching the blacklisted area.

the scenario I was thinking of is that you move into the right bottom corner, then along in the blacklisted area to the left bottom corner and then up into the allowed area. This would generate a jump the length of the touchpad's width.

We can work around that though. Since by default the touchpad is in relative mode anyway, the only thing that should be capped here are the actual posts of the motion event, not the internal state. So at any point in time, the movement history contains the actual values - including the blacklisted area.

For your patch, this means that you hook in before posting the event and reset dx/dy to zero for blacklisted areas. I think that should do the job.
This simple diff shows what I mean:

iff --git a/src/synaptics.c b/src/synaptics.c
index a40d160..40ee7f2 100644
--- a/src/synaptics.c
+++ b/src/synaptics.c
@@ -2230,6 +2230,9 @@ HandleState(LocalDevicePtr local, struct SynapticsHwState *hw)
            buttons |= tap_mask;
     }
 
+    /* FIXME: HACK: */
+    if (edge) dx = dy = 0;
+
     /* Post events */
     if (dx || dy)
        xf86PostMotionEvent(local->dev, 0, 0, 2, dx, dy);

If the finger triggers an edge, no motion event is posted (i.e. the edges form the blacklisted area).

(In reply to comment #16)
> 
> the scenario I was thinking of is that you move into the right bottom corner,
> then along in the blacklisted area to the left bottom corner and then up into
> the allowed area. This would generate a jump the length of the touchpad's
> width.
> 
> We can work around that though. Since by default the touchpad is in relative
> mode anyway, the only thing that should be capped here are the actual posts of
> the motion event, not the internal state. So at any point in time, the movement
> history contains the actual values - including the blacklisted area.
> 
> For your patch, this means that you hook in before posting the event and reset
> dx/dy to zero for blacklisted areas. I think that should do the job.
> 
> If the finger triggers an edge, no motion event is posted (i.e. the edges form
> the blacklisted area).
> 

Ok, I see what you mean now.

I think I can start working on a prototype now.

Created attachment 27410 [details] [review]
First draft of the patch

The attached patch is a just first draft and I haven't touched the man page yet.

HOW IT WORKS:
When values other than 0 are assigned to the "Synaptics Area" property, taps, scrolling and movements are allowed only in the "Synaptics Area".

For example, if I set the bottom edge of the area to 3700:
xinput set-int-prop $YOUR_DEVICE_ID "Synaptics Area" 32 0 0 0 3700

the active area will be a rectangle which has the top, left and right physical edges and the bottom edge of the area (3700) as its limits. Of course it is also possible to use custom values for all the 4 edges of the area so as to create an area whose borders don't match with the physical ones.

NOTE: The line which begins with "if ((dx || dy) && (is_inside_active_area" is necessary only if we disable taps, in order to prevent the cursor from jumping. Maybe there's a better way to disable taps (?).

Let me know what you think.

thanks for the patch! here's a few comments:
- style: no spaces after negations. for example, if (! inside_active_area) should be if (!inside_active_area)

- don't use 0 as the "no range" marker. there might be touchpads (one day) that have negative ranges. In the server we use valid ranges instead, i.e. (min >= max) designates an unset range.

- one could argue that if the range is user-configured anyway we don't need to query for it (eventcomm.c).

- second-to-last hunk is whitespace only hunk, please remove


I'm a bit confused abut the comment above the is_inside_active_area(.. HIST(0)) part. can you paraphrase this please?

Created attachment 27494 [details] [review]
Second draft of the patch

(In reply to comment #19)
> thanks for the patch! here's a few comments:
> - style: no spaces after negations. for example, if (! inside_active_area)
> should be if (!inside_active_area)
> 
Ok, fixed.

> - don't use 0 as the "no range" marker. there might be touchpads (one day) that
> have negative ranges. In the server we use valid ranges instead, i.e. (min >=
> max) designates an unset range.
> 
Ok, now if the edges of the area match the physical ones (priv->minx, etc.) the property is unset. 

> - one could argue that if the range is user-configured anyway we don't need to
> query for it (eventcomm.c).
> 
Good point. I got rid of it

> - second-to-last hunk is whitespace only hunk, please remove
> 
There's also a curly brace there.

> 
> I'm a bit confused abut the comment above the is_inside_active_area(.. HIST(0))
> part. can you paraphrase this please?
> 
That approach was wrong and I've come up with a better (and simpler) solution i.e. I set priv->tap_button to 0 in HandleTapProcessing(). Now the cursor doesn't jump and taps are disabled in the area.

Furthermore I have added an entry in the man page and added support for the new property in synclient.

The new patch seems to work for me without problems. More testing is welcome though.

Instructions for testing:
The physical edges are the default values of the area (the following are Dell Mini 10V's):
1472 5472 1408 4448

Set the bottom edge of the area to 4000 (as maybe 3700 was too much for the Dell mini) with the following command:
xinput set-int-prop $YOUR_DEVICE_ID "Synaptics Area" 32 1472 5472 1408 4000

Both scrolling and tapping are disabled outside of the active area and the cursor won't move when you perform a click.


What do you think?

(In reply to comment #20)
> > - don't use 0 as the "no range" marker. there might be touchpads (one day) that
> > have negative ranges. In the server we use valid ranges instead, i.e. (min >=
> > max) designates an unset range.
> > 
> Ok, now if the edges of the area match the physical ones (priv->minx, etc.) the
> property is unset. 

Still not quite sure about this. The reason is that the kernel doesn't
actually guaranteed a min/max range so it can be confusing during debugging
whether a value is supposed to be sent now or not. 
I suggest to add another variable to the private rec that contains flags if an
area is set for a given edge. (Have a look at
afb60a0b2497c5d08cbd1739fa8ae6585c428881 for an example).

The initial value of the property can be empty (which means the property
handler must allow a prop->size 0 to unset). Again, this is an explicit signal
that no edges are set. The question is however how we then signal through the
property if only one edge is actually set.

Any suggestions?

> > I'm a bit confused abut the comment above the is_inside_active_area(.. HIST(0))
> > part. can you paraphrase this please?
> > 
> That approach was wrong and I've come up with a better (and simpler) solution
> i.e. I set priv->tap_button to 0 in HandleTapProcessing(). Now the cursor
> doesn't jump and taps are disabled in the area.

wouldn't it be easier to only call HandleTapProcessing conditional to
!inside_active_area? or does that mess with the state?

> Furthermore I have added an entry in the man page and added support for the new
> property in synclient.

the man page entry is a bit brief, please explain what the options actually
do (and the property as well).

(In reply to comment #21)
> (In reply to comment #20)
> > > - don't use 0 as the "no range" marker. there might be touchpads (one day) that
> > > have negative ranges. In the server we use valid ranges instead, i.e. (min >=
> > > max) designates an unset range.
> > > 
> > Ok, now if the edges of the area match the physical ones (priv->minx, etc.) the
> > property is unset. 
> 
> Still not quite sure about this. The reason is that the kernel doesn't
> actually guaranteed a min/max range so it can be confusing during debugging
> whether a value is supposed to be sent now or not. 
> I suggest to add another variable to the private rec that contains flags if an
> area is set for a given edge. (Have a look at
> afb60a0b2497c5d08cbd1739fa8ae6585c428881 for an example).
> 
> The initial value of the property can be empty (which means the property
> handler must allow a prop->size 0 to unset). Again, this is an explicit signal
> that no edges are set. The question is however how we then signal through the
> property if only one edge is actually set.
> 
> Any suggestions?
> 

Why don't we just modify what I wrote in the 1st draft?

Let's consider is_inside_active_area():
In the 1st draft I tested priv->synpara.area_left_edge > 0 but I could simply test that priv->synpara.area_left_edge != 0 (etc.) so as to allow negative ranges:

if ((priv->synpara.area_left_edge != 0) && (x < priv->synpara.area_left_edge))

etc.

And of course I would have to modify SetProperty() (properties.c) accordingly:

if ((((area[0] != 0) && (area[1] != 0)) && (area[0] > area[1]) ) || (((area[2] != 0) && (area[3] != 0)) && (area[2] > area[3])))

This way things should work as you suggested and we won't need additional variables in the private rec.

What do you think?

> wouldn't it be easier to only call HandleTapProcessing conditional to
> !inside_active_area? or does that mess with the state?
> 
That's the first thing that I tried but it made the touchpad unusable.


> the man page entry is a bit brief, please explain what the options actually
> do (and the property as well).
> 
Sure, I wasn't sure about how verbose the man page could be.

Created attachment 27726 [details] [review]
Third draft of the patch

The attached patch contains the latest changes described in the previous comment.

Let me know what you think.

(In reply to comment #23)
> Created an attachment (id=27726) [details]
> Third draft of the patch
> 
> The attached patch contains the latest changes described in the previous
> comment.
> 
> Let me know what you think.
> 

The code is clear and constitutes a good patch. I did not read all the details of the discussion, so I might have missed some important point, but the area parameters constitutes the physical area of the device and could be filled with default values based on the reported driver dimensions?

one minor fix: please shift the hunk encompassing the scroll.up while and the other loops  by 4 spaces. It'll make the patch appear larger, but now that we've reviewed it several times that shouldn't be an issue.

Attach the final patch as git-formatted patch please, makes it easier to apply.

Henrik: see comment #21 for the explanation why min/max is not really suitable.

Created attachment 27758 [details] [review]
Final version of the patch

The attached patch is git-formatted.

I indented that block of code as Peter suggested, furthermore I have updated the comment about is_inside_active_area() so that it matches the current behaviour.

Let me know if there's something else that I should do.

Pushed as 7179a0eb11a842d9d5a420f5702a411b0dc217a2. Thanks for your work.

I just git checkout/compiled and must say that this patch is not yet working correctly :(

I have a macbook pro here and this machine also does not have a visible mouse button, but only one below the touchpad.

When I use my thumb to click at the very bottom of the pad and then my index finger to move the mouse cursor the pad will simply say: Two fingers detected I will do twofinger scrolling. This happens even with AreaBottomEdge defined leaving enough room for my thumb.

I just checked how OSX handles this and indeed as soon as I click below some AreaBottomEdge (say vertically 650-800) the finger is not counted - so here my index finger works nice for moving the mouse.

So I would propose to do the same here: Either substract the number of fingers by one or ignore finger presses in that area (whatever is easier).

Agreed, this seems to be the better behaviour. Can you help us out with a
patch?

(In reply to comment #28)
> I just git checkout/compiled and must say that this patch is not yet working
> correctly :(
> 
> I have a macbook pro here and this machine also does not have a visible mouse
> button, but only one below the touchpad.
> 
> When I use my thumb to click at the very bottom of the pad and then my index
> finger to move the mouse cursor the pad will simply say: Two fingers detected I
> will do twofinger scrolling. This happens even with AreaBottomEdge defined
> leaving enough room for my thumb.
> 
> I just checked how OSX handles this and indeed as soon as I click below some
> AreaBottomEdge (say vertically 650-800) the finger is not counted - so here my
> index finger works nice for moving the mouse.
> 
> So I would propose to do the same here: Either substract the number of fingers
> by one or ignore finger presses in that area (whatever is easier).
> 

Ok, it looks like I neglected multi-touch touchpads.

What happens in OSX when you use you thumb to click and then put two fingers outside of the AreaBottomEdge (i.e. its equivalent in OSX) and move them? Do you get two finger scrolling? What happens with 3 fingers (if it makes any difference)?

when I hold thumb and in addition use two fingers I get two finger scrolling.

so number_of_fingers-1 :)

it just strikes me that one can do two(or three)-finger clicking in the bottom region and then use a finger above to scroll.

so I guess the correct handling would be 

corrected_number_of_fingers= total - number_of_fingers_in_bottom_region

closing this again, was pushed as outlined in comment #27 and that behaviour has been unchanged for 6 years now. if you still want the extra finger counting please open a new bug but I should point out that it's unlikely to be implement for synaptics. libinput has that feature already, probably better to use that then.