Re: [Kicad-developers] [PATCH] Implement auto annotation on component/symbol placement.
HI,
thank you so much for the feedback.
I agree with your first point, will do that very soon.
Regarding the code formatting, I tried using both uncrustify and
clang-format with the supplied configs but none of them really worked
because they would end up changing lots of other code outside of the
scope of this feature so I'm not sure what to do about that (even just
other parts of files I edited). I could always do this by hand but
automagic code formatting is much nicer.
I suspected there may be some issues with multi-unit symbols so I will
look into that.
I will also use `format-patch` when sending in new patches. Thank you
for letting me know about it.
On Tue, Oct 15, 2019 at 8:41 PM Seth Hillbrand wrote:
> On 2019-10-14 14:42, Zficani Zficani wrote:
>
> > Hi,
> > No problem, I just wanted to make sure I sent the message properly.
> > Here's a single squashed patch with all previous changes and these
> > comments about copying selection.
> >
> > Thank you so much for your review.
> >
>
> Hi Zficani-
>
> The functionality feels correct and I really like it. Here are a few
> comments on the current patch:
>
> 1) I would prefer that the disabled options in the Annotation page are
> grey (disabled) and not hidden when the option is unchecked. This
> reserves the correct space for them when we add options in the future.
>
> 2) Please double-check your code formatting. Spaces inside the
> parentheses are missing in a few spots.
>
> 3) Don't use C-style casts. C++ static_cast() is preferred.
>
> 4) Single-line statements after if/else don't get brackets {}
>
> 5) I think that pasting Unit B of a component should paste as the first
> missing Unit B in the schematic and not the next open annotation number.
> See the attached image for the result of duplicating a quad op-amp for
> an example of this problem.
>
> This will be a great addition to KiCad. Thank you for taking this one
> on!
>
> Best-
> Seth
>
> Seth Hillbrand
> KiCad Services Corporation
> +1 530 302 5483 | +1 212 603 9372
> www.kipro-pcb.com
> Davis, CA
___
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
Re: [Kicad-developers] [PATCH] Implement auto annotation on component/symbol placement.
Hi, No problem, I just wanted to make sure I sent the message properly. Here's a single squashed patch with all previous changes and these comments about copying selection. Thank you so much for your review. On Mon, Oct 14, 2019 at 12:13 AM Ian McInerney wrote: > Zifcani, > > Sorry for the delay in the response. Yes, you can just attach the new > patches to an email and send it to the list. You can either squash them all > into 1 patch, or create 2 patches (the first just the wxFormbuilder dialog > class, so all the _base.fbp/cpp/h files, and the second the remaining > files). > > In regard to the selection return type, it sounds like the sorting > operation is invalidating an iterator held in another object for iterating > over the selection, causing the hang/crash. I guess that for now it is fine > to make these a full copy instead of a reference, but it would be good if > you included a comment with them explaining why they are a copy instead of > the reference. We can look into which iterators are being broken by the > sort operation later (unless you want to do it now). > > -Ian > > On Fri, Oct 11, 2019 at 9:09 AM Zficani Zficani wrote: > >> Hi, >> I see, I will send in new patches later today. Do I just send them in an >> email like this? >> >> On Fri, 11 Oct 2019, 02:10 Seth Hillbrand, wrote: >> >>> >>> On 2019-10-09 17:08, Zficani Zficani wrote: >>> >>> Hi, >>> it's been about 10 days since my message so I just want to make sure I >>> sent it properly and you noticed it because I'm not sure as I'd want to be >>> able to actually use this feature soon because it's very useful for me. >>> >>> Thank you. >>> >>> On Sun, Sep 29, 2019 at 11:55 PM Zficani Zficani >>> wrote: >>> >>>> 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). >>>> >>>> >>>> >>> Hi Zficani- >>> >>> >>> I can't speak for others but I was waiting for either a patch with the >>> revised code or a merge request on launchpad. I like the idea of your >>> feature and look forward to helping get it into merge shape. I agree with >>> Ian's comments and was hoping to see how you address them. >>> >>> >>> Best- >>> >>> Seth >>> >>> >>> Seth Hillbrand >>> >>> Chief Technologist >>> >>> KiCad Services Corporation >>> Twitter [image: Twitter] <https://twitter.com/KiProEDA> LinkedIn [image: >>> LinkedIn] <https://www.linkedin.com/company/kicad/about> >>> +1 530 302 5483 <+15303025483> | +1 212 603 9372 <+12126039372> >>> www.kipro-pcb.com >>> Davis, CA >>> >> diff --git a/.gitignore b/.gitignore index 619c7f963..f62e05e15 100644 --- a/.gitignore +++ b/.gitignore @@ -74,6 +74,7 @@ pcbnew/Info.plist .*.swp *~ .idea +.vscode .DS_Store *.png *.kiface diff --git a/eeschema/CMakeLists.txt b/eeschema/CMakeLists.txt index f22886414..86569a39d 100644 --- a/eeschema/CMakeLists.txt +++ b/eeschema/CMakeLists.txt @@ -97,6 +97,8 @@ set( EESCHEMA_DLGS dialogs/panel_eeschema_display_options_base.cpp dialogs/panel_eeschema_settings.cpp dialogs/panel_eeschema_settings_base.cpp +dialogs/panel_eeschema_annotation_options.cpp +dialogs/panel_eeschema_annotation_options_base.cpp dialogs/panel_libedit_settings.cpp dialogs/panel_libedit_settings_base.cpp dialogs/panel_sym_lib_table.cpp diff --git a/eeschema/component_references_lister.cpp b/eeschema/component_references_lister.cpp index 867fd18e3..503202ea3 100644 --- a/eeschema/component_references_lister.cpp +++ b/eeschema/component_references_lister.cpp @@ -112,21 +112,7 @@ bool SCH_REFERENCE_LIST::sortByRefAnd
Re: [Kicad-developers] [PATCH] Implement auto annotation on component/symbol placement.
Hi, I see, I will send in new patches later today. Do I just send them in an email like this? On Fri, 11 Oct 2019, 02:10 Seth Hillbrand, wrote: > > On 2019-10-09 17:08, Zficani Zficani wrote: > > Hi, > it's been about 10 days since my message so I just want to make sure I > sent it properly and you noticed it because I'm not sure as I'd want to be > able to actually use this feature soon because it's very useful for me. > > Thank you. > > On Sun, Sep 29, 2019 at 11:55 PM Zficani Zficani > wrote: > >> 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). >> >> >> > Hi Zficani- > > > I can't speak for others but I was waiting for either a patch with the > revised code or a merge request on launchpad. I like the idea of your > feature and look forward to helping get it into merge shape. I agree with > Ian's comments and was hoping to see how you address them. > > > Best- > > Seth > > > Seth Hillbrand > > Chief Technologist > > KiCad Services Corporation > Twitter [image: Twitter] <https://twitter.com/KiProEDA> LinkedIn [image: > LinkedIn] <https://www.linkedin.com/company/kicad/about> > +1 530 302 5483 <+15303025483> | +1 212 603 9372 <+12126039372> > www.kipro-pcb.com > Davis, CA > ___ 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
Re: [Kicad-developers] [PATCH] Implement auto annotation on component/symbol placement.
Hi, it's been about 10 days since my message so I just want to make sure I sent it properly and you noticed it because I'm not sure as I'd want to be able to actually use this feature soon because it's very useful for me. Thank you. On Sun, Sep 29, 2019 at 11:55 PM Zficani Zficani wrote: > 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 > 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 styl
Re: [Kicad-developers] [PATCH] Implement auto annotation on component/symbol placement.
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 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 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 >>
