kicad-developers team mailing list archive

[Seth Hillbrand <seth@xxxxxxxxxxxxx>]

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

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

Excellent, thanks.  I've added Jeff for a second set of eyes to look for
possible Mac issues.  I've added a few comments for formatting (they
probably sound pedantic by now)and a couple suggestions.  Once my comments
are addressed, it looks good to me.



> > 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.
>

Removing both is fine.  If we do decide to leave them in for future
implementation, they will need to be safe for callers.

-Seth

>
> 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
>>>>>>>>>>
>>>>>>>>>