kicad-developers team mailing list archive

[Ian McInerney <Ian.S.McInerney@xxxxxxxx>]

Thanks for putting together this changeset. I have a few comments from just
looking through it (I haven't actually compiled and tested it yet).

1) Why did you change the variable type for the EE_SELECTION in patches 5
and 6? The requestSelection function returns a reference to the object, and
noting that it is a reference is useful.
2) Use the enumeration values when interfacing with the annotation
algorithm data
(e.g. SetAutoAnnotationNumberingOption, GetAutoAnnotationAlgoOption, inside
switch statements, etc). This will reduce the need for the casting and also
make it clear what you are setting the algorithm to.
3) It would be better if the annotation options were not stored in static
variables inside the SCH_COMPONENT class, but instead as members of
SCH_EDIT_FRAME (possibly in a new structure so that they are
self-contained) with the getters/setters in there. We have had some issues
in the past with static variables not being initialized properly on some
operating systems, so I would like to avoid using them if possible. This
would also eliminate the need for the pointer accessor functions.
4) You seem to have some duplicated code for comparing two SCH_COMPONENTS
when sorting the components by their reference designator. It would
probably be worth adding a new function to the SCH_COMPONENT class to
perform that comparison (it would compare the current object against the
supplied object), and it could have a return style similar
to RefDesStringCompare.
5) The copyright statements in patch 15 should just have 2019 in it, not
1992-2019 since those files are new.

It would also be appreciated if you could squash some of these commits
together. For instance, the last three commits seem to be just fixing small
issues from the earlier commits, so they could be squashed into those
earlier commits, and one of them is just renaming functions you created in
earlier commits. Also, there is a lot of noise in terms of TODO statements
and if( true ) floating in each commit that seem to be changed each time,
so it is hard to see exactly what the overall changeset looks like.

Please look over the style guide here:
http://docs.kicad-pcb.org/doxygen/md_Documentation_development_coding-style-policy.html
and
update your patches using it, I notice there are some style issues in your
changeset (such as including spaces after the language keywords if, for,
etc. and before their opening parenthesis). You don't have to fix the code
you haven't touched, but things like reordering the headers into
alphabetical order if you add a new one would be appreciated.

Thanks,
-Ian

On Sat, Sep 28, 2019 at 8:41 PM Zficani Zficani <zifcani@xxxxxxxxx> wrote:

> This patch adds an option to automatically annotate components/symbols
> when they're placed, copied or duplicated. Configuration can be found
> in Preferences and it is akin to 'Annotate Schematic...' tool. User can
> choose how they want their components number and whether the whole
> schematic should be taken into account when doing so.
>
> Summary of this patch:
>   - Add .vscode to .gitignore
>   - Create `Annotation Options` settings panel in eeschema preferences.
>   - Save selected Annotation Options to project file.
>   - Introduce SCH_COMPONENT::Annotate to annotate components
>     individually.
>   - Introduce EE_SELECTION::SortComponentsByRef to properly sort
>     selection when auto annotating.
>   - Auto annoate components when placed, pasted and duplicated.
>
> I have also only now realized New tag in launchpad means the feature
> request still needs to be looked at but I've already implemented this so
> I might as well send it and see what you think about it.
>
> This patch fixes lp:1335616.
> _______________________________________________
> 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
>