Re: FW: [PATCH] xf86-input-wacom

[Peter Hutterer-3]

sorry, I missed this one. my spamfilter got overly excited over the last
week. thanks Ping for forwarding it.

> >From f2ac561863aa95aad03a3975e8c1813391b306e9 Mon Sep 17 00:00:00 2001
> From: Przemo Firszt <przemo@...>
> Date: Fri, 13 Nov 2009 17:53:04 +0000
> Subject: [PATCH 1/3] Remove duplicate of #include "xf86Wacom.h" and #include "wcmFilter.h"
>
> ---
>  src/xf86Wacom.c |   12 ++++++------
>  1 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/src/xf86Wacom.c b/src/xf86Wacom.c
> index 58ecef0..029da44 100755
> --- a/src/xf86Wacom.c
> +++ b/src/xf86Wacom.c
> @@ -44,16 +44,13 @@
>   */
>  
>  /****************************************************************************/
> -
> -#include "xf86Wacom.h"
> -#include "wcmFilter.h"
> -#include <sys/stat.h>
> -#include <fcntl.h>
> -
>  #ifdef HAVE_CONFIG_H
>  #include <config.h>
>  #endif
>  
> +#include <sys/stat.h>
> +#include <fcntl.h>
> +
>  #include "xf86Wacom.h"
>  #include "wcmFilter.h"
>  
> @@ -61,6 +58,9 @@
>  #include <xserver-properties.h>
>  #endif
>  
> +/*****************************************************************************
> + * Forward declaration
> + ****************************************************************************/
>  void xf86WcmVirtaulTabletPadding(LocalDevicePtr local);
>  void xf86WcmVirtaulTabletSize(LocalDevicePtr local);
>  Bool xf86WcmIsWacomDevice (char* fname);
> --
> 1.6.5.2

applied, see next comment though.

> >From 3e6878ba7288ff71fc2d64d845fd71b91d1545f8 Mon Sep 17 00:00:00 2001
> From: Przemo Firszt <przemo@...>
> Date: Fri, 13 Nov 2009 18:24:42 +0000
> Subject: [PATCH 2/3] Add InputDriverRec
>
> ---
>  src/xf86Wacom.c |   13 +++++++++++--
>  1 files changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/src/xf86Wacom.c b/src/xf86Wacom.c
> index 029da44..071b370 100755
> --- a/src/xf86Wacom.c
> +++ b/src/xf86Wacom.c
> @@ -52,8 +52,7 @@
>  #include <fcntl.h>
>  
>  #include "xf86Wacom.h"
> -#include "wcmFilter.h"
> -
> +

I'm not a big fan of changes like this in an otherwise unrelated patches.
the wcmFilter removal seems to fit into the previous patch, doesn't it? i'll
squash it in there.

