Hi Chris, In case you get bored with the long weekend, I want to make sure we utilize those precious brain cells :).
You can test/repatch the "[PATCH 3/3] Add two finger gestures for Bamboo with touch" now. As for the "[PATCH 1/3] Change b to button and oldButton to oldOption", you can either ignore it or update it as Peter suggested (somehow I feel less eager to update it. The patch is irrelevant to PATCH 3 anyway.). Thank you, Chris. I am looking forward to your progress. Ping On Thu, May 27, 2010 at 3:26 PM, Peter Hutterer <[email protected]> wrote: > On Thu, May 27, 2010 at 08:34:31AM -0700, Ping Cheng wrote: >> Thank you for the review (for both patches). My reply is inline as usual. >> >> Ping >> >> On Wed, May 26, 2010 at 11:08 PM, Peter Hutterer >> <[email protected]> wrote: >> > On Tue, May 25, 2010 at 08:07:58PM -0700, Ping Cheng wrote: >> >> Change b to button and oldButton to oldOption to avoid confusion. >> > >> >> Signed-off-by: Ping Cheng <[email protected]> >> >> --- >> >> src/wcmValidateDevice.c | 32 ++++++++++++++++---------------- >> >> 1 files changed, 16 insertions(+), 16 deletions(-) >> >> >> >> diff --git a/src/wcmValidateDevice.c b/src/wcmValidateDevice.c >> >> index 438cf38..c46043f 100644 >> >> --- a/src/wcmValidateDevice.c >> >> +++ b/src/wcmValidateDevice.c >> >> @@ -385,8 +385,8 @@ int wcmParseOptions(LocalDevicePtr local, int >> >> hotplugged) >> >> { >> >> WacomDevicePtr priv = (WacomDevicePtr)local->private; >> >> WacomCommonPtr common = priv->common; >> >> - char *s, b[12]; >> >> - int i, oldButton; >> >> + char *s, button[12]; >> >> + int i, oldOption; >> > >> > >> > also, I'd suggest moving b down (see below). >> >> We are on the same page for this one. I had the same idea when I made >> the change. However, if we move it inside the for-loop, it would be >> declared WCM_MAX_BUTTONS times. I don't like that. > > the compiler optimises things like that away so this has little effect on > the code other than improved error checking at compile- and debug time. > > generally I find that relying on the compiler for micro-optimization like > this is much more reliable than relying on myself :) > >> >> WacomToolPtr tool = NULL; >> >> WacomToolAreaPtr area = NULL; >> >> >> >> @@ -570,12 +570,12 @@ int wcmParseOptions(LocalDevicePtr local, int >> >> hotplugged) >> >> if (TabletHasFeature(common, WCM_TPC)) >> >> common->wcmTPCButtonDefault = 1; >> >> >> >> - oldButton = xf86SetBoolOption(local->options, "TPCButton", >> >> + oldOption = xf86SetBoolOption(local->options, "TPCButton", >> >> common->wcmTPCButtonDefault); >> > >> > personally, I find oldOption to be more confusing than oldButton. How about >> > just tpc_button? >> >> It is not the button that matters. It is what the variable really >> represent that matters here. The oldBuuton is also used for: >> >> --------------------------------------------- >> - oldButton = xf86SetBoolOption(local->options, "Capacity", >> + oldOption = xf86SetBoolOption(local->options, "Capacity", >> common->wcmCapacityDefault); >> >> if (hotplugged || IsTouch(priv)) >> - common->wcmCapacity = oldButton; >> - else if (oldButton != common->wcmCapacity) >> + common->wcmCapacity = oldOption; >> + else if (oldOption != common->wcmCapacity) >> xf86Msg(X_WARNING, "%s: Touch Capacity option >> can only be" >> "set by a touch tool.\n", local->name); >> } >> @@ -612,12 +612,12 @@ int wcmParseOptions(LocalDevicePtr local, int >> hotplugged) >> * except when multi-touch is supported */ >> common->wcmGestureDefault = 1; >> >> - oldButton = xf86SetBoolOption(local->options, "Gesture", >> + oldOption = xf86SetBoolOption(local->options, "Gesture", >> common->wcmGestureDefault); >> ------------------------------------------ >> >> They are the reason that I made the change. Unless you'd like to >> introduce another variable for those two cases, it is confusing either >> way. For me, no matter they are buttons or Gesture/Capacity, they are >> all options in general. > > urgh, yes. please add a separate variable for each. it helps reading the > code and as with the above, the compiler will probably squash them together > into one anyway. > > thanks. > > Cheers, > Peter > ------------------------------------------------------------------------------ _______________________________________________ Linuxwacom-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/linuxwacom-devel
