kicad-developers team mailing list archive

[Maciej Sumiński <maciej.suminski@xxxxxxx>]

Hi Miles,

On 02/27/2018 09:20 AM, miles mccoo wrote:
> Is the patch required? It appears that I don't need it.
> 
> However...
> 
> I would like to know what formatting errors I made in the patch. Avoid it
> next time.

- in .cpp files there are two empty lines between consecutive methods

+BOARD_COMMIT*
+NewBoardCommit()
+{
+    if( s_PcbEditFrame ) {
+        return new BOARD_COMMIT( s_PcbEditFrame );
+    }
+}

should read:

+BOARD_COMMIT* NewBoardCommit()
+{
+    if( s_PcbEditFrame )
+    {
+        return new BOARD_COMMIT( s_PcbEditFrame );
+    }
+}

Mostly it is about putting spaces in the right places. I know it is hard
to get accustomed to, especially when one works on multiple projects
using different coding styles.

> For the other folks involved in the earlier mail threads, they seemed to
> think exposing this is a good idea.
> 
>    - Perhaps they still do and so this patch would be helpful for them
>    - Perhaps they never did see the need, in which case I would say, "Grrr!"
> 

Responding as one of the people who commented on the patch: I do not
write Python scripts myself, but I realized you faced some issues and I
tried to help by explaining the logic behind view/model updates.

It is hard for me to comment on what is useful for the Python script
developers, so I would rather hear what is more convenient for people
who actually use the scripting interface. I think this is also the
reason why Wayne asked you whether the patch is needed. The only thing I
would like to avoid is frequent scripting interface modifications, so we
do not break existing scripts.

Regards,
Orson

