kicad-developers team mailing list archive

[Seth Hillbrand <seth@xxxxxxxxxxxxx>]

Hi Andrew-

I like the patch idea and your implementation approach is good.

The coding style policy is located at
https://kicad-source-mirror.readthedocs.io/en/stable/Documentation/development/coding-style-policy/
We're not totally consistent but we try to ensure any new code follows it
and clean up the old code as we go.

On the errors, please don't throw an error to the user.  It may be useful
to insert a wxLog() call that we could utilize in the future but that is
ignored for now.

I'm opposed to merging patches that are partially complete.  And I would
consider the lack of user control over this feature problematic.  This is
not a judgement on the patch as you've implemented it.  I really like the
functionality and think KiCad users will appreciate it as well.  However, a
partially implemented feature creates the opportunity for problems down the
road that will distract developer time from the other tasks they are
working on.  If you do not have the time to fully implement user control
over this feature (I completely understand competing time pressures), you
may consider opening a bug report and attaching your patch there for
interested future developers.  Squashing the patchset for review is also a
good idea.

Overall this looks really promising.

Best-
Seth

Am Do., 16. Aug. 2018 um 23:02 Uhr schrieb Andrew Lutsenko <
anlutsenko@xxxxxxxxx>:

> Hi Seth
>
> I just checked out new preferences in pcbnew, looks much more organized
> than before.
> I totally can add a new tab there "pcbnew->Action plugins" and list the
> plugins there with option
> to remove toolbar icon. But that is a non-negligible amount of work. Will
> you hold off on merging
> my current patches until I implement that too?
> By default plugins will not show any buttons on toolbar, plugin writers
> will have to explicitly update
> their plugins and provide an icon for them to show up so you will not run
> into an issue with full
> toolbar for a while. See my screenshot in second email of the chain, it
> has 4 plugins but only
> 2 of them register with an icon and toolbar button.
>
> > headers get 1 space between function defs
> I tried to follow existing style in each file and didn't notice that it's
> not consistent across different files.
> action_plugin.h has two new lines between most functions but I can change
> it to one.
>
> What do you think about throwing an error to user if icon failed to load?
> Andrey Kuznetsov made a
> point that user can't do anything about it anyway. I agree that asking
> users to fix plugin icon declaration
> is a bit much and developer will be able to see that icon didn't load
> without the error too.
>
> Andrew
>
> On Thu, Aug 16, 2018 at 10:22 PM Seth Hillbrand <seth@xxxxxxxxxxxxx>
> wrote:
>
>> Hi Andrew-
>>
>> I like the idea.  Aside from minor formatting (headers get 1 space
>> between function defs, need a space before the if block), the patch looks
>> good.
>>
>> However, I wouldn't want everything showing on my toolbar (speaking as
>> someone who has 10 plugins installed, 5 of which get regular use).  I'd
>> prefer the option to be configurable.  This should probably be in the
>> preferences pane that Jeff has been re-working.
>>
>> -Seth
>>
>> Am Do., 16. Aug. 2018 um 22:11 Uhr schrieb Andrew Lutsenko <
>> anlutsenko@xxxxxxxxx>:
>>
>>> Hi Clemens,
>>>
>>> See sample plugin attached. Extract it into kicad's
>>> share/scripting/plugins folder.
>>> One of other scanned directories that are documented in kicadplugins.i
>>> <https://github.com/KiCad/kicad-source-mirror/blob/6fdc5972f8431b4d5831a32649e67bfe20d05de8/scripting/kicadplugins.i#L180> should
>>> work too.
>>>
>>> Or are you asking to update docs in the repo?
>>> Documentation/development/pcbnew-plugins.md seems like the right place.
>>> I will update it once committers agree with the path I've chosen to
>>> implement this.
>>>
>>>
>>>
>>> On Thu, Aug 16, 2018 at 4:48 AM Clemens Koller <cko@xxxxxxxxx> wrote:
>>>
>>>> Hello,  Andrew!
>>>>
>>>> I am somehow missing the Python example you are giving in your patch.
>>>> Can you add this a simple example to the sources to get a basic/dummy
>>>> skeleton working right from scratch?
>>>>
>>>> Regards,
>>>>
>>>> Clemens
>>>>
>>>>
>>>> On 15/08/2018 11.31, Andrew Lutsenko wrote:
>>>> > Hi KiCad devs,
>>>> >
>>>> > I am proposing an addition to plugin system.
>>>> > Probably most will agree that menus suck. Toolbars suck less :)
>>>> >
>>>> > In my plugin I added a dirty hack to modify top toolbar from plugin
>>>> init code to add a button
>>>> > that calls plugins run() method. It is broken on linux X11 and is not
>>>> a sustainable way others
>>>> > can add buttons in their plugins. But having a button was quite
>>>> popular among users so I
>>>> > decided to implement this functionality directly in pcbnew.
>>>> >
>>>> > I introduced one more field plugin writers can define in defaults()
>>>> that contains path to png icon
>>>> > and if that string is not empty, pcbnew will attempt to load that
>>>> icon and add a button to top
>>>> > toolbar with action that calls the same run() method. I traced in
>>>> code how plugin action menu
>>>> > is generated and added similar logic for buttons.
>>>> >
>>>> > Here is how the result looks like:
>>>> >
>>>> > https://i.imgur.com/f3xg1FE.gif
>>>> >
>>>> > Sample dummy plugin __init__.py code:
>>>> >
>>>> > import os
>>>> > import pcbnew
>>>> > import wx
>>>> >
>>>> > class Plugin1(pcbnew.ActionPlugin):
>>>> >
>>>> >     def defaults(self):
>>>> >         self.name <http://self.name> = "Dummy Plugin 1"
>>>> >         self.category = "Read PCB"
>>>> >         self.description = ""
>>>> >         self.icon_file_name = os.path.join(os.path.dirname(__file__),
>>>> 'icon.png')
>>>> >
>>>> >     def Run(self):
>>>> >         wx.MessageBox("Plugin 1")
>>>> >
>>>> > Plugin1().register()
>>>> >
>>>> > It's as simple as that.
>>>> >
>>>> > The patch is attached. It probably needs some error checking but
>>>> seems to be working great.
>>>> > Tested in win64 so far.
>>>> > I'm open to suggestions on how to get it to good state, I will also
>>>> test on linux asap.
>>>> >
>>>> > Regards,
>>>> > Andrew
>>>> >
>>>> > _______________________________________________
>>>> > Mailing list: https://launchpad.net/~kicad-developers
>>>> > Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx
>>>> > Unsubscribe : https://launchpad.net/~kicad-developers
>>>> > More help   : https://help.launchpad.net/ListHelp
>>>> >
>>>>
>>>> _______________________________________________
>>>> Mailing list: https://launchpad.net/~kicad-developers
>>>> Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx
>>>> Unsubscribe : https://launchpad.net/~kicad-developers
>>>> More help   : https://help.launchpad.net/ListHelp
>>>>
>>> _______________________________________________
>>> Mailing list: https://launchpad.net/~kicad-developers
>>> Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx
>>> Unsubscribe : https://launchpad.net/~kicad-developers
>>> More help   : https://help.launchpad.net/ListHelp
>>>
>>