Re: [Kicad-developers] [PATCH] Fix for bug/1754049

[jp charras]

Le 27/03/2018 à 14:16, jp charras a écrit :
<>
> I just committed a first fix that fixes a few issues.
> Currently, a bug is still pending: a crash can happen if a undo command uses 
> a removed item.
> 
> About vias, that are multilayers items, blind/ buried vias and especially 
> micro vias are a tricky case.
> 
> Currently they are removed (due to a small bug in code) if they start on a 
> removed layer.
> But the actual problem is: Should we remove them if they are still living on 
> existing layers?
> 
> And if not should we modify the layer pair.
> Micro vias for instance are blind vias connecting 2 adjacent layers, so we 
> could modify easily the
> layer pair without removing them.
> 

I committed (in rev 2edc675) an other fix to force 4 layers (edge cut, 
courtyard and margin layers)
always in use, because they are (or will be, for margin) used in DRC.

This is a small fix, easy to undo or modify if it looks like not good.

just FYI.

-- 
Jean-Pierre CHARRAS

___
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] Fix for bug/1754049

[jp charras]

Le 26/03/2018 à 21:58, Maciej Suminski a écrit :
> Hi Jean-Pierre,
> 
 -Not so minor issues:
 Multilayers items (blind/buried vias, keepout zones) are incorrectly 
 handled:
 * In GAL mode, removed Multilayers items are still visible after deletion.
>>>
>>> This should be fixed.  Even if it's something as crude as checking to
>>> make sure the layer actually exists in the board before drawing an
>>> object in gal.
>>
>> I am certainly not a GAL specialist, but it is just a cache rebuild issue.
>>
>> However, apart GAL issue, Multilayers items are not correctly handled, 
>> because they are deleted
>> regardless they are still existing on not deleted layers.
> 
> I have just done the following:
> - set number of copper layers to 8
> - placed a few buried vias between In2.Cu (3rd layer from the top) and
> In5.Cu (3rd layer from the bottom)
> 
> Now when I decrease the number of Cu layers to 4, the vias remain but
> have the target layer undefined as In5.Cu does not exist anymore. Once I
> reduce the number of Cu layers to 2, they disappear. Restoring the
> original number of layers brings them back.
> 
> Is that what you see as well? As far as I understand, the items are not
> deleted as they are restored once the required layers are available. How
> to trigger the issues you describe?
> 
> Regards,
> Orson

I just committed a first fix that fixes a few issues.
Currently, a bug is still pending: a crash can happen if a undo command uses a 
removed item.

About vais, that are multilayers items, blind/ buried vias and especially micro 
vias are a tricky case.

Currently they are removed (due to a small bug in code) if they start on a 
removed layer.
But the actual problem is: Should we remove them if they are still living on 
existing layers?

And if not should we modify the layer pair.
Micro vias for instance are blind vias connecting 2 adjacent layers, so we 
could modify easily the
layer pair without removing them.

-- 
Jean-Pierre CHARRAS

___
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] Fix for bug/1754049

[Maciej Suminski]

Hi Jean-Pierre,

>>> -Not so minor issues:
>>> Multilayers items (blind/buried vias, keepout zones) are incorrectly 
>>> handled:
>>> * In GAL mode, removed Multilayers items are still visible after deletion.
>>
>> This should be fixed.  Even if it's something as crude as checking to
>> make sure the layer actually exists in the board before drawing an
>> object in gal.
> 
> I am certainly not a GAL specialist, but it is just a cache rebuild issue.
> 
> However, apart GAL issue, Multilayers items are not correctly handled, 
> because they are deleted
> regardless they are still existing on not deleted layers.

I have just done the following:
- set number of copper layers to 8
- placed a few buried vias between In2.Cu (3rd layer from the top) and
In5.Cu (3rd layer from the bottom)

Now when I decrease the number of Cu layers to 4, the vias remain but
have the target layer undefined as In5.Cu does not exist anymore. Once I
reduce the number of Cu layers to 2, they disappear. Restoring the
original number of layers brings them back.

Is that what you see as well? As far as I understand, the items are not
deleted as they are restored once the required layers are available. How
to trigger the issues you describe?

Regards,
Orson

___
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] Fix for bug/1754049

[jp charras]

Le 26/03/2018 à 18:40, Wayne Stambaugh a écrit :
> On 3/26/2018 7:19 AM, jp charras wrote:
>> Le 26/03/2018 à 01:31, Wayne Stambaugh a écrit :
>>> Now that a fix for the lost data has been merged, we should defer the fix 
>>> for how to handle deleting
>>> objects from removed layers until v6.  In the mean time, we should clearly 
>>> define the behavior
>>> before we write any code to prevent any wasted developer time.
>>>
>>> Cheers,
>>>
>>> Wayne
>>
>> Hi Wayne,
>>
>> Before waiting for V6, there are a few things that must be fixed.
>>
>> - Minor issues:
>> * I am thinking the layers now used in DRC should be not removable (edge 
>> cut, courtyards and in the
>> future margin layer)
>> I have a basic fix for that.
> 
> I'm fine with this but we should define which layers are "mandatory" and
> which layers are not and make the UI behave accordingly.

I am thinking edge cut is mandatory.
I do not have a strong opinion about margin layer, not yet in use.

Because courtyards are used in V5 libs and DRC, I am inclined to think they are 
mandatory.

My fix makes the UI behave accordingly.

> 
>> * non copper layers used in footprints can be removed  (without fortunately 
>> destroying the
>> footprint) without warning: I have a fix to warn the user then removed non 
>> copper layers are in use
>> in a footprint.
> 
> This seem reasonable.

OK.
I can commit this small change.

> 
>>
>> -Not so minor issues:
>> Multilayers items (blind/buried vias, keepout zones) are incorrectly handled:
>> * In GAL mode, removed Multilayers items are still visible after deletion.
> 
> This should be fixed.  Even if it's something as crude as checking to
> make sure the layer actually exists in the board before drawing an
> object in gal.

I am certainly not a GAL specialist, but it is just a cache rebuild issue.

However, apart GAL issue, Multilayers items are not correctly handled, because 
they are deleted
regardless they are still existing on not deleted layers.

> 
>> * In all modes: because undo is not handled, if a removed item (for instance 
>> a track) was previously
>> modified (and therefore in the undo list), and if a undo command is made, 
>> Pcbnew crashes.
>> At least the undo/redo list should be cleared.
> 
> I'm guessing users are not going to like having their undo/redo historyt
> wiped out after a long editing session.  Would it be possible to just
> remove the objects/actions on the removed layers from the undo/redo
> buffer rather than clearing the entire buffer?

Sure, but I am not sure this is a small change.

A stub exists: see in pcbnew/undo_redo.cpp: TestForExistingItem( BOARD* aPcb, 
BOARD_ITEM* aItem ),
but I am not sure it can be done for rc2.


-- 
Jean-Pierre CHARRAS

___
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] Fix for bug/1754049

[Wayne Stambaugh]

On 3/26/2018 7:19 AM, jp charras wrote:
> Le 26/03/2018 à 01:31, Wayne Stambaugh a écrit :
>> Now that a fix for the lost data has been merged, we should defer the fix 
>> for how to handle deleting
>> objects from removed layers until v6.  In the mean time, we should clearly 
>> define the behavior
>> before we write any code to prevent any wasted developer time.
>>
>> Cheers,
>>
>> Wayne
> 
> Hi Wayne,
> 
> Before waiting for V6, there are a few things that must be fixed.
> 
> - Minor issues:
> * I am thinking the layers now used in DRC should be not removable (edge cut, 
> courtyards and in the
> future margin layer)
> I have a basic fix for that.

I'm fine with this but we should define which layers are "mandatory" and
which layers are not and make the UI behave accordingly.

> * non copper layers used in footprints can be removed  (without fortunately 
> destroying the
> footprint) without warning: I have a fix to warn the user then removed non 
> copper layers are in use
> in a footprint.

This seem reasonable.

