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. >> 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. > >> >> if (hotplugged || IsStylus(priv)) >> - common->wcmTPCButton = oldButton; >> - else if (oldButton != common->wcmTPCButton) >> + common->wcmTPCButton = oldOption; >> + else if (oldOption != common->wcmTPCButton) >> xf86Msg(X_WARNING, "%s: TPCButton option can only be set " >> "by stylus.\n", local->name); >> > > [...] > >> @@ -628,8 +628,8 @@ int wcmParseOptions(LocalDevicePtr local, int hotplugged) >> >> for (i=0; i<WCM_MAX_BUTTONS; i++) >> { > > + char *b[12]; > > could be declared here, that way it's local and we don't have to worry about > the naming as much since the use is just in the next line anyway. > > Cheers, > Peter > >> - sprintf(b, "Button%d", i+1); >> - priv->button[i] = xf86SetIntOption(local->options, b, >> priv->button[i]); >> + sprintf(button, "Button%d", i+1); >> + priv->button[i] = xf86SetIntOption(local->options, button, >> priv->button[i]); > >> } >> >> if (common->wcmForceDevice == DEVICE_ISDV4) >> -- >> 1.6.6.1 >> > ------------------------------------------------------------------------------ _______________________________________________ Linuxwacom-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/linuxwacom-devel
