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 <[email protected]> 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 : [email protected] > Unsubscribe : https://launchpad.net/~kicad-developers > More help : https://help.launchpad.net/ListHelp >
_______________________________________________ Mailing list: https://launchpad.net/~kicad-developers Post to : [email protected] Unsubscribe : https://launchpad.net/~kicad-developers More help : https://help.launchpad.net/ListHelp