> 
> -Not so minor issues:
> Multilayers items (blind/buried vias, keepout zones) are incorrectly handled:
> * In GAL mode, removed Multilayers items are still visible after deletion.

This should be fixed.  Even if it's something as crude as checking to
make sure the layer actually exists in the board before drawing an
object in gal.

> * In all modes: because undo is not handled, if a removed item (for instance 
> a track) was previously
> modified (and therefore in the undo list), and if a undo command is made, 
> Pcbnew crashes.
> At least the undo/redo list should be cleared.

I'm guessing users are not going to like having their undo/redo historyt
wiped out after a long editing session.  Would it be possible to just
remove the objects/actions on the removed layers from the undo/redo
buffer rather than clearing the entire buffer?

___
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] Fix for bug/1754049

[jp charras]

Le 26/03/2018 à 13:24, Jeff Young a écrit :
> Could we allow deletion/disabling of the courtyard layer if we also disabled 
> the two courtyard-related checkboxes in DRC?  (Then again, that might be more 
> work than it deserves.)

No.
* These checkboxes are in the DRC dialog because the control of courtyard is a 
recent feature, and
many or all old boards/old footprints do not use courtyard, thus creating a lot 
of errors if they
are checked.
* This option is not directly related to the Layers setup dialog.
* All V5 footprints use a courtyard.

(These layers can be hidden for users that do not use yet these layers)

-- 
Jean-Pierre CHARRAS

___
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] Fix for bug/1754049

[jp charras]

Le 26/03/2018 à 01:31, Wayne Stambaugh a écrit :
> Now that a fix for the lost data has been merged, we should defer the fix for 
> how to handle deleting
> objects from removed layers until v6.  In the mean time, we should clearly 
> define the behavior
> before we write any code to prevent any wasted developer time.
> 
> Cheers,
> 
> Wayne

Hi Wayne,

Before waiting for V6, there are a few things that must be fixed.

- Minor issues:
* I am thinking the layers now used in DRC should be not removable (edge cut, 
courtyards and in the
future margin layer)
I have a basic fix for that.
* non copper layers used in footprints can be removed  (without fortunately 
destroying the
footprint) without warning: I have a fix to warn the user then removed non 
copper layers are in use
in a footprint.

-Not so minor issues:
Multilayers items (blind/buried vias, keepout zones) are incorrectly handled:
* In GAL mode, removed Multilayers items are still visible after deletion.
* In all modes: because undo is not handled, if a removed item (for instance a 
track) was previously
modified (and therefore in the undo list), and if a undo command is made, 
Pcbnew crashes.
At least the undo/redo list should be cleared.


-- 
Jean-Pierre CHARRAS

___
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] Fix for bug/1754049

[Jeff Young]

Could we allow deletion/disabling of the courtyard layer if we also disabled 
the two courtyard-related checkboxes in DRC?  (Then again, that might be more 
work than it deserves.)



___
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] Fix for bug/1754049

[Wayne Stambaugh]

Now that a fix for the lost data has been merged, we should defer the 
fix for how to handle deleting objects from removed layers until v6.  In 
the mean time, we should clearly define the behavior before we write any 
code to prevent any wasted developer time.


Cheers,

Wayne

On 03/22/2018 03:09 PM, hauptmech wrote:
No comments on the current discussion, it all makes sense. Just want to 
point out a couple things in case they were missed.


The layers are being deleted (instead of disabled) to prevent DRC 
problems and prevent file cruft, according to comments in the code.


kicad plugin is deleting layers from footprints when it saves a board by 
omitting layers not enabled. If you think removing layers from 
footprints is a bad idea, you will need to fix that.



 From a user point of view I would prefer layer setup to not delete 
anything. I can use the global deletion tool to delete ghings if i need. 
If there is DRC problems on a disabled layer, an exra note that the 
layer is disabled in the drc message would help.


On Fri, 23 Mar 2018, 07:34 jp charras, > wrote:


Le 22/03/2018 à 18:48, Andrey Kuznetsov a écrit :
 > I thought we were talking about deleting layers, not disabling them?

Both.
The dialog disables layers.
How to manage items living on disabled layers (considering they can
live on more than one layer,
like pads and keepout zones)?

Currently, I certainly do not see this problem as fixed.

 >
 > I agree with Wayne, a footprint is a whole item, deleting part of
it means it is no longer valid,
 > and thus must be removed.
 >
 > What if someone wants to remove the silkscreen layer because they
don't want it for production? I
 > guess they can just delete the silk gerber before sending it in.
I can't think of a reason someone
 > would want to delete a layer on a final product, without being
able to do it through gerber files.
 > Otherwise deleting layers would be done during initial design
stage where someone is trying to
 > figure out how many layers to have, but if they delete a top
layer with all the SMDs on it, then
 > well, they should be warned about it, and if they choose to
delete it, it's their fault and they
 > will have to undo or reimport all the items again.

Plot files has its own layer list.
Anyway, plot file list is not only the layers needed to make a board.
Fabrication files are needed only for assembly.
Gerber files are for making the board.
Other files are for assembly/documentation, and are usually not in
Gerber format.

 >
 > Disabling a layer should never delete existing objects.

However this topic is about deleting objects living on disabled layers.

 >
 > On Thu, Mar 22, 2018 at 10:15 AM, jp charras
mailto:jp.char...@wanadoo.fr>
>>
 > wrote:
 >
 >     Le 21/03/2018 à 17:46, Wayne Stambaugh a écrit :
 >     > JP,
 >     >
 >     > Did you take a look at this patch?  I know we have talked
about this in
 >     > the past and that the fix would not be easy.  Until we can
define and
 >     > implement a complete solution, this could be a short term
fix.  When you
 >     > get a chance, please take a look at it an comment on it.
 >     >
 >     > hauptmech,
 >     >
 >     > I'm not sure about the idea of breaking a footprint
(module) into layer
 >     > by layer pieces to match the removed layers.  Footprints
are typically
 >     > thought of as atomic objects.  I wonder if it wouldn't be
more prudent
 >     > to remove the footprint if any of it's layers are removed
from the layer
 >     > list and warn the user that removing said layer(s) would
result in
 >     > footprints being removed.
 >     >
 >     > Thanks,
 >     >
 >     > Wayne
 >     >
 >
 >     I had a look at this patch.
 >
 >     I do not think removing something to footprints already on
board is a good idea.
 >
 >     I understand other board items can or must be removed when
disabling a layer, but removing something
 >     to a footprint is breaking this footprint, that become no
more reliable.
 >
 >     What happens if later, a disabled layer like a silkscreen is
re-enabled for some reason?
 >     Footprints carefully designed are now broken.
 >
 >     Like Seth, I am thinking disabling a layer (disabling is not
deleting) should not modify footprints.
 >
 >     Currently, the Layer Setup dialog can create issues because
it allows disabling layers that are now
 >     used in DRC (edge cuts, courtyard, and margin that should be
used in V6 to create obstacles).
 >     Some other layers are mandatory to make a board: solder mask,
solder paste.
  

Re: [Kicad-developers] [PATCH] Fix for bug/1754049

[hauptmech]

No comments on the current discussion, it all makes sense. Just want to
point out a couple things in case they were missed.

The layers are being deleted (instead of disabled) to prevent DRC problems
and prevent file cruft, according to comments in the code.

kicad plugin is deleting layers from footprints when it saves a board by
omitting layers not enabled. If you think removing layers from footprints
is a bad idea, you will need to fix that.


>From a user point of view I would prefer layer setup to not delete
anything. I can use the global deletion tool to delete ghings if i need. If
there is DRC problems on a disabled layer, an exra note that the layer is
disabled in the drc message would help.

On Fri, 23 Mar 2018, 07:34 jp charras,  wrote:

