kicad-developers team mailing list archive

[Andrew Lutsenko <anlutsenko@xxxxxxxxx>]

Awesome!

I updated the docs for plugin writers. Please see attached file.

On Sun, Aug 26, 2018 at 7:48 PM Seth Hillbrand <seth@xxxxxxxxxxxxx> wrote:

> Merged.
>
> Thank you for your contribution to KiCad!
>
> -Seth
>
> Am So., 26. Aug. 2018 um 13:27 Uhr schrieb Andrew Lutsenko <
> anlutsenko@xxxxxxxxx>:
>
>> Thanks all for the review.
>>
>> Now that proposal is approved I'm not sure what next steps are. Only docs
>> I could find on launchpad are about bazaar.
>> But I would guess commit has to be manually merged. I attached approved
>> revision of the patch here.
>>
>> On Thu, Aug 23, 2018 at 1:50 PM Seth Hillbrand <seth@xxxxxxxxxxxxx>
>> wrote:
>>
>>>
>>>
>>> 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
>>>>>>>>>>>>>
>>>>>>>>>>>>

Attachment: 0001-Update-pcbnew-plugins.md-with-info-about-icons.patch
Description: Binary data