Thanks, I opened merge request on launchpad: https://code.launchpad.net/~qu1ck/kicad/+git/kicad/+merge/353677
> 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. On Thu, Aug 23, 2018 at 10:56 AM Seth Hillbrand <s...@hillbrand.org> wrote: > > > Am Do., 23. Aug. 2018 um 00:51 Uhr schrieb Andrew Lutsenko < > anlutse...@gmail.com>: > >> 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 <s...@hillbrand.org> >> 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 < >>> anlutse...@gmail.com>: >>> >>>> 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 <anlutse...@gmail.com> >>>> 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 <s...@hillbrand.org> >>>>> 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 < >>>>>> anlutse...@gmail.com>: >>>>>> >>>>>>> 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 <s...@hillbrand.org> >>>>>>> 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 < >>>>>>>> anlutse...@gmail.com>: >>>>>>>> >>>>>>>>> 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 <c...@embeon.de> >>>>>>>>> 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@lists.launchpad.net >>>>>>>>>> > Unsubscribe : https://launchpad.net/~kicad-developers >>>>>>>>>> > More help : https://help.launchpad.net/ListHelp >>>>>>>>>> > >>>>>>>>>> >>>>>>>>>> _______________________________________________ >>>>>>>>>> Mailing list: https://launchpad.net/~kicad-developers >>>>>>>>>> Post to : kicad-developers@lists.launchpad.net >>>>>>>>>> Unsubscribe : https://launchpad.net/~kicad-developers >>>>>>>>>> More help : https://help.launchpad.net/ListHelp >>>>>>>>>> >>>>>>>>> _______________________________________________ >>>>>>>>> Mailing list: https://launchpad.net/~kicad-developers >>>>>>>>> Post to : kicad-developers@lists.launchpad.net >>>>>>>>> Unsubscribe : https://launchpad.net/~kicad-developers >>>>>>>>> More help : https://help.launchpad.net/ListHelp >>>>>>>>> >>>>>>>>
_______________________________________________ Mailing list: https://launchpad.net/~kicad-developers Post to : kicad-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~kicad-developers More help : https://help.launchpad.net/ListHelp