> Le 22/03/2018 à 18:48, Andrey Kuznetsov a écrit :
> > I thought we were talking about deleting layers, not disabling them?
>
> Both.
> The dialog disables layers.
> How to manage items living on disabled layers (considering they can live
> on more than one layer,
> like pads and keepout zones)?
>
> Currently, I certainly do not see this problem as fixed.
>
> >
> > I agree with Wayne, a footprint is a whole item, deleting part of it
> means it is no longer valid,
> > and thus must be removed.
> >
> > What if someone wants to remove the silkscreen layer because they don't
> want it for production? I
> > guess they can just delete the silk gerber before sending it in. I can't
> think of a reason someone
> > would want to delete a layer on a final product, without being able to
> do it through gerber files.
> > Otherwise deleting layers would be done during initial design stage
> where someone is trying to
> > figure out how many layers to have, but if they delete a top layer with
> all the SMDs on it, then
> > well, they should be warned about it, and if they choose to delete it,
> it's their fault and they
> > will have to undo or reimport all the items again.
>
> Plot files has its own layer list.
> Anyway, plot file list is not only the layers needed to make a board.
> Fabrication files are needed only for assembly.
> Gerber files are for making the board.
> Other files are for assembly/documentation, and are usually not in Gerber
> format.
>
> >
> > Disabling a layer should never delete existing objects.
>
> However this topic is about deleting objects living on disabled layers.
>
> >
> > On Thu, Mar 22, 2018 at 10:15 AM, jp charras  >
> > wrote:
> >
> > Le 21/03/2018 à 17:46, Wayne Stambaugh a écrit :
> > > JP,
> > >
> > > Did you take a look at this patch?  I know we have talked about
> this in
> > > the past and that the fix would not be easy.  Until we can define
> and
> > > implement a complete solution, this could be a short term fix.
> When you
> > > get a chance, please take a look at it an comment on it.
> > >
> > > hauptmech,
> > >
> > > I'm not sure about the idea of breaking a footprint (module) into
> layer
> > > by layer pieces to match the removed layers.  Footprints are
> typically
> > > thought of as atomic objects.  I wonder if it wouldn't be more
> prudent
> > > to remove the footprint if any of it's layers are removed from the
> layer
> > > list and warn the user that removing said layer(s) would result in
> > > footprints being removed.
> > >
> > > Thanks,
> > >
> > > Wayne
> > >
> >
> > I had a look at this patch.
> >
> > I do not think removing something to footprints already on board is
> a good idea.
> >
> > I understand other board items can or must be removed when disabling
> a layer, but removing something
> > to a footprint is breaking this footprint, that become no more
> reliable.
> >
> > What happens if later, a disabled layer like a silkscreen is
> re-enabled for some reason?
> > Footprints carefully designed are now broken.
> >
> > Like Seth, I am thinking disabling a layer (disabling is not
> deleting) should not modify footprints.
> >
> > Currently, the Layer Setup dialog can create issues because it
> allows disabling layers that are now
> > used in DRC (edge cuts, courtyard, and margin that should be used in
> V6 to create obstacles).
> > Some other layers are mandatory to make a board: solder mask, solder
> paste.
> > These layers should be *always* enabled.
> >
> > So a better fix is certainly not to delete something in footprints,
> but do not allow disabling these
> > mandatory layers, and for others layers, display a warning if a
> disabled layer is in use in a
> > footprint.
> >
> > For me, the major bug is in the Layer Setup dialog that allows
> disabling any layer.
> >
> >
> > > On 3/20/2018 4:19 AM, Eeli Kaikkonen wrote:
> > >> 2018-03-20 0:19 GMT+02:00 Seth Hillbrand <
> seth.hillbr...@gmail.com 
> > >>  >>>:
> > >>
> > >>
>

Re: [Kicad-developers] [PATCH] Fix for bug/1754049

[jp charras]

Le 22/03/2018 à 18:48, Andrey Kuznetsov a écrit :
> I thought we were talking about deleting layers, not disabling them?

Both.
The dialog disables layers.
How to manage items living on disabled layers (considering they can live on 
more than one layer,
like pads and keepout zones)?

Currently, I certainly do not see this problem as fixed.

> 
> I agree with Wayne, a footprint is a whole item, deleting part of it means it 
> is no longer valid,
> and thus must be removed.
> 
> What if someone wants to remove the silkscreen layer because they don't want 
> it for production? I
> guess they can just delete the silk gerber before sending it in. I can't 
> think of a reason someone
> would want to delete a layer on a final product, without being able to do it 
> through gerber files.
> Otherwise deleting layers would be done during initial design stage where 
> someone is trying to
> figure out how many layers to have, but if they delete a top layer with all 
> the SMDs on it, then
> well, they should be warned about it, and if they choose to delete it, it's 
> their fault and they
> will have to undo or reimport all the items again.

Plot files has its own layer list.
Anyway, plot file list is not only the layers needed to make a board.
Fabrication files are needed only for assembly.
Gerber files are for making the board.
Other files are for assembly/documentation, and are usually not in Gerber 
format.

> 
> Disabling a layer should never delete existing objects.

However this topic is about deleting objects living on disabled layers.

> 
> On Thu, Mar 22, 2018 at 10:15 AM, jp charras  >
> wrote:
> 
> Le 21/03/2018 à 17:46, Wayne Stambaugh a écrit :
> > JP,
> >
> > Did you take a look at this patch?  I know we have talked about this in
> > the past and that the fix would not be easy.  Until we can define and
> > implement a complete solution, this could be a short term fix.  When you
> > get a chance, please take a look at it an comment on it.
> >
> > hauptmech,
> >
> > I'm not sure about the idea of breaking a footprint (module) into layer
> > by layer pieces to match the removed layers.  Footprints are typically
> > thought of as atomic objects.  I wonder if it wouldn't be more prudent
> > to remove the footprint if any of it's layers are removed from the layer
> > list and warn the user that removing said layer(s) would result in
> > footprints being removed.
> >
> > Thanks,
> >
> > Wayne
> >
> 
> I had a look at this patch.
> 
> I do not think removing something to footprints already on board is a 
> good idea.
> 
> I understand other board items can or must be removed when disabling a 
> layer, but removing something
> to a footprint is breaking this footprint, that become no more reliable.
> 
> What happens if later, a disabled layer like a silkscreen is re-enabled 
> for some reason?
> Footprints carefully designed are now broken.
> 
> Like Seth, I am thinking disabling a layer (disabling is not deleting) 
> should not modify footprints.
> 
> Currently, the Layer Setup dialog can create issues because it allows 
> disabling layers that are now
> used in DRC (edge cuts, courtyard, and margin that should be used in V6 
> to create obstacles).
> Some other layers are mandatory to make a board: solder mask, solder 
> paste.
> These layers should be *always* enabled.
> 
> So a better fix is certainly not to delete something in footprints, but 
> do not allow disabling these
> mandatory layers, and for others layers, display a warning if a disabled 
> layer is in use in a
> footprint.
> 
> For me, the major bug is in the Layer Setup dialog that allows disabling 
> any layer.
> 
> 
> > On 3/20/2018 4:19 AM, Eeli Kaikkonen wrote:
> >> 2018-03-20 0:19 GMT+02:00 Seth Hillbrand  
> >> >>:
> >>
> >>
> >>     As it is, the patch resolves an issue and creates another.
> >>
> >>
> >> Actually Seth is wrong here. It doesn't create another problem. Namely,
> >> as the code without the patch works now, it leaves the board uneditable
> >> anyways, and without a warning. Just test with a footprint which has
> >> nothing but ref and value and one paste-only pad. It doesn't matter
> >> whether the pad is left there or removed after the layer is deleted. 
> The
> >> footprint can't be selected or edited.
> >>
> >> I would still go with this patch, just add a sentence to the warning if
> >> pads are deleted. "Additionally this may lead to footprints which 
> cannot
> >> be edited or deleted" or something like that.
> 
> --
> Jean-Pierre CHARRAS-- 
Jean-Pierre CHARRAS

___
Mailing list: https://launchpad.net/~kicad-developers
Post to   

Re: [Kicad-developers] [PATCH] Fix for bug/1754049

[Jon Evans]

Is there any good reason to allow "deleting" layers that are part of
footprints?
I could see deleting routing layers (inner copper, etc) but I think we
should just get rid of the idea that you can "delete" the silkscreen etc.,
just make it "disable" so that it can be re-enabled without any fuss.

