kicad-developers team mailing list archive

[Seth Hillbrand <seth@xxxxxxxxxxxxx>]

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

Attachment: Screenshot_2018-08-23_10-54-57.png
Description: PNG image