kicad-developers team mailing list archive

[Seth Hillbrand <seth@xxxxxxxxxxxxx>]

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