kicad-developers team mailing list archive

[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-Add-toolbar-buttons-for-action-plugins.patch
Description: Binary data