kicad-developers team mailing list archive

[Seth Hillbrand <seth@xxxxxxxxxxxxx>]

Thanks Andrew, I'll have a chance to test this in detail this weekend.  The
movie looks really nice.

Short comments:

- You should protect array dereference indices to prevent overflow.  It
would be a good idea to define a specific "no icon" index (-1 maybe?  or 0)
that returns a default icon (0) or prevents an icon from being loaded (-1).

- It looks like your editor doesn't automatically clear trailing whitespace
at the end of lines.  Can you check if there's an option in it that does
that for you?

- There are a couple small spacing errors (single line between function
defs, space before closing parens)

Overall, excellent work!  This feels like a nice addition for people who
regularly use plugins.

-Seth

Am Mi., 22. Aug. 2018 um 04:28 Uhr schrieb Andrew Lutsenko <
anlutsenko@xxxxxxxxx>:

> Hi Seth,
>
> I built the settings dialog for action plugins. You can reorder and
> enable/disable buttons for each plugin individually.
>
> Short demo:
> https://i.imgur.com/33iJC7b.gif
>
> Squashed patch is attached. I've tested it on win, debian8 and debian9.
> If it's easier to review diff can be viewed here as well:
>
> https://github.com/KiCad/kicad-source-mirror/compare/master...qu1ck:plugin-icon
>
> Also I've attached few dummy plugins to play with, 3 out of 4 have icons.
>
> Let me know if you have any comments.
>
> Thanks,
> Andrew
>
> On Fri, Aug 17, 2018 at 3:02 PM Andrew Lutsenko <anlutsenko@xxxxxxxxx>
> wrote:
>
>> Hi Seth,
>>
>> That makes sense. I will keep working on this feature and will ping this
>> thread again once user configuration is implemented.
>>
>> Thanks,
>> Andrew
>>
>> On Fri, Aug 17, 2018 at 10:03 AM Seth Hillbrand <seth@xxxxxxxxxxxxx>
>> wrote:
>>
>>> 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
>>>>>>
>>>>>