On Thu, Mar 22, 2018 at 1:48 PM, Andrey Kuznetsov 
wrote:

> I thought we were talking about deleting layers, not disabling them?
>
> I agree with Wayne, a footprint is a whole item, deleting part of it means
> it is no longer valid, and thus must be removed.
>
> What if someone wants to remove the silkscreen layer because they don't
> want it for production? I guess they can just delete the silk gerber before
> sending it in. I can't think of a reason someone would want to delete a
> layer on a final product, without being able to do it through gerber files.
> Otherwise deleting layers would be done during initial design stage where
> someone is trying to figure out how many layers to have, but if they delete
> a top layer with all the SMDs on it, then well, they should be warned about
> it, and if they choose to delete it, it's their fault and they will have to
> undo or reimport all the items again.
>
> Disabling a layer should never delete existing objects.
>
> On Thu, Mar 22, 2018 at 10:15 AM, jp charras 
> wrote:
>
>> Le 21/03/2018 à 17:46, Wayne Stambaugh a écrit :
>> > JP,
>> >
>> > Did you take a look at this patch?  I know we have talked about this in
>> > the past and that the fix would not be easy.  Until we can define and
>> > implement a complete solution, this could be a short term fix.  When you
>> > get a chance, please take a look at it an comment on it.
>> >
>> > hauptmech,
>> >
>> > I'm not sure about the idea of breaking a footprint (module) into layer
>> > by layer pieces to match the removed layers.  Footprints are typically
>> > thought of as atomic objects.  I wonder if it wouldn't be more prudent
>> > to remove the footprint if any of it's layers are removed from the layer
>> > list and warn the user that removing said layer(s) would result in
>> > footprints being removed.
>> >
>> > Thanks,
>> >
>> > Wayne
>> >
>>
>> I had a look at this patch.
>>
>> I do not think removing something to footprints already on board is a
>> good idea.
>>
>> I understand other board items can or must be removed when disabling a
>> layer, but removing something
>> to a footprint is breaking this footprint, that become no more reliable.
>>
>> What happens if later, a disabled layer like a silkscreen is re-enabled
>> for some reason?
>> Footprints carefully designed are now broken.
>>
>> Like Seth, I am thinking disabling a layer (disabling is not deleting)
>> should not modify footprints.
>>
>> Currently, the Layer Setup dialog can create issues because it allows
>> disabling layers that are now
>> used in DRC (edge cuts, courtyard, and margin that should be used in V6
>> to create obstacles).
>> Some other layers are mandatory to make a board: solder mask, solder
>> paste.
>> These layers should be *always* enabled.
>>
>> So a better fix is certainly not to delete something in footprints, but
>> do not allow disabling these
>> mandatory layers, and for others layers, display a warning if a disabled
>> layer is in use in a footprint.
>>
>> For me, the major bug is in the Layer Setup dialog that allows disabling
>> any layer.
>>
>>
>> > On 3/20/2018 4:19 AM, Eeli Kaikkonen wrote:
>> >> 2018-03-20 0:19 GMT+02:00 Seth Hillbrand > >> >:
>> >>
>> >>
>> >> As it is, the patch resolves an issue and creates another.
>> >>
>> >>
>> >> Actually Seth is wrong here. It doesn't create another problem. Namely,
>> >> as the code without the patch works now, it leaves the board uneditable
>> >> anyways, and without a warning. Just test with a footprint which has
>> >> nothing but ref and value and one paste-only pad. It doesn't matter
>> >> whether the pad is left there or removed after the layer is deleted.
>> The
>> >> footprint can't be selected or edited.
>> >>
>> >> I would still go with this patch, just add a sentence to the warning if
>> >> pads are deleted. "Additionally this may lead to footprints which
>> cannot
>> >> be edited or deleted" or something like that.
>>
>> --
>> Jean-Pierre CHARRAS
>>
>> ___
>> 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
>>
>
>
>
> --
> Remember The Past, Live The Present, Change The Future
> Those who look only to the past or the present are certain to miss the
> future [JFK]
>
> kandre...@gmail.com
> Live Long and Prosper,
> Andrey
>
> ___
> Mailing list: https://launchpad.net/~kicad-developers
> Post to : kicad-developers@lists.launchpad.net
> Unsubscribe : https://launchpad.net/~kicad

Re: [Kicad-developers] [PATCH] Fix for bug/1754049

[Andrey Kuznetsov]

I thought we were talking about deleting layers, not disabling them?

I agree with Wayne, a footprint is a whole item, deleting part of it means
it is no longer valid, and thus must be removed.

What if someone wants to remove the silkscreen layer because they don't
want it for production? I guess they can just delete the silk gerber before
sending it in. I can't think of a reason someone would want to delete a
layer on a final product, without being able to do it through gerber files.
Otherwise deleting layers would be done during initial design stage where
someone is trying to figure out how many layers to have, but if they delete
a top layer with all the SMDs on it, then well, they should be warned about
it, and if they choose to delete it, it's their fault and they will have to
undo or reimport all the items again.

Disabling a layer should never delete existing objects.

On Thu, Mar 22, 2018 at 10:15 AM, jp charras  wrote:

> Le 21/03/2018 à 17:46, Wayne Stambaugh a écrit :
> > JP,
> >
> > Did you take a look at this patch?  I know we have talked about this in
> > the past and that the fix would not be easy.  Until we can define and
> > implement a complete solution, this could be a short term fix.  When you
> > get a chance, please take a look at it an comment on it.
> >
> > hauptmech,
> >
> > I'm not sure about the idea of breaking a footprint (module) into layer
> > by layer pieces to match the removed layers.  Footprints are typically
> > thought of as atomic objects.  I wonder if it wouldn't be more prudent
> > to remove the footprint if any of it's layers are removed from the layer
> > list and warn the user that removing said layer(s) would result in
> > footprints being removed.
> >
> > Thanks,
> >
> > Wayne
> >
>
> I had a look at this patch.
>
> I do not think removing something to footprints already on board is a good
> idea.
>
> I understand other board items can or must be removed when disabling a
> layer, but removing something
> to a footprint is breaking this footprint, that become no more reliable.
>
> What happens if later, a disabled layer like a silkscreen is re-enabled
> for some reason?
> Footprints carefully designed are now broken.
>
> Like Seth, I am thinking disabling a layer (disabling is not deleting)
> should not modify footprints.
>
> Currently, the Layer Setup dialog can create issues because it allows
> disabling layers that are now
> used in DRC (edge cuts, courtyard, and margin that should be used in V6 to
> create obstacles).
> Some other layers are mandatory to make a board: solder mask, solder paste.
> These layers should be *always* enabled.
>
> So a better fix is certainly not to delete something in footprints, but do
> not allow disabling these
> mandatory layers, and for others layers, display a warning if a disabled
> layer is in use in a footprint.
>
> For me, the major bug is in the Layer Setup dialog that allows disabling
> any layer.
>
>
> > On 3/20/2018 4:19 AM, Eeli Kaikkonen wrote:
> >> 2018-03-20 0:19 GMT+02:00 Seth Hillbrand  >> >:
> >>
> >>
> >> As it is, the patch resolves an issue and creates another.
> >>
> >>
> >> Actually Seth is wrong here. It doesn't create another problem. Namely,
> >> as the code without the patch works now, it leaves the board uneditable
> >> anyways, and without a warning. Just test with a footprint which has
> >> nothing but ref and value and one paste-only pad. It doesn't matter
> >> whether the pad is left there or removed after the layer is deleted. The
> >> footprint can't be selected or edited.
> >>
> >> I would still go with this patch, just add a sentence to the warning if
> >> pads are deleted. "Additionally this may lead to footprints which cannot
> >> be edited or deleted" or something like that.
>
> --
> Jean-Pierre CHARRAS
>
> ___
> 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
>



-- 
Remember The Past, Live The Present, Change The Future
Those who look only to the past or the present are certain to miss the
future [JFK]

kandre...@gmail.com
Live Long and Prosper,
Andrey
___
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] Fix for bug/1754049