>  #if GET_ABI_MAJOR(ABI_XINPUT_VERSION) >= 7
>  #include <xserver-properties.h>
>  #endif
> @@ -87,6 +86,16 @@ static Bool xf86WcmDevConvert(LocalDevicePtr local, int first, int num,
>  static Bool xf86WcmDevReverseConvert(LocalDevicePtr local, int x, int y,
>   int* valuators);
>  
> +InputDriverRec WACOM = {
> +    1,
> +    "wacom",
> +    NULL,
> +    NULL,
> +    NULL,
> +    NULL,
> +    0
> +};
> +
>  WacomModule gWacomModule =
>  {
>   NULL,           /* input driver pointer */
> --
> 1.6.5.2

 
we have this struct defined in wcmConfig.c, adding it here would duplicate
it.

> >From a99805846f3d5e41cda7834a925c9417dfa5c138 Mon Sep 17 00:00:00 2001
> From: Przemo Firszt <przemo@...>
> Date: Fri, 13 Nov 2009 18:25:37 +0000
> Subject: [PATCH 3/3] Add XF86ModuleVersionInfo
>
> ---
>  src/xf86Wacom.c |   13 +++++++++++++
>  1 files changed, 13 insertions(+), 0 deletions(-)
>
> diff --git a/src/xf86Wacom.c b/src/xf86Wacom.c
> index 071b370..0426f65 100755
> --- a/src/xf86Wacom.c
> +++ b/src/xf86Wacom.c
> @@ -96,6 +96,19 @@ InputDriverRec WACOM = {
>      0
>  };
>  
> +static XF86ModuleVersionInfo VersionRec = {
> +    "wacom",
> +    MODULEVENDORSTRING,
> +    MODINFOSTRING1,
> +    MODINFOSTRING2,
> +    XORG_VERSION_CURRENT,
> +    PACKAGE_VERSION_MAJOR, PACKAGE_VERSION_MINOR, PACKAGE_VERSION_PATCHLEVEL,
> +    ABI_CLASS_XINPUT,
> +    ABI_XINPUT_VERSION,
> +    MOD_CLASS_XINPUT,
> +    {0, 0, 0, 0}
> +};
> +
>  WacomModule gWacomModule =
>  {
>   NULL,           /* input driver pointer */
> --
> 1.6.5.2

same as with the above patch.

now, moving these structs and the associated bits over to xf86Wacom.c makes
sense in principle, though right now the distinction between what goes into
wcmConfig.c and xf86Wacom.c isn't quite clear to me. hence I don't really
see the point of moving it around for now until we've sorted out why there's
two different files for it :)

Cheers,
  Peter

------------------------------------------------------------------------------
Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day
trial. Simplify your report design, integration and deployment - and focus on
what you do best, core application coding. Discover what's new with
Crystal Reports now.  http://p.sf.net/sfu/bobj-july
_______________________________________________
Linuxwacom-devel mailing list
Linuxwacom-devel@...
https://lists.sourceforge.net/lists/listinfo/linuxwacom-devel

[przemof]

Dnia 2009-11-19, czw o godzinie 12:00 +1000, Peter Hutterer pisze:
[..]
> I'm not a big fan of changes like this in an otherwise unrelated patches.
> the wcmFilter removal seems to fit into the previous patch, doesn't it? i'll
> squash it in there.
You're right - I'll do it that way next time (same for the comment
below).
[..]
> now, moving these structs and the associated bits over to xf86Wacom.c makes
> sense in principle, though right now the distinction between what goes into
> wcmConfig.c and xf86Wacom.c isn't quite clear to me. hence I don't really
> see the point of moving it around for now until we've sorted out why there's
> two different files for it :)
Ping, do we have a guideline what should be where?

--
Regards,
Przemo


------------------------------------------------------------------------------
Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day
trial. Simplify your report design, integration and deployment - and focus on
what you do best, core application coding. Discover what's new with
Crystal Reports now.  http://p.sf.net/sfu/bobj-july
_______________________________________________
Linuxwacom-devel mailing list
Linuxwacom-devel@...
https://lists.sourceforge.net/lists/listinfo/linuxwacom-devel

[Ping Cheng-2]

2009/11/22 Przemysław Firszt <przemo@...>

Dnia 2009-11-19, czw o godzinie 12:00 +1000, Peter Hutterer pisze:
> now, moving these structs and the associated bits over to xf86Wacom.c makes
> sense in principle, though right now the distinction between what goes into
> wcmConfig.c and xf86Wacom.c isn't quite clear to me. hence I don't really
> see the point of moving it around for now until we've sorted out why there's
> two different files for it :)
Ping, do we have a guideline what should be where?

That's a good question. The short answer would be no. But that answer doesn't even make myself feel comfortable. There is a history on what we have now.  Initially, there was only one file which is xf86Wacom.c.  When John Joganic created this project about 7 years ago, he refactored the code and made it easier for us to maintain.  I guess, wcmConfig.c was intended only to deal with configuration related stuff.  Those routines that have a closer tie to XFree86, such as xf86WcmDevConvert, xf86WcmDevReverseConvert, xf86WcmDevProc, etc., stayed.  I have added a few new files while introducing new features.  With so many features and new models to support, halting the development for a whole code refactoring was something I could not afford.

 

