On 04/30/2012 06:19 PM, Peter Hutterer wrote: > On Mon, Apr 30, 2012 at 10:45:30AM -0700, Chase Douglas wrote: >> On 04/29/2012 08:21 PM, Peter Hutterer wrote: >>> SLOTSTATE_OPEN_EMPTY on resume leads to erroneously detected touches. >>> >>> Signed-off-by: Peter Hutterer <[email protected]> >>> --- >>> src/eventcomm.c | 2 +- >>> src/synaptics.c | 6 +++--- >>> src/synproto.c | 6 +++--- >>> src/synproto.h | 2 +- >>> 4 files changed, 8 insertions(+), 8 deletions(-) >>> >>> diff --git a/src/eventcomm.c b/src/eventcomm.c >>> index 741f988..4ef8ad6 100644 >>> --- a/src/eventcomm.c >>> +++ b/src/eventcomm.c >>> @@ -662,7 +662,7 @@ EventReadHwState(InputInfoPtr pInfo, >>> SynapticsParameters *para = &priv->synpara; >>> struct eventcomm_proto_data *proto_data = priv->proto_data; >>> >>> - SynapticsResetTouchHwState(hw); >>> + SynapticsResetTouchHwState(hw, FALSE); >>> >>> /* Reset cumulative values if buttons were not previously pressed */ >>> if (!hw->left && !hw->right && !hw->middle) >>> diff --git a/src/synaptics.c b/src/synaptics.c >>> index 935650d..6dc8004 100644 >>> --- a/src/synaptics.c >>> +++ b/src/synaptics.c >>> @@ -1619,7 +1619,7 @@ timerFunc(OsTimerPtr timer, CARD32 now, pointer arg) >>> >>> priv->hwState->millis += now - priv->timer_time; >>> SynapticsCopyHwState(hw, priv->hwState); >>> - SynapticsResetTouchHwState(hw); >>> + SynapticsResetTouchHwState(hw, FALSE); >>> delay = HandleState(pInfo, hw, hw->millis, TRUE); >>> >>> priv->timer_time = now; >>> @@ -1659,7 +1659,7 @@ ReadInput(InputInfoPtr pInfo) >>> int delay = 0; >>> Bool newDelay = FALSE; >>> >>> - SynapticsResetTouchHwState(hw); >>> + SynapticsResetTouchHwState(hw, FALSE); >>> >>> while (SynapticsGetHwState(pInfo, priv, hw)) { >>> /* Semi-mt device touch slots do not track touches. When there is a >>> @@ -3017,7 +3017,7 @@ UpdateTouchState(InputInfoPtr pInfo, struct >>> SynapticsHwState *hw) >>> } >>> } >>> >>> - SynapticsResetTouchHwState(hw); >>> + SynapticsResetTouchHwState(hw, FALSE); >>> #endif >>> } >>> >>> diff --git a/src/synproto.c b/src/synproto.c >>> index cf54c4d..e7534e2 100644 >>> --- a/src/synproto.c >>> +++ b/src/synproto.c >>> @@ -153,11 +153,11 @@ SynapticsResetHwState(struct SynapticsHwState *hw) >>> hw->middle = 0; >>> memset(hw->multi, 0, sizeof(hw->multi)); >>> >>> - SynapticsResetTouchHwState(hw); >>> + SynapticsResetTouchHwState(hw, TRUE); >>> } >>> >>> void >>> -SynapticsResetTouchHwState(struct SynapticsHwState *hw) >>> +SynapticsResetTouchHwState(struct SynapticsHwState *hw, Bool force_empty) >>> { >>> #ifdef HAVE_MULTITOUCH >>> int i; >>> @@ -175,7 +175,7 @@ SynapticsResetTouchHwState(struct SynapticsHwState *hw) >>> case SLOTSTATE_OPEN: >>> case SLOTSTATE_OPEN_EMPTY: >>> case SLOTSTATE_UPDATE: >>> - hw->slot_state[i] = SLOTSTATE_OPEN_EMPTY; >>> + hw->slot_state[i] = force_empty ? SLOTSTATE_EMPTY : >>> SLOTSTATE_OPEN_EMPTY; >>> break; >>> >>> default: >>> diff --git a/src/synproto.h b/src/synproto.h >>> index 7f80bcd..413579d 100644 >>> --- a/src/synproto.h >>> +++ b/src/synproto.h >>> @@ -120,7 +120,7 @@ extern void SynapticsHwStateFree(struct >>> SynapticsHwState **hw); >>> extern void SynapticsCopyHwState(struct SynapticsHwState *dst, >>> const struct SynapticsHwState *src); >>> extern void SynapticsResetHwState(struct SynapticsHwState *hw); >>> -extern void SynapticsResetTouchHwState(struct SynapticsHwState *hw); >>> +extern void SynapticsResetTouchHwState(struct SynapticsHwState *hw, Bool >>> force_empty); >>> >>> extern Bool SynapticsIsSoftButtonAreasValid(int *values); >>> >> >> This looks functionally correct, but I don't love the extra force_empty >> parameter. There's only one case where we want to force empty slots. >> What if instead we make this a one-line change in SynapticsResetHwState >> after the call to SynapticsResetTouchHwState: >> >> /* Force all touches to the empty, closed state */ >> memset(hw->slot_state, 0, hw->num_mt_mask * sizeof(enum SynapticsSlotState); > > I'm not a fan of this. If you reset, you shouldn't need code outside the > caller to re-set some arrays again. We already have a reset_hw_state() that > does something else again. (eventually) having one function that resets the > struct is a good thing, even if the type of reset is controlled by some > parameters.
That's a fair point. I'm still not crazy about "force_empty" as it sounds like a hack due to "force" and it's not clear what are we emptying. Maybe rename it to "close_touches" or "end_touches"? It's bike shedding though, so either way: Reviewed-by: Chase Douglas <[email protected]> -- Chase _______________________________________________ [email protected]: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