[jp charras]

Le 21/03/2018 à 17:46, Wayne Stambaugh a écrit :
> JP,
> 
> Did you take a look at this patch?  I know we have talked about this in
> the past and that the fix would not be easy.  Until we can define and
> implement a complete solution, this could be a short term fix.  When you
> get a chance, please take a look at it an comment on it.
> 
> hauptmech,
> 
> I'm not sure about the idea of breaking a footprint (module) into layer
> by layer pieces to match the removed layers.  Footprints are typically
> thought of as atomic objects.  I wonder if it wouldn't be more prudent
> to remove the footprint if any of it's layers are removed from the layer
> list and warn the user that removing said layer(s) would result in
> footprints being removed.
> 
> Thanks,
> 
> Wayne
> 

I had a look at this patch.

I do not think removing something to footprints already on board is a good idea.

I understand other board items can or must be removed when disabling a layer, 
but removing something
to a footprint is breaking this footprint, that become no more reliable.

What happens if later, a disabled layer like a silkscreen is re-enabled for 
some reason?
Footprints carefully designed are now broken.

Like Seth, I am thinking disabling a layer (disabling is not deleting) should 
not modify footprints.

Currently, the Layer Setup dialog can create issues because it allows disabling 
layers that are now
used in DRC (edge cuts, courtyard, and margin that should be used in V6 to 
create obstacles).
Some other layers are mandatory to make a board: solder mask, solder paste.
These layers should be *always* enabled.

So a better fix is certainly not to delete something in footprints, but do not 
allow disabling these
mandatory layers, and for others layers, display a warning if a disabled layer 
is in use in a footprint.

For me, the major bug is in the Layer Setup dialog that allows disabling any 
layer.


> On 3/20/2018 4:19 AM, Eeli Kaikkonen wrote:
>> 2018-03-20 0:19 GMT+02:00 Seth Hillbrand > >:
>>
>>
>> As it is, the patch resolves an issue and creates another.
>>
>>
>> Actually Seth is wrong here. It doesn't create another problem. Namely,
>> as the code without the patch works now, it leaves the board uneditable
>> anyways, and without a warning. Just test with a footprint which has
>> nothing but ref and value and one paste-only pad. It doesn't matter
>> whether the pad is left there or removed after the layer is deleted. The
>> footprint can't be selected or edited.
>>
>> I would still go with this patch, just add a sentence to the warning if
>> pads are deleted. "Additionally this may lead to footprints which cannot
>> be edited or deleted" or something like that.

-- 
Jean-Pierre CHARRAS

___
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] Fix for bug/1754049

[Eeli Kaikkonen]

2018-03-22 1:02 GMT+02:00 Seth Hillbrand :

> With the patch, the state became that you had a valid board but could no
> longer delete or edit the module.
>

I repeat: this state doesn't come with the patch. It exists already.
Without the patch you will have a pad like this:

(pad 1 smd circle (at -0.38 -2.54) (size 1.524 1.524) (layers))

With the patch the pad would be deleted. But in both cases the footprint
becomes non-selectable if there are no other pads or other items which make
it selectable. The faulty pad above doesn't make it selectable. That's why
I said that the patch doesn't create any new problem.

Eeli Kaikkonen
___
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] Fix for bug/1754049

[Seth Hillbrand]

Hi Eeli-

My comment above was to note that previously, we could get into a state
where you needed to delete the module to get back to a valid board.  With
the patch, the state became that you had a valid board but could no longer
delete or edit the module.  This is what I meant by solving one problem and
creating another.

It is not a question of whether we fix this bug.  It is only how we fix
it.  I have a preference for keeping the modules unchanged by the layer
setup.  This minimizes the number of nag screens and prevents data loss.
This is the direction I'm working at the moment.

Best-
Seth

2018-03-21 15:08 GMT-07:00 Eeli Kaikkonen :

>
>
> 2018-03-21 20:16 GMT+02:00 Wayne Stambaugh :
>
>> Sounds reasonable.  If I'm reading the comments correctly, I don't like
>> the idea that the current proposed patch leaves footprints in a state
>> where they cannot be selected and edited.  I'm sure that will quickly
>> produce another but report.
>>
>>
> Now I have to say again that the fix doesn't actually cause that
> situation. It happens already. If there is a footprint with only pads with
> e.g. paste layer only and then the paste layer is removed from the board,
> the footprint can't be selected. It happens already without the patch.
> Apparently my bug report was the first one and the patch wouldn't trigger
> more problems. Of course I understand that footprints should be always
> selectable, and the fact they are not is a bug on its own.
>
> BTW, also the situation when such a footprint is added to a board which
> lacks the necessary layer should be handled. It also leads to a
> non-selectable footprint.
>
> But now, let's be realistic. What are the odds that people have footprints
> which don't have copper pads but have paste-only pads or mask-only pads and
> then they remove that layer, leaving the footprint non-selectable? After
> all, this happens *only* when a pad has only paste or mask and no copper
> *and* the footprint doesn't have any pads with other layers. I have seen
> enough to have learned to never say "why would anybody ever do that", but
> it seems very unlikely, and removing a copper layer which already has pads
> seems also very unlikely (well... it seems to be impossible to do it from
> the UI).
>
> Another thing to remember is that my original report was about having data
> lost by accident because there was no warning. If users are warned about
> consequences when they do something they do it at their own risk, right? I
> won't run into the same situation again even if the bug remains unfixed, so
> I don't have an axe to grind, but it would feel weird if it wouldn't be
> fixed now, considering that even the patch wouldn't lead to any *new*
> problems and that the original problem would have been prevented by only
> adding necessary checks to the code and a warning to the dialog.
>
> Eeli Kaikkonen
>
___
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] Fix for bug/1754049

[Eeli Kaikkonen]

2018-03-21 20:16 GMT+02:00 Wayne Stambaugh :

> Sounds reasonable.  If I'm reading the comments correctly, I don't like
> the idea that the current proposed patch leaves footprints in a state
> where they cannot be selected and edited.  I'm sure that will quickly
> produce another but report.
>
>
Now I have to say again that the fix doesn't actually cause that situation.
It happens already. If there is a footprint with only pads with e.g. paste
layer only and then the paste layer is removed from the board, the
footprint can't be selected. It happens already without the patch.
Apparently my bug report was the first one and the patch wouldn't trigger
more problems. Of course I understand that footprints should be always
selectable, and the fact they are not is a bug on its own.

BTW, also the situation when such a footprint is added to a board which
lacks the necessary layer should be handled. It also leads to a
non-selectable footprint.

But now, let's be realistic. What are the odds that people have footprints
which don't have copper pads but have paste-only pads or mask-only pads and
then they remove that layer, leaving the footprint non-selectable? After
all, this happens *only* when a pad has only paste or mask and no copper
*and* the footprint doesn't have any pads with other layers. I have seen
enough to have learned to never say "why would anybody ever do that", but
it seems very unlikely, and removing a copper layer which already has pads
seems also very unlikely (well... it seems to be impossible to do it from
the UI).

Another thing to remember is that my original report was about having data
lost by accident because there was no warning. If users are warned about
consequences when they do something they do it at their own risk, right? I
won't run into the same situation again even if the bug remains unfixed, so
I don't have an axe to grind, but it would feel weird if it wouldn't be
fixed now, considering that even the patch wouldn't lead to any *new*
problems and that the original problem would have been prevented by only
adding necessary checks to the code and a warning to the dialog.

Eeli Kaikkonen
___
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] Fix for bug/1754049

[Wayne Stambaugh]

Sounds reasonable.  If I'm reading the comments correctly, I don't like
the idea that the current proposed patch leaves footprints in a state
where they cannot be selected and edited.  I'm sure that will quickly
produce another but report.

