← Back to team overview

kicad-developers team mailing list archive

Re: [PATCH] libedit: part deletion

 

Hi,

On 02/07/2017 03:21 PM, Wayne Stambaugh wrote:
> I committed this patch.  Thanks Oliver.  Please keep in mind when
> submitting patches that fix bugs to include the bug report information
> in the commit message.  At a minimum, "Fixes: lp:######" is required.
> It's also not a bad idea to include the link to the bug report even
> though our commit hook should take care of this automatically.

Unfortunately, the commit hook cannot modify/reject commits, it is
restricted to processing them. In the end, it only adds links to bug
fixing commits in corresponding lp reports, but there is no way to
modify an existing commit message to add a link.

Cheers,
Orson

> Cheers,
> 
> Wayne
> 
> On 2/4/2017 11:21 PM, Oliver Walters wrote:
>> Ok, I have found the cause of this behaviour:
>>
>> As the selection dialog's true purpose is to *select* a component, it
>> was setting the m_unit* variable to the selected unit. If the currently
>> selected part did not have at last as many units, it was showing a
>> non-existent view of the selected part.
>>
>> This has been addressed in the attached version of the patch.
>>
>> Hopefully no more errors!
>>
>> On Sun, Feb 5, 2017 at 12:36 AM, Chris Pavlina <pavlina.chris@xxxxxxxxx
>> <mailto:pavlina.chris@xxxxxxxxx>> wrote:
>>
>>     Nope, this doesn't fix the weird behavior. However, I have noticed that
>>     it only happens in some specific case (the conditions of which I have
>>     yet to properly identify), I just got lucky and found that case first.
>>     Here are steps that should reproduce this:
>>
>>     1) Create and load a new project.
>>     2) Open eeschema. Remove all libs, then add the library at [1], [2].
>>     3) Open libedit and choose the Atmel library I attached.
>>     4) Load component ATMEGA168A-AU.
>>     5) Delete unit B of ATSAM4E16CA-AU.
>>
>>     Follow those steps *exactly* and it should happen.
>>
>>     Also, I'm not really happy with how units are being handled here. If I
>>     select a single unit, it shouldn't delete the whole part - that's
>>     misleading. If it's too troublesome to implement single-unit delete, we
>>     should add a mode to the component selector widget to disable display of
>>     units.
>>
>>     **However**, even if we just drop the unit selection from the dialog, I
>>     really think it might be worth looking into why this happens when using
>>     it...this is weird, may indicate some deeper bug...
>>
>>     [1] https://misc.c4757p.com/Atmel.lib
>>     <https://misc.c4757p.com/Atmel.lib>
>>     [2] https://misc.c4757p.com/Atmel.dcm
>>     <https://misc.c4757p.com/Atmel.dcm>
>>
>>     On Sat, Feb 04, 2017 at 07:14:42PM +1100, Oliver Walters wrote:
>>     > Chris,
>>     >
>>     > I have attached an updated patch that (at least on my end) fixes the
>>     > unit-deletion bug.
>>     >
>>     > Can you please confirm that you no longer see the erroneous behaviour?
>>     >
>>     > I have also included your preselect patch.
>>     >
>>     > Cheers, Oliver
>>     >
>>     > On Thu, Feb 2, 2017 at 10:33 PM, Chris Pavlina
>>     <pavlina.chris@xxxxxxxxx <mailto:pavlina.chris@xxxxxxxxx>>
>>     > wrote:
>>     >
>>     > > On second thought, I'm reverting it, I found a weird bug.
>>     Apologies for
>>     > > the accidental push, I'll be more careful next time.
>>     > >
>>     > > I *was* investigating what I thought was a minor bug (i.e.
>>     didn't keep
>>     > > me from pushing the patch), but I found this. To reproduce:
>>     > >
>>     > > 1) Select a single-unit part
>>     > > 2) Delete one unit of a different, multi-unit part
>>     > >
>>     > > The current part gets messed up, like: https://misc.c4757p.com/
>>     > > messed-up-part.png
>>     > >
>>     > > Not sure if that's a rendering issue or if the pins are actually
>>     gone.
>>     > >
>>     > > What I *was* looking at is that I thought deleting a unit would just
>>     > > delete the entire part, and I was going to suggest one of two
>>     possible
>>     > > fixes:
>>     > >
>>     > > 1) Modify the component selector to have a mode where it doesn't
>>     delete
>>     > > units
>>     > > 2) Actually delete the unit, adding code to filter the part and
>>     remove
>>     > > anything on the unit to be deleted and reorder subsequent units.
>>     > >
>>     > > I was going to offer to write #2 myself, because that'd be
>>     really useful
>>     > > to me. But you should probably fix the bug described above first and
>>     > > resubmit.
>>     > >
>>     > > ALSO, I was going to contribute a small patch to this to
>>     preselect the
>>     > > current component just like I added to Load Component recently - I
>>     > > attached a diff for that in case you want it, I had it all done
>>     before
>>     > > noticing this.
>>     > >
>>     > > On Thu, Feb 02, 2017 at 06:15:57AM -0500, Chris Pavlina wrote:
>>     > > > Oops, I didn't mean to push this one at the same time, clumsy push
>>     > > > command - but it looks good too. I always hated that delete
>>     dialog.
>>     > > > Thank you!
>>     > > >
>>     > > > On Thu, Feb 02, 2017 at 09:42:01PM +1100, Oliver Walters wrote:
>>     > > > > The attached patch removes the clunky "Delete Single Part"
>>     dialog in
>>     > > > > libedit, and re-uses the existing part-selector to find a
>>     component to
>>     > > > > delete.
>>     > > > >
>>     > > > > Now, the user can use the filter tool to select a component for
>>     > > deletion.
>>     > > >
>>     > >
>>
>>
>>
>>
>> _______________________________________________
>> 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