← Back to team overview

kicad-developers team mailing list archive

Re: [PATCH] Implement auto annotation on component/symbol placement.


I reviewed your remarks and made appropriate changes but I have a few
Regarding your first remark, I had to make them non-reference because
the selection is sorted inplace which means that the original selection
will be modified which in turn make unhighlight hang/enter and infinite
loop when copying/duplicating or just unhighlighting multiple objects.
I'm not sure why exactly this is happening but it obviously has
something to do with the fact that selection is now sorted by

I did change the code according to the second and third remark though
and will send in the patches later (it's currently available on
if anyone wants to take a look).

Regaring the fourth remark, I'm not sure what duplicate code you are
actually referring to but I guess it would be
SCH_REFERENCE_LIST::sortByReferenceOnly and my
EE_SELECTION::SortComponentsByRef. So I have implemented a function to
compare two SCH_COMPONENTs based on their reference and I think
that's what you actually wanted.

Also, could I instead of squashing commits just squash all of them into
one big patch. This would get rid of any WIP code or TODOs. If not, I
will try my to squash them as best as I can but TODOs and placeholders
are just how I work.

I have tried to use both clang-format and uncrustify to format the code
but neither of them seem to have a proper config so it would end up
changing lots of code that actually conforms to the code style
guidelines but also code that I didn't write but is a part of files I
actually modified. I'm not sure what exactly I should do about this.
I guess I could do the formatting manually but I wonder if there's a
better automagic way to do it.

Thank you for your review.

On Sun, Sep 29, 2019 at 1:11 AM 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

Follow ups