On 3/21/2018 1:36 PM, Seth Hillbrand wrote:
> Hi Wayne-
> 
> Perhaps there should be a clear limit to layers that remove full
> footprints.  Maybe only copper layers?  Otherwise, a user accidentally
> removing F.Mask removes most of their footprints.  Warning or not, that
> seems extreme.
> 
> -S
> 
> 2018-03-21 9:46 GMT-07:00 Wayne Stambaugh  >:
> 
> JP,
> 
> Did you take a look at this patch?  I know we have talked about this in
> the past and that the fix would not be easy.  Until we can define and
> implement a complete solution, this could be a short term fix.  When you
> get a chance, please take a look at it an comment on it.
> 
> hauptmech,
> 
> I'm not sure about the idea of breaking a footprint (module) into layer
> by layer pieces to match the removed layers.  Footprints are typically
> thought of as atomic objects.  I wonder if it wouldn't be more prudent
> to remove the footprint if any of it's layers are removed from the layer
> list and warn the user that removing said layer(s) would result in
> footprints being removed.
> 
> Thanks,
> 
> Wayne
> 
> On 3/20/2018 4:19 AM, Eeli Kaikkonen wrote:
> > 2018-03-20 0:19 GMT+02:00 Seth Hillbrand  
> > >>:
> >
> >
> >     As it is, the patch resolves an issue and creates another.
> >
> >
> > Actually Seth is wrong here. It doesn't create another problem. Namely,
> > as the code without the patch works now, it leaves the board uneditable
> > anyways, and without a warning. Just test with a footprint which has
> > nothing but ref and value and one paste-only pad. It doesn't matter
> > whether the pad is left there or removed after the layer is deleted. The
> > footprint can't be selected or edited.
> >
> > I would still go with this patch, just add a sentence to the warning if
> > pads are deleted. "Additionally this may lead to footprints which cannot
> > be edited or deleted" or something like that.
> >
> >
> > ___
> > 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
> 
> 
> 

___
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] Fix for bug/1754049

[Seth Hillbrand]

Hi Wayne-

Perhaps there should be a clear limit to layers that remove full
footprints.  Maybe only copper layers?  Otherwise, a user accidentally
removing F.Mask removes most of their footprints.  Warning or not, that
seems extreme.

-S

2018-03-21 9:46 GMT-07:00 Wayne Stambaugh :

> JP,
>
> Did you take a look at this patch?  I know we have talked about this in
> the past and that the fix would not be easy.  Until we can define and
> implement a complete solution, this could be a short term fix.  When you
> get a chance, please take a look at it an comment on it.
>
> hauptmech,
>
> I'm not sure about the idea of breaking a footprint (module) into layer
> by layer pieces to match the removed layers.  Footprints are typically
> thought of as atomic objects.  I wonder if it wouldn't be more prudent
> to remove the footprint if any of it's layers are removed from the layer
> list and warn the user that removing said layer(s) would result in
> footprints being removed.
>
> Thanks,
>
> Wayne
>
> On 3/20/2018 4:19 AM, Eeli Kaikkonen wrote:
> > 2018-03-20 0:19 GMT+02:00 Seth Hillbrand  > >:
> >
> >
> > As it is, the patch resolves an issue and creates another.
> >
> >
> > Actually Seth is wrong here. It doesn't create another problem. Namely,
> > as the code without the patch works now, it leaves the board uneditable
> > anyways, and without a warning. Just test with a footprint which has
> > nothing but ref and value and one paste-only pad. It doesn't matter
> > whether the pad is left there or removed after the layer is deleted. The
> > footprint can't be selected or edited.
> >
> > I would still go with this patch, just add a sentence to the warning if
> > pads are deleted. "Additionally this may lead to footprints which cannot
> > be edited or deleted" or something like that.
> >
> >
> > ___
> > 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
>
___
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] Fix for bug/1754049

[Wayne Stambaugh]

JP,

Did you take a look at this patch?  I know we have talked about this in
the past and that the fix would not be easy.  Until we can define and
implement a complete solution, this could be a short term fix.  When you
get a chance, please take a look at it an comment on it.

hauptmech,

I'm not sure about the idea of breaking a footprint (module) into layer
by layer pieces to match the removed layers.  Footprints are typically
thought of as atomic objects.  I wonder if it wouldn't be more prudent
to remove the footprint if any of it's layers are removed from the layer
list and warn the user that removing said layer(s) would result in
footprints being removed.

Thanks,

Wayne

On 3/20/2018 4:19 AM, Eeli Kaikkonen wrote:
> 2018-03-20 0:19 GMT+02:00 Seth Hillbrand  >:
> 
> 
> As it is, the patch resolves an issue and creates another.
> 
> 
> Actually Seth is wrong here. It doesn't create another problem. Namely,
> as the code without the patch works now, it leaves the board uneditable
> anyways, and without a warning. Just test with a footprint which has
> nothing but ref and value and one paste-only pad. It doesn't matter
> whether the pad is left there or removed after the layer is deleted. The
> footprint can't be selected or edited.
> 
> I would still go with this patch, just add a sentence to the warning if
> pads are deleted. "Additionally this may lead to footprints which cannot
> be edited or deleted" or something like that.
> 
> 
> ___
> 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

Re: [Kicad-developers] [PATCH] Fix for bug/1754049

[Eeli Kaikkonen]

2018-03-20 0:19 GMT+02:00 Seth Hillbrand :

>
> As it is, the patch resolves an issue and creates another.
>
>
Actually Seth is wrong here. It doesn't create another problem. Namely, as
the code without the patch works now, it leaves the board uneditable
anyways, and without a warning. Just test with a footprint which has
nothing but ref and value and one paste-only pad. It doesn't matter whether
the pad is left there or removed after the layer is deleted. The footprint
can't be selected or edited.

I would still go with this patch, just add a sentence to the warning if
pads are deleted. "Additionally this may lead to footprints which cannot be
edited or deleted" or something like that.
___
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] Fix for bug/1754049

[hauptmech]

On 20/03/18 14:07, Seth Hillbrand wrote:



2018-03-19 17:18 GMT-07:00 hauptmech >:


On 20/03/18 11:19, Seth Hillbrand wrote:

Hi hauptmech-

I disagree with your assessment of whether we should allow the
creation of broken boards.  I don't think that we should ever
allow KiCad to create a board that it cannot modify itself. Doing
this is to invite confusion and bug reports.


Seth I'd rather you say that you think this puts the board in a
broken state and you don't think we should do it. I'm ok  with
that. But when you summarize my assessment using your own framing
and then disagree with it, it makes it hard to work on this
objectively.


​That's fair.  Your phrasing is much better.  I'm sorry my statement 
misrepresented your assessment.

All good.


If you can update your patch to allow for fully-editable boards after 
removing layers, I'll have another look at it.


I'm not sure I can. I don't have any more free time for a while. You'll 
have to figure out which is worse, the bug as it is now or the patch as 
it is now.


I think allowing KiCad to delete/edit a module using only the remaining 
reference point is the best option if someone picks this bug up.





Best-
​S​



___
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] Fix for bug/1754049

[Seth Hillbrand]

2018-03-19 17:18 GMT-07:00 hauptmech :

> On 20/03/18 11:19, Seth Hillbrand wrote:
>
> Hi hauptmech-
>
> I disagree with your assessment of whether we should allow the creation of
> broken boards.  I don't think that we should ever allow KiCad to create a
> board that it cannot modify itself.  Doing this is to invite confusion and
> bug reports.
>
>
> Seth I'd rather you say that you think this puts the board in a broken
> state and you don't think we should do it. I'm ok  with that. But when you
> summarize my assessment using your own framing and then disagree with it,
> it makes it hard to work on this objectively.
>
>
​That's fair.  Your phrasing is much better.  I'm sorry my statement
misrepresented your assessment.

If you can update your patch to allow for fully-editable boards after
removing layers, I'll have another look at it.

Best-
​S​
___
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] Fix for bug/1754049

[hauptmech]

On 20/03/18 11:19, Seth Hillbrand wrote:

Hi hauptmech-

I disagree with your assessment of whether we should allow the 
creation of broken boards.  I don't think that we should ever allow 
KiCad to create a board that it cannot modify itself.  Doing this is 
to invite confusion and bug reports.


