Hi, I reviewed your remarks and made appropriate changes but I have a few questions. 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 reference.
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 https://github.com/Nufflee/kicad-source-mirror/tree/1335616-annotate-on-placement 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.mciner...@ieee.org> wrote: > 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 <zifc...@gmail.com> 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@lists.launchpad.net >> Unsubscribe : https://launchpad.net/~kicad-developers >> More help : https://help.launchpad.net/ListHelp >> >
_______________________________________________ Mailing list: https://launchpad.net/~kicad-developers Post to : kicad-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~kicad-developers More help : https://help.launchpad.net/ListHelp
