← Back to team overview

kicad-developers team mailing list archive

Re: Bug 1677282 fix feedback.

 

I just took the cheap and dirty way out and made sure that the unit is
never 0.  I agree, fixing this at the selector level probably makes more
sense but this will ensure that if someone mucks it up in the future, at
least the unit value can never be zero and reintroduce this bug.

Interestingly, debugging this exposed a separate issue.  When using the
viewer to select the component, the unit selected is always 1.  I'll
file a bug report on this as soon as I get a chance.

On 3/30/2017 3:53 PM, Chris Pavlina wrote:
> lol, I was just going to ask you why that line is even there if _no_
> components can have a zero unit, and then decided to git-blame it and
> realized _I_ wrote that line.
> 
> I think the reason I did that is that the new selector returns zero when
> the user has selected a root component - it only returns a nonzero unit
> if a unit was actually selected. The documentation for the selector
> points this out, but clearly it's sufficiently confusing that I got it
> wrong in my own code :\
> 
> Probably best to fix DIALOG_CHOOSE_COMPONENT::GetSelectedAlias() to
> always return a valid unit. I wouldn't go so far as to make all
> CMP_TREE_NODE_ALIASes have Unit=1; that would have the desired effect,
> but that class is meant to represent just an alias and an alias by
> itself doesn't really have a unit number.
> 
> 
> On Thu, Mar 30, 2017 at 03:26:11PM -0400, Wayne Stambaugh wrote:
>> The problem is line 169 of eeschema/getpart.cpp.  The component unit
>> cannot be 0 even for components that do not have multiple units.  There
>> probably should be an assertion in the SCH_COMPONENT ctor to warn devs
>> about this.
>>
>> On 3/30/2017 3:24 PM, Chris Pavlina wrote:
>>> Ooooops. Sorry.
>>>
>>> I caught one place where I had done that. Thought that was all. :\
>>>
>>>
>>> On Thu, Mar 30, 2017 at 03:13:31PM -0400, Wayne Stambaugh wrote:
>>>> I believe since the new component chooser dialog has been implement
>>>> which hasn't been very long.
>>>>
>>>> On 3/30/2017 2:53 PM, Maciej Suminski wrote:
>>>>> Do you know for how long the bug was present? It could help estimate the
>>>>> number of schematic files affected.
>>>>>
>>>>> As much as I hate windows popping up, I do not think notifying the users
>>>>> would be that annoying. It is going to happen only once for each invalid
>>>>> file. I doubt there are people working on 10+ designs in parallel.
>>>>>
>>>>> I vote for #3. IMHO #4 is acceptable too. I would avoid #1 and #2 in
>>>>> particular.
>>>>>
>>>>> Regards,
>>>>> Orson
>>>>>
>>>>> On 03/30/2017 07:33 PM, Wayne Stambaugh wrote:
>>>>>> I have found and fixed the bug in this bug report:
>>>>>>
>>>>>> https://bugs.launchpad.net/kicad/+bug/1677282
>>>>>>
>>>>>> This bug creates invalid schematic files which sets the component unit
>>>>>> flag to 0 and breaks the netlist generator.  The fix itself is simple.
>>>>>> What is not simple is what to do about the invalid schematic files that
>>>>>> have already been created.  I added code to schematic parser to fix this
>>>>>> but this creates a dilemma.  Technically the schematic is modified which
>>>>>> begs the question, what to do next.  None of the following choices are
>>>>>> particularly appealing:
>>>>>>
>>>>>> 1) Do nothing and leave the file in an invalid status until the next
>>>>>> time the user saves the schematic.  This is probably the most convenient
>>>>>> but what about broken schematics being used by versions of kicad prior
>>>>>> to this fix?
>>>>>>
>>>>>> 2) Set the schematic modified status which will trigger a save warning
>>>>>> when eeschema is closed even if the user hasn't made any changes.  We
>>>>>> already do this with SCH_SCREEN::SchematicCleanUp() that gets called
>>>>>> lots of places outside of schematic editing.  The problem with this is
>>>>>> that the user has no idea why they are getting a save warning when they
>>>>>> didn't actually change anything.
>>>>>>
>>>>>> 3) Set the schematic modified status and inform the user that there was
>>>>>> an error in their schematic file that was repaired on load and requires
>>>>>> saving.  This is the most informative for the user but reeks of nagware.
>>>>>>
>>>>>> 4) Silently save the corrected schematic with no user interaction.  This
>>>>>> will undoubtedly make VCS users unhappy.
>>>>>>
>>>>>> As much as I hate nagware, I like unexpected save warnings when I
>>>>>> haven't changed anything even less so I'm leaning towards option 3.  Any
>>>>>> feedback would be appreciated.
>>>>>>
>>>>>> Thanks,
>>>>>>
>>>>>> Wayne
>>>>>>
>>>>>> _______________________________________________
>>>>>> 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
>>>>>>
>>>>>
>>>>> _______________________________________________
>>>>> 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
>>>>>
>>>>
>>>> _______________________________________________
>>>> 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


References