Seth I'd rather you say that you think this puts the board in a broken 
state and you don't think we should do it. I'm ok  with that. But when 
you summarize my assessment using your own framing and then disagree 
with it, it makes it hard to work on this objectively.


I don't think leaving things as they are will result in broken boards. 
The boards will load and save cleanly until the GUI can catch up.


I think the best option of what you suggested is to allow selecting 
modules via the reference point. This allows selecting (via box select I 
presume) modules that have nothing showing on any visible layers. 
However, this is beyond what I am able to do for this patch.


I am able to implement either of the other two options suggested, 
deleting the module or preventing deletion of the layer. The layers are 
being deleted because of an issue with the DRC according to code 
comments, so preventing deletion may not have the affect that is desired.


I already said why I don't think deleting the modules are the way to go, 
however I am happy to implement whatever you guys want.


Were this work for version 6 I'd say that the kicad team really needs to 
rethink the setup layers dialog and how it fits into the workflow.


Anyway, let me know. I just got a pile of new work and would like to put 
this to rest.


I don't have an opinion as to which is preferable, deleting all of the 
module or preventing deletion of a selectable layer.  But either one 
will allow KiCad to edit the file later.  Alternatively, we might 
consider allowing KiCad to delete/edit a module using only the 
remaining reference point.


As it is, the patch resolves an issue and creates another.  I'd prefer 
we didn't kick the can down the road like this.


-S



2018-03-18 2:09 GMT-07:00 hauptmech >:


Updated patch attached. Coding standard stuff fixed.

I took a second look at the case where all items in a module are
deleted.

I checked the parser and writer. In the case where all items are
deleted, the modules are still OK as far as reading and writing to
the file.

I don't think deleting the module is the right way to go. They are
on layers which cannot be deleted in the current implementation.

Most modules will have pads on the undeleteable layers, so they
will be fine.

For the case of someone with modules composed only of
non-top/bottom elements and who are messing with the layer setup
midway through the design, I think we should just leave it as it
is. Either they are a ninja doing things that kicad should stay
out of the way of  or they are a very confused beginner for whom a
little extra cruft in the file is the least of their problems.

-hauptmech




On 15/03/18 05:17, Seth Hillbrand wrote:

Hi hautmech-

Looks good.  Two comments:

- Code style.  There are a couple of small issues: one line is
too long, there should be spaces in parentheses for
pad->GetParent()->Remove(pad) and no space before parentheses in
"if ( pad->GetLayerSet()..."

- We need to handle the case where all items in a module are
deleted.  Currently, the code will remove all items except the
value and reference.  This means that we can no longer select the
module in pcbnew to delete it. Either we need to fully delete the
module when the last selectable item is removed or we need to
keep sufficient information to allow the module to be selected
when the layers are re-enabled.

Best-
Seth

2018-03-14 2:54 GMT-07:00 hauptmech mailto:hauptm...@gmail.com>>:

https://bugs.launchpad.net/kicad/+bug/1754049


This patch follows the edict (stated in
dialog_layers_setup.cpp) that items on a layer that is
removed must be deleted.



___
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

Re: [Kicad-developers] [PATCH] Fix for bug/1754049

[Seth Hillbrand]

Hi hauptmech-

I disagree with your assessment of whether we should allow the creation of
broken boards.  I don't think that we should ever allow KiCad to create a
board that it cannot modify itself.  Doing this is to invite confusion and
bug reports.

I don't have an opinion as to which is preferable, deleting all of the
module or preventing deletion of a selectable layer.  But either one will
allow KiCad to edit the file later.  Alternatively, we might consider
allowing KiCad to delete/edit a module using only the remaining reference
point.

As it is, the patch resolves an issue and creates another.  I'd prefer we
didn't kick the can down the road like this.

-S



2018-03-18 2:09 GMT-07:00 hauptmech :

> Updated patch attached. Coding standard stuff fixed.
>
> I took a second look at the case where all items in a module are deleted.
>
> I checked the parser and writer. In the case where all items are deleted,
> the modules are still OK as far as reading and writing to the file.
>
> I don't think deleting the module is the right way to go. They are on
> layers which cannot be deleted in the current implementation.
>
> Most modules will have pads on the undeleteable layers, so they will be
> fine.
>
> For the case of someone with modules composed only of non-top/bottom
> elements and who are messing with the layer setup midway through the
> design, I think we should just leave it as it is. Either they are a ninja
> doing things that kicad should stay out of the way of  or they are a very
> confused beginner for whom a little extra cruft in the file is the least of
> their problems.
>
> -hauptmech
>
>
>
>
> On 15/03/18 05:17, Seth Hillbrand wrote:
>
> Hi hautmech-
>
> Looks good.  Two comments:
>
> - Code style.  There are a couple of small issues: one line is too long,
> there should be spaces in parentheses for pad->GetParent()->Remove(pad) and
> no space before parentheses in "if ( pad->GetLayerSet()..."
>
> - We need to handle the case where all items in a module are deleted.
> Currently, the code will remove all items except the value and reference.
> This means that we can no longer select the module in pcbnew to delete it.
> Either we need to fully delete the module when the last selectable item is
> removed or we need to keep sufficient information to allow the module to be
> selected when the layers are re-enabled.
>
> Best-
> Seth
>
> 2018-03-14 2:54 GMT-07:00 hauptmech :
>
>> https://bugs.launchpad.net/kicad/+bug/1754049
>>
>> This patch follows the edict (stated in dialog_layers_setup.cpp) that
>> items on a layer that is removed must be deleted.
>>
>>
>>
>> ___
>> 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

Re: [Kicad-developers] [PATCH] Fix for bug/1754049

[Jon Evans]

I don't have a strong opinion on the functionality of this patch (I tested
it and it works as advertised) but there is one more style issue; blank
lines are needed before each if statement
https://kicad-source-mirror.readthedocs.io/en/stable/Documentation/development/coding-style-policy/#42-blank-lines-blank_lines

and also the big if...else block under the for loop in
TransferDataFromWindow() should probably be enclosed in braces -- it is
valid C++ without the braces, but our style guide expects braces unless the
body of the loop is a one-liner.

(I could fix these in-place but I'll let Seth chime in since he reviewed
this originally)

-Jon

On Sun, Mar 18, 2018 at 5:23 AM, Eeli Kaikkonen 
wrote:

>
>
> 2018-03-18 11:09 GMT+02:00 hauptmech :
>
>>
>>
>> For the case of someone with modules composed only of non-top/bottom
>> elements and who are messing with the layer setup midway through the
>> design, I think we should just leave it as it is. Either they are a ninja
>> doing things that kicad should stay out of the way of  or they are a very
>> confused beginner for whom a little extra cruft in the file is the least of
>> their problems.
>>
>> -hauptmech
>>
>>
>>
> I agree. Of course an extra warning could be added to the already existing
> warning dialog. Maybe it would be good to warn anyways if footprints are
> changed because of removed layers. It may not be obvious, especially if the
> user doesn't happen to know how the fooprints used are constructed.
>
> ___
> 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

Re: [Kicad-developers] [PATCH] Fix for bug/1754049

[Eeli Kaikkonen]

2018-03-18 11:09 GMT+02:00 hauptmech :

>
>
> For the case of someone with modules composed only of non-top/bottom
> elements and who are messing with the layer setup midway through the
> design, I think we should just leave it as it is. Either they are a ninja
> doing things that kicad should stay out of the way of  or they are a very
> confused beginner for whom a little extra cruft in the file is the least of
> their problems.
>
> -hauptmech
>
>
>
I agree. Of course an extra warning could be added to the already existing
warning dialog. Maybe it would be good to warn anyways if footprints are
changed because of removed layers. It may not be obvious, especially if the
user doesn't happen to know how the fooprints used are constructed.
___
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] Fix for bug/1754049

[hauptmech]

Updated patch attached. Coding standard stuff fixed.

I took a second look at the case where all items in a module are deleted.

I checked the parser and writer. In the case where all items are 
deleted, the modules are still OK as far as reading and writing to the file.


I don't think deleting the module is the right way to go. They are on 
layers which cannot be deleted in the current implementation.