However, this is an open source project. You can provide a guideline for us or share your suggestions with us. We are more than happy to accept any contributions to the project, especially for xf86-input-wacom, which bears our future. We want, by all means, to make it easier for others to contribue to linuxwacom project.

 

Ping

 

 

 

 

------------------------------------------------------------------------------
Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day
trial. Simplify your report design, integration and deployment - and focus on
what you do best, core application coding. Discover what's new with
Crystal Reports now.  http://p.sf.net/sfu/bobj-july
_______________________________________________
Linuxwacom-devel mailing list
Linuxwacom-devel@...
https://lists.sourceforge.net/lists/listinfo/linuxwacom-devel

[przemof]

Dnia 2009-11-22, nie o godzinie 14:05 -0800, Ping pisze:
> However, this is an open source project. You can provide a guideline
> for us or share your suggestions with us. We are more than happy to
> accept any contributions to the project, especially for
> xf86-input-wacom, which bears our future. We want, by all means, to
> make it easier for others to contribue to linuxwacom project.

OK, I've some ideas but I need to polish them first.
--
Przemo



------------------------------------------------------------------------------
Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day
trial. Simplify your report design, integration and deployment - and focus on
what you do best, core application coding. Discover what's new with
Crystal Reports now.  http://p.sf.net/sfu/bobj-july
_______________________________________________
Linuxwacom-devel mailing list
Linuxwacom-devel@...
https://lists.sourceforge.net/lists/listinfo/linuxwacom-devel

[przemof]

Dnia 2009-11-22, nie o godzinie 14:05 -0800, Ping pisze:

> However, this is an open source project. You can provide a guideline
> for us or share your suggestions with us. We are more than happy to
> accept any contributions to the project, especially for
> xf86-input-wacom, which bears our future. We want, by all means, to
> make it easier for others to contribue to linuxwacom project.

OK, here is first sketch of proposed changes in the structure of the
driver:

wcmCommon.c -  contains all funcions required for handling a tablet
(receiving, handling, sending events), but not for
initializing/opening/closing/configuring tablet or manipulating data).
xf86WcmOpen should be in xf86Wacom.c

