kicad-developers team mailing list archive

[Andrew Lutsenko <anlutsenko@xxxxxxxxx>]

Thanks, I opened merge request on launchpad:
https://code.launchpad.net/~qu1ck/kicad/+git/kicad/+merge/353677

> ACTION_PLUGINS::GetActionButton.  Takes an integer and picks the element
from the actions list vector.  But std::vector[] doesn't provide bounds
checking.  You could use at() and catch the out_of_range error.

That method is not used. I only added it because there was ::GetActionMenu,
which has the same issue you mentioned. It is also unused, maybe I should
remove both.

On Thu, Aug 23, 2018 at 10:56 AM Seth Hillbrand <seth@xxxxxxxxxxxxx> wrote:

>
>
> Am Do., 23. Aug. 2018 um 00:51 Uhr schrieb Andrew Lutsenko <
> anlutsenko@xxxxxxxxx>:
>
>> I fixed whitespace issue and formatting (at least what I caught).
>> Fixed a bug that would not load plugin list properly if one plugin was
>> removed and another was added at the same time.
>> Also made plugin buttons scale same as the main buttons.
>>
>> Please see updated patch attached.
>>
>> > 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).
>>
>> I am not sure what place in code are you referring to here. Nowhere do I
>> reference icons by index. Icons are fields of action plugins and I get them
>> from a map, not a vector.
>>
>
> ACTION_PLUGINS::GetActionButton.  Takes an integer and picks the element
> from the actions list vector.  But std::vector[] doesn't provide bounds
> checking.  You could use at() and catch the out_of_range error.
>
>
>
>> Would you prefer if I opened a PR on github? I know it won't be merged,
>> just to make exchanging comments on code easier during review.
>>
>
> No, I'd prefer not to split the conversation.  However, you can make a
> branch on launchpad and then make a merge proposal.  That should be the
> same workflow and keeps information on our primary platform. (see attached
> screenshot for details on where this button is hidden)
>
> -S
>
>
>
>>
>> Andrew
>>
>> On Wed, Aug 22, 2018 at 2:03 PM Seth Hillbrand <seth@xxxxxxxxxxxxx>
>> wrote:
>>
>>> 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
>>>>>>>>>
>>>>>>>>