Most modules will have pads on the undeleteable layers, so they will be 
fine.


For the case of someone with modules composed only of non-top/bottom 
elements and who are messing with the layer setup midway through the 
design, I think we should just leave it as it is. Either they are a 
ninja doing things that kicad should stay out of the way of  or they are 
a very confused beginner for whom a little extra cruft in the file is 
the least of their problems.


-hauptmech



On 15/03/18 05:17, Seth Hillbrand wrote:

Hi hautmech-

Looks good. Two comments:

- Code style. There are a couple of small issues: one line is too 
long, there should be spaces in parentheses for 
pad->GetParent()->Remove(pad) and no space before parentheses in "if ( 
pad->GetLayerSet()..."


- We need to handle the case where all items in a module are deleted. 
Currently, the code will remove all items except the value and 
reference.  This means that we can no longer select the module in 
pcbnew to delete it.  Either we need to fully delete the module when 
the last selectable item is removed or we need to keep sufficient 
information to allow the module to be selected when the layers are 
re-enabled.


Best-
Seth

2018-03-14 2:54 GMT-07:00 hauptmech >:


https://bugs.launchpad.net/kicad/+bug/1754049


This patch follows the edict (stated in dialog_layers_setup.cpp)
that items on a layer that is removed must be deleted.



___
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





>From a1dee36c3f0fde28646d5448d7aacd25996e159d Mon Sep 17 00:00:00 2001
From: hauptmech 
Date: Wed, 14 Mar 2018 22:31:59 +1300
Subject: [PATCH] Fix Layer setup can be changed leading to data loss Edit
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="2.16.2"

This is a multi-part message in MIME format.
--2.16.2
Content-Type: text/plain; charset=UTF-8; format=fixed
Content-Transfer-Encoding: 8bit


Check for items inside modules and delete them if they belong to the layer we are removing.
Pads get the offending layer removed from their layers list and deleted if they have no more layers.

Fixes: lp:1754049
https://bugs.launchpad.net/kicad/+bug/1754049
---
 pcbnew/collectors.cpp  | 14 +-
 pcbnew/dialogs/dialog_layers_setup.cpp | 27 ++-
 2 files changed, 39 insertions(+), 2 deletions(-)


--2.16.2
Content-Type: text/x-patch; name="0001-Fix-Layer-setup-can-be-changed-leading-to-data-loss-.patch"
Content-Transfer-Encoding: 8bit
Content-Disposition: attachment; filename="0001-Fix-Layer-setup-can-be-changed-leading-to-data-loss-.patch"

diff --git a/pcbnew/collectors.cpp b/pcbnew/collectors.cpp
index 2219886c2..6e1477f52 100644
--- a/pcbnew/collectors.cpp
+++ b/pcbnew/collectors.cpp
@@ -65,6 +65,9 @@ const KICAD_T GENERAL_COLLECTOR::BoardLevelItems[] = {
 PCB_VIA_T,
 PCB_TRACE_T,
 PCB_MODULE_T,
+PCB_MODULE_TEXT_T,
+PCB_MODULE_EDGE_T,
+PCB_PAD_T,
 PCB_ZONE_T,
 PCB_ZONE_AREA_T,
 EOT
@@ -499,7 +502,16 @@ SEARCH_RESULT PCB_LAYER_COLLECTOR::Inspect( EDA_ITEM* testItem, void* testData )
 {
 BOARD_ITEM* item = (BOARD_ITEM*) testItem;
 
-if( item->GetLayer() == m_layer_id )
+if( item->Type() == PCB_PAD_T )
+{
+D_PAD* pad = nullptr;
+
+wxASSERT( !pad );
+pad = static_cast( item );
+if( pad->GetLayerSet()[m_layer_id] )
+Append( testItem );
+}
+else if( item->GetLayer() == m_layer_id )
 Append( testItem );
 
 return SEARCH_CONTINUE;
diff --git a/pcbnew/dialogs/dialog_layers_setup.cpp b/pcbnew/dialogs/dialog_layers_setup.cpp
index 28b4d2d33..0c9817b2e 100644
--- a/pcbnew/dialogs/dialog_layers_setup.cpp
+++ b/pcbnew/dialogs/dialog_layers_setup.cpp
@@ -35,6 +35,7 @@
 #include 
 
 #include 
+#include 
 
 
 // some define to choose how copper layers widgets are shown
@@ -655,7 +656,31 @@ bool DIALOG_LAYERS_SETUP::TransferDataFromWindow()
 if( collector.GetCount() != 0 )
 {
 for( int i = 0; i < collector.GetCount(); i++ )
- 

Re: [Kicad-developers] [PATCH] Fix for bug/1754049

[hauptmech]

On 15 Mar 2018 05:17, "Seth Hillbrand"  wrote:

Hi hautmech-

Looks good.  Two comments:

- Code style.  There are a couple of small issues: one line is too long,
there should be spaces in parentheses for pad->GetParent()->Remove(pad) and
no space before parentheses in "if ( pad->GetLayerSet()..."


Thanks, I'll fix that.


- We need to handle the case where all items in a module are deleted.
Currently, the code will remove all items except the value and reference.
This means that we can no longer select the module in pcbnew to delete it.
Either we need to fully delete the module when the last selectable item is
removed or we need to keep sufficient information to allow the module to be
selected when the layers are re-enabled.


Is there a preferred approach?

If we don't delete it, then we need to keep the minimum valid info to round
trip to the file (need to check what this is) rather than worry about if it
is selectable, no? UI functionality always lags the internal and external
tools...


Best-
Seth

2018-03-14 2:54 GMT-07:00 hauptmech :

> https://bugs.launchpad.net/kicad/+bug/1754049
>
> This patch follows the edict (stated in dialog_layers_setup.cpp) that
> items on a layer that is removed must be deleted.
>
>
>
> ___
> 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

Re: [Kicad-developers] [PATCH] Fix for bug/1754049

[Seth Hillbrand]

Hi hautmech-

Looks good.  Two comments:

- Code style.  There are a couple of small issues: one line is too long,
there should be spaces in parentheses for pad->GetParent()->Remove(pad) and
no space before parentheses in "if ( pad->GetLayerSet()..."

- We need to handle the case where all items in a module are deleted.
Currently, the code will remove all items except the value and reference.
This means that we can no longer select the module in pcbnew to delete it.
Either we need to fully delete the module when the last selectable item is
removed or we need to keep sufficient information to allow the module to be
selected when the layers are re-enabled.

Best-
Seth

2018-03-14 2:54 GMT-07:00 hauptmech :

> https://bugs.launchpad.net/kicad/+bug/1754049
>
> This patch follows the edict (stated in dialog_layers_setup.cpp) that
> items on a layer that is removed must be deleted.
>
>
>
> ___
> 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

Re: [Kicad-developers] [PATCH] Fix for bug/1754049

[Eeli Kaikkonen]

Wrong alert, the compilation completed and it seems to work. Apparently
"collector" handles it.

2018-03-14 15:41 GMT+02:00 Eeli Kaikkonen :

> If I read your patch correctly it's only halfway there. You should
> possibly modify getRemovedLayersWithItems(), too. Otherwise it looses even
> more data without a warning. Am I correct?
>
> 2018-03-14 11:54 GMT+02:00 hauptmech :
>
>> https://bugs.launchpad.net/kicad/+bug/1754049
>>
>> This patch follows the edict (stated in dialog_layers_setup.cpp) that
>> items on a layer that is removed must be deleted.
>>
>>
>>
>> ___
>> 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

Re: [Kicad-developers] [PATCH] Fix for bug/1754049

[Eeli Kaikkonen]

If I read your patch correctly it's only halfway there. You should possibly
modify getRemovedLayersWithItems(), too. Otherwise it looses even more data
without a warning. Am I correct?

2018-03-14 11:54 GMT+02:00 hauptmech :

> https://bugs.launchpad.net/kicad/+bug/1754049
>
> This patch follows the edict (stated in dialog_layers_setup.cpp) that
> items on a layer that is removed must be deleted.
>
>
>
> ___
> 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