wcmCompat.c - DELETE. xf86WcmWait to be moved to wcmISDV4.c (it's used
only there) xf86WcmReady be moved to wcmWacom.c (it's used only there)

wcmConfig.c - allocating device, initialising, plugging, unplugging,
uninitialising. xf86WcmPointInArea, xf86WcmAreasOverlap,
xf86WcmAreaListOverlap should be moved to wcmCommon

wcmFilter.c - data filtering (like pressure curve functions or jitter
correction, tilt2rotation)

wcmFilter.h - header file for wcmFilter.c

wcmISDV4.c - Wacom IV protocol specific functions

wcmTilt2Rotation.c - DELETE. Function xf86WcmTilt2R to be moved to
wcmFilter, tiltTable to go to wcmFilter.h

wcmUSB.c - USB specific functions. WacomModel definitions to be moved to
a new wcmUSB.h header file. Another option is to create wcmConfig.h and
drop all USB and ISDV4 definitions there.

wcmValidateDevice.c - probing, parsing, checking for duplicate device

wcmXCommand.c - Mode switching and screen changing functions,
xf86WcmSetProperty and InitWcmDeviceProperties to be moved to
xf86Config.c

xf86Wacom.c - opening, closing device, change control. Option 2: merge
xf86Wacom with wcmConfig. xf86WcmRegisterX11Devices to be moved to
wcmConfig.c and xf86WcmIsWacomDevice to wcmValidateDevice.c

My goal is to create a logical structure, easy to understand by a
newcomer. A next thing is that if you, lets say, want to check some USB
functions you don't need to see all the properties of all USB tablets
(even nicely folded by vim), that's why I added a separate header file
with the definitions. I'm sure it can be done better, so I'm open to all
suggestions.

--
regards,
Przemo



------------------------------------------------------------------------------
Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day
trial. Simplify your report design, integration and deployment - and focus on
what you do best, core application coding. Discover what's new with
Crystal Reports now.  http://p.sf.net/sfu/bobj-july
_______________________________________________
Linuxwacom-devel mailing list
Linuxwacom-devel@...
https://lists.sourceforge.net/lists/listinfo/linuxwacom-devel

[Peter Hutterer-3]

On Fri, Nov 27, 2009 at 05:49:09PM +0000, Przemysław Firszt wrote:

> Dnia 2009-11-22, nie o godzinie 14:05 -0800, Ping pisze:
>
> > However, this is an open source project. You can provide a guideline
> > for us or share your suggestions with us. We are more than happy to
> > accept any contributions to the project, especially for
> > xf86-input-wacom, which bears our future. We want, by all means, to
> > make it easier for others to contribue to linuxwacom project.
>
> OK, here is first sketch of proposed changes in the structure of the
> driver:
>
> wcmCommon.c -  contains all funcions required for handling a tablet
> (receiving, handling, sending events), but not for
> initializing/opening/closing/configuring tablet or manipulating data).
> xf86WcmOpen should be in xf86Wacom.c
>
> wcmCompat.c - DELETE. xf86WcmWait to be moved to wcmISDV4.c (it's used
> only there) xf86WcmReady be moved to wcmWacom.c (it's used only there)
>
> wcmConfig.c - allocating device, initialising, plugging, unplugging,
> uninitialising. xf86WcmPointInArea, xf86WcmAreasOverlap,
> xf86WcmAreaListOverlap should be moved to wcmCommon

these xf86* functions are wrongly named, they're not X interface functions
and should be renamed to wcm*. after the move, that is (see end of email).
 

> wcmFilter.c - data filtering (like pressure curve functions or jitter
> correction, tilt2rotation)
>
> wcmFilter.h - header file for wcmFilter.c
>
> wcmISDV4.c - Wacom IV protocol specific functions
>
> wcmTilt2Rotation.c - DELETE. Function xf86WcmTilt2R to be moved to
> wcmFilter, tiltTable to go to wcmFilter.h
>
> wcmUSB.c - USB specific functions. WacomModel definitions to be moved to
> a new wcmUSB.h header file. Another option is to create wcmConfig.h and
> drop all USB and ISDV4 definitions there.
>
> wcmValidateDevice.c - probing, parsing, checking for duplicate device
>
> wcmXCommand.c - Mode switching and screen changing functions,
> xf86WcmSetProperty and InitWcmDeviceProperties to be moved to
> xf86Config.c

IMO, wcmXCommand.c is a confusing name as it is. we could just rename it to
wcmProperties.c and move the other stuff (which is a mere 100 LOC or so)
into xf86Wacom.c

> xf86Wacom.c - opening, closing device, change control. Option 2: merge
> xf86Wacom with wcmConfig. xf86WcmRegisterX11Devices to be moved to
> wcmConfig.c and xf86WcmIsWacomDevice to wcmValidateDevice.c

I think xf86Wacom.c should only contain the interface to the X server: the
module data, driver hooks to initialize, set up and remove the driver, etc.
No wacom-specific internal data.
this is hopefully in Ping's best interest as well, as that should be the
only file that changes when the X API or ABI changes and most if not all ABI
ifdefs should be in that file only.
the rest of the code can then be better shared with linuxwacom.

> My goal is to create a logical structure, easy to understand by a
> newcomer. A next thing is that if you, lets say, want to check some USB
> functions you don't need to see all the properties of all USB tablets
> (even nicely folded by vim), that's why I added a separate header file
> with the definitions. I'm sure it can be done better, so I'm open to all
> suggestions.

git is good at detecting moved content, including maintaining the
git blame history. i.e. if you run git blame on file it'll give you the
history across the move.

This works as long as you move the code as-is and then change it later - I
recommend doing exactly this. Don't be tempted to fix up whitespaces or
even bugs when you copy from A to B, put it as a separate patch on top of
the move.
(note that fixing bugs during a janitorial cleanup such as this is a big
no-no in any project anyway).

The plan itself sounds good, though though I recommend not doing everything
in one go but rather in small patches, one-by-one.
intrusive stuff like this is hard to slot in and a merge would be a
nightmare to bisect. the smaller the patches, the easier we can just slot
them in between the others and the easier the conflict resolution on rebases.
I really want to avoid a merge commit for this.

Ping - how does that sound to you?
 
Cheers,
  Peter

------------------------------------------------------------------------------
Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day
trial. Simplify your report design, integration and deployment - and focus on
what you do best, core application coding. Discover what's new with
Crystal Reports now.  http://p.sf.net/sfu/bobj-july
_______________________________________________
Linuxwacom-devel mailing list
Linuxwacom-devel@...
https://lists.sourceforge.net/lists/listinfo/linuxwacom-devel

[Ping Cheng-2]

2009/11/29 Peter Hutterer <peter.hutterer@...>

git is good at detecting moved content, including maintaining the

git blame history. i.e. if you run git blame on file it'll give you the
history across the move.

This works as long as you move the code as-is and then change it later - I
recommend doing exactly this. Don't be tempted to fix up whitespaces or
even bugs when you copy from A to B, put it as a separate patch on top of
the move.
(note that fixing bugs during a janitorial cleanup such as this is a big
no-no in any project anyway).

The plan itself sounds good, though though I recommend not doing everything
in one go but rather in small patches, one-by-one.
intrusive stuff like this is hard to slot in and a merge would be a
nightmare to bisect. the smaller the patches, the easier we can just slot
them in between the others and the easier the conflict resolution on rebases.
I really want to avoid a merge commit for this.

Ping - how does that sound to you?

I have to say the plan is a very good one. 

My concern is how to prioritize this plan v.s. our current development effort. xf86-input-wacom isn't in stable condition yet. Should we finish the basic features first or do both in parallel? From users perspective, having a stable driver is definitely the top priority. From long-term maintenance and software best practice point of view, this plan has its importance too.

So, Przemo, a question for you.  Are you interested in working with us to get the basic features (by basic features I mean porting the features that we have at linuxwacom to xf86-input-wacom) out first or does the ugly code really reduces your willingness to make a contribution now? All effort is extremely important to this project. I don't wan to lose even a bit of it.

Ping

------------------------------------------------------------------------------
Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day
trial. Simplify your report design, integration and deployment - and focus on
what you do best, core application coding. Discover what's new with
Crystal Reports now.  http://p.sf.net/sfu/bobj-july
_______________________________________________
Linuxwacom-devel mailing list
Linuxwacom-devel@...
https://lists.sourceforge.net/lists/listinfo/linuxwacom-devel

[przemof]

Dnia 2009-11-29, nie o godzinie 19:52 -0800, Ping pisze:
> So, Przemo, a question for you.  Are you interested in working with us
> to get the basic features (by basic features I mean porting the
> features that we have at linuxwacom to xf86-input-wacom) out first or
> does the ugly code really reduces your willingness to make a
> contribution now? All effort is extremely important to this project. I
> don't wan to lose even a bit of it.
Peter, Ping,
Thanks for the tips about git - I didn't know that I shouldn't make move
& change in one commit. I'm going to work on the driver and I think that
the best way is to make these housekeeping changes as we go. The
guideline with your comments seems to clear enough to do it.
Anyway I'm going to start with proper module unloading, so I have to
modify xserver first.

Thanks again for your comments and tips!
--
Przemo




------------------------------------------------------------------------------
Join us December 9, 2009 for the Red Hat Virtual Experience,
a free event focused on virtualization and cloud computing.
Attend in-depth sessions from your desk. Your couch. Anywhere.
http://p.sf.net/sfu/redhat-sfdev2dev
_______________________________________________
Linuxwacom-devel mailing list
Linuxwacom-devel@...
https://lists.sourceforge.net/lists/listinfo/linuxwacom-devel