kicad-developers team mailing list archive

[Wayne Stambaugh <stambaughw@xxxxxxxxx>]

Seth,

I have a few comments on this patch.

First and foremost, you should have gotten input before you spent the
time and effort doing this.  It may have saved you some grief.  I also
know there is a tendency to make a big push to get last minute changes
in before the upcoming feature freeze so that is not helping your cause.

There are few coding policy issues.  Two spaces are required between
function definitions in cpp source files.  It also preferable to have a
empty line before and after control blocks so it's easier to see where
they begin and end.  Other than that, everything else looks fine.

On 11/10/2017 1:50 PM, Seth Hillbrand wrote:
> One of the Eeschema features that has been requested for a while is
> customizable graphic line styles that allow greater differentiation in
> schematic documentation.  c.f. 
> https://bugs.launchpad.net/kicad/+bug/594059
> https://bugs.launchpad.net/kicad/+bug/1405026
> 
> The limitation has been not wanting to change the existing schematic format.
> 
> I propose a way around this while implementing the desired feature. 
> Specifically, the attached patch allows the user to customize graphic
> lines (schematic wires are disallowed), with the extra wire data being
> stored at the end of the wire line.  This change should be transparent
> to the existing schematic format as the trailing characters are ignored
> in the legacy file reader.  Thus customization will be ignored if you
> are sharing files between versions.

Just because you found a work around in the parser does not mean your
change does not constitute a file format change.  When someone loads a
schematic in an earlier version of eeschema and saves the schematic, all
of this information will be lost.  That smells like a file format change
to me.  If I were going to accept this patch, it would require a file
version bump.  I would also prefer that you didn't use tabs in the file
writer.  Did you try loading this with stable version 4 which uses the
old parser?

> 
> Second, to avoid large, useless diffs in versioning storage, the patch
> does not store any additional formatting data unless it has been changed
> from the default.

This is always a good idea IMO.  Creating unnecessary file diffs makes
our VCS users happy.

> 
> I'm attaching an image showing the line edit dialog as well as the patch
> implementing this.  Please let me know if there are any questions or
> suggestions for improvement.

The dialog alignment could use some work but overall it looks good.  If
you make the changes I suggested and no one else strongly objects, I
would be willing to permit this change even though it goes against my
better judgement.

Cheers,

Wayne

> 
> Best-
> Seth
> 
> 
> _______________________________________________
> Mailing list: https://launchpad.net/~kicad-developers
> Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx
> Unsubscribe : https://launchpad.net/~kicad-developers
> More help   : https://help.launchpad.net/ListHelp
>