> Miles
> 
> On Mon, Feb 26, 2018 at 1:59 PM, Wayne Stambaugh <stambaughw@xxxxxxxxx>
> wrote:
> 
>> Miles,
>>
>> Does this mean your patch is not required?  If it's not necessary, then
>> I would prefer to not merge it.
>>
>> Wayne
>>
>> On 2/26/2018 5:29 AM, miles mccoo wrote:
>>> So I think I may have discovered something that renders this patch
>>> irrelevant.
>>>
>>> I was trying to debug the behavior I mentioned in the first mail of this
>>> thread, where undo via BOARD_COMMIT doesn't work right when running code
>>> from within an external plugin.
>>>
>>> Turns out that the external plugin code handles undo, redraw,... for me.
>>> Trying to handle undo twice is what caused my issue.
>>>
>>> The funny thing is that I worked on exposing the BOARD_COMMIT stuff as a
>>> prep step to plugin'ifying a bunch of my utilities.
>>>
>>>
>>>
>>> Miles
>>>
>>>
>>> On Mon, Feb 26, 2018 at 10:23 AM, miles mccoo <mail@xxxxxxxxxx
>>> <mailto:mail@xxxxxxxxxx>> wrote:
>>>
>>>
>>>     On whether it's appropriate to expose BOARD_COMMIT to python, the
>>>     class originally came to my attention in this message (though not by
>>>     a core kicad developer?):
>>>     https://lists.launchpad.net/kicad-developers/msg32275.html
>>>     <https://lists.launchpad.net/kicad-developers/msg32275.html>
>>>
>>>
>>>     As a follow on to that suggestion, I started this thread which was
>>>     premised on the idea of exposing BOARD_COMMIT.
>>>     https://lists.launchpad.net/kicad-developers/msg32063.html
>>>     <https://lists.launchpad.net/kicad-developers/msg32063.html>
>>>
>>>     Most of the discussion in that thread centers around understanding
>>>     BC, so perhaps it wasn't noticed that exposing BC to python is the
>>>     point.
>>>
>>>
>>>     If it's preferred to omit from python, please advise on a)how to
>>>     best get redraw to work? My last patched improved, but didn't
>>>     entirely fix redraw. b)undo would be nice to have in external
>>>     plugins. How best to achieve this?
>>>
>>>
>>>
>>>
>>>
>>>     On formatting, I looked at it again and I see two errors. In one
>>>     function, I'm missing two leading spaces. On another, I have a curly
>>>     on the same line as the if. (or perhaps the curly shouldn't be
>> there?)
>>>
>>>     Is there more to it? I'm happy to resubmit but would prefer only one
>>>     iteration (assuming the consensus is that BOARD_COMMIT should be
>>>     exposed)
>>>
>>>
>>>     Miles
>>>
>>>
>>>
>>>
>>>     for reference:
>>>      #include <macros.h>
>>>      #include <stdlib.h>
>>>      #include <pcb_draw_panel_gal.h>
>>>     +#include <board_commit.h>
>>>
>>>      static PCB_EDIT_FRAME* s_PcbEditFrame = NULL;
>>>
>>>     @@ -57,6 +58,10 @@ void ScriptingSetPcbEditFrame( PCB_EDIT_FRAME*
>>>     aPcbEditFrame )
>>>          s_PcbEditFrame = aPcbEditFrame;
>>>      }
>>>
>>>     +PCB_EDIT_FRAME* GetPcbEditFrame()
>>>     +{
>>>     +  return s_PcbEditFrame;
>>>     +}
>>>
>>>      BOARD* LoadBoard( wxString& aFileName )
>>>      {
>>>     @@ -135,3 +140,12 @@ void UpdateUserInterface()
>>>          if( s_PcbEditFrame )
>>>              s_PcbEditFrame->UpdateUserInterface();
>>>      }
>>>     +
>>>     +
>>>     +BOARD_COMMIT*
>>>     +NewBoardCommit()
>>>     +{
>>>     +    if( s_PcbEditFrame ) {
>>>     +        return new BOARD_COMMIT( s_PcbEditFrame );
>>>     +    }
>>>     +}
>>>
>>>
>>>
>>>     On Sat, Feb 24, 2018 at 3:02 PM, Wayne Stambaugh
>>>     <stambaughw@xxxxxxxxx <mailto:stambaughw@xxxxxxxxx>> wrote:
>>>
>>>         Miles,
>>>
>>>         I'm not opposite to exposing the BOARD_COMMIT object but there
>>>         are some serious ramifications of doing so.  Although we already
>>>         expose objects that can crash the board editor from python
>>>         scripting so I don't know that one more will be all that
>>>         significant.  Can anyone else think of a reason not to expose
>>>         the BOARD_COMMIT object to the python scripting? Before I merged
>>>         your patch I am going to ask you to reformat the c++ code in the
>>>         pcbnew_scripting_helpers.cpp file.  If you are unsure of the
>>>         code formating, please take a look at the kicad coding policy[1].
>>>
>>>         Thanks,
>>>
>>>         Wayne
>>>
>>>         [1]:
>>>         http://docs.kicad-pcb.org/doxygen/md_Documentation_
>> development_coding-style-policy.html
>>>         <http://docs.kicad-pcb.org/doxygen/md_Documentation_
>> development_coding-style-policy.html>
>>>
>>>         On 02/24/2018 05:04 AM, miles mccoo wrote:
>>>
>>>             Back in November I was in a thread about board_commit:
>>>             https://lists.launchpad.net/kicad-developers/msg32063.html
>>>             <https://lists.launchpad.net/kicad-developers/msg32063.html>
>>>
>>>             The topic had come up in another patch:
>>>             https://lists.launchpad.net/kicad-developers/msg32134.html
>>>             <https://lists.launchpad.net/kicad-developers/msg32134.html>
>>>
>>>
>>>             See attached path file. added needed code to SWIG to enable
>>>             using BOARD_COMMIT
>>>
>>>
>>>             As a basic test, I ran the attached commit.py [1] on the
>>>             attached commit.kicad_pcb. It just moves some modules and
>>>             adds some vias. undo seems to work fine and (perhaps more
>>>             important) the canvas updates correctly (there are some bugs
>>>             in fresh that I haven't figured out)
>>>
>>>             I have also updated some of my other scripts to use it and
>>>             almost all seems well.
>>>             *
>>>             *
>>>             *The thing that doesn't work...*
>>>
>>>
>>>             If I run a script as an external plugin, undoing, brings me
>>>             back to an empty canvas. I don't know how to debug that.
>>>             Some guidance could be helpful. I don't think this is a
>>>             problem in the python interface, but rather some
>>>             multi-threading thing in BOARD_COMMIT itself.
>>>
>>>             Here's a demo of the problem:
>>>             https://youtu.be/YR-9dx_lkUo
>>>
>>>             place_by_sch described in the video is here:
>>>             https://github.com/mmccoo/kicad_mmccoo/blob/master/
>> place_by_sch/place_by_sch.py
>>>             <https://github.com/mmccoo/kicad_mmccoo/blob/master/
>> place_by_sch/place_by_sch.py>
>>>
>>>             The board_commit stuff isn't in that version[2]
>>>
>>>
>>>             Miles
>>>
>>>
>>>             [1] I have a feeling the py file will be filtered, so here
>> it is
>>>             toplayer = layertable['F.Cu']
>>>             bottomlayer = layertable['B.Cu']
>>>             for mod in board.GetModules():
>>>                  print("mod {}".format(mod.GetReference()))
>>>                  newvia = pcbnew.VIA(board)
>>>                  board.Add(newvia)
>>>                  newvia.SetNet(gnd)
>>>                  nc = gnd.GetNetClass()
>>>                  newvia.SetWidth(nc.GetViaDiameter())
>>>                  newvia.SetPosition(mod.GetPosition())
>>>                  newvia.SetLayerPair(toplayer, bottomlayer)
>>>                  newvia.SetViaType(pcbnew.VIA_THROUGH)
>>>                  bc.Added(newvia)
>>>                  bc.Modify(mod)
>>>                  mod.SetPosition(pcbnew.wxPoint(mod.GetPosition().x +
>>>             pcbnew.Millimeter2iu(10),
>>>                                                 mod.GetPosition().y +
>>>             pcbnew.Millimeter2iu(10)))
>>>
>>>
>>>             bc.Push("moved stuff")
>>>
>>>
>>>             [2]
>>>             diff --git a/place_by_sch/place_by_sch.py
>>>             b/place_by_sch/place_by_sch.py
>>>             index 22bc21c..5f70354 100644
>>>             --- a/place_by_sch/place_by_sch.py
>>>             +++ b/place_by_sch/place_by_sch.py
>>>             @@ -7,7 +7,9 @@ import re
>>>               def PlaceBySch():
>>>                   board = pcbnew.GetBoard()
>>>             -
>>>             +    print("getting board commit")
>>>             +    bc = pcbnew.NewBoardCommit()
>>>             +
>>>                   board_path = board.GetFileName()
>>>                   sch_path = board_path.replace(".kicad_pcb", ".sch")
>>>             @@ -114,9 +116,10 @@ def PlaceBySch():
>>>                       # oldvalue * 25.4 / 10e4
>>>                       newx = locs[ref][0] * 25.4 * 1000.0
>>>                       newy = locs[ref][1] * 25.4 * 1000.0
>>>             +        bc.Modify(mod)
>>>                       mod.SetPosition(pcbnew.wxPoint(int(newx),
>> int(newy)))
>>>                       mod.SetOrientation(locs[ref][2]*10)
>>>                       print("placing {} at {},{}".format(ref, newx,
>> newy))
>>>             -
>>>             -    pcbnew.Refresh();
>>>             +    bc.Push("place_by_sch")
>>>             +    #pcbnew.Refresh();
>>>
>>>
>>>
>>>
>>>             for my personal reference: this is my sixth patch
>>>
>>>
>>>
>>>             _______________________________________________
>>>             Mailing list: https://launchpad.net/~kicad-developers
>>>             <https://launchpad.net/~kicad-developers>
>>>             Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx
>>>             <mailto:kicad-developers@xxxxxxxxxxxxxxxxxxx>
>>>             Unsubscribe : https://launchpad.net/~kicad-developers
>>>             <https://launchpad.net/~kicad-developers>
>>>             More help   : https://help.launchpad.net/ListHelp
>>>             <https://help.launchpad.net/ListHelp>
>>>
>>>
>>>         _______________________________________________
>>>         Mailing list: https://launchpad.net/~kicad-developers
>>>         <https://launchpad.net/~kicad-developers>
>>>         Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx
>>>         <mailto:kicad-developers@xxxxxxxxxxxxxxxxxxx>
>>>         Unsubscribe : https://launchpad.net/~kicad-developers
>>>         <https://launchpad.net/~kicad-developers>
>>>         More help   : https://help.launchpad.net/ListHelp
>>>         <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
>

Attachment: signature.asc
Description: OpenPGP digital signature