On 8/23/2018 3:50 AM, Andrew Lutsenko wrote: > 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. > > 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.
We do not use github for kicad source development. It is merely a mirror for user convenience. Please do not open any issues on github for the kicad source mirror. Cheers, Wayne > > Andrew > > On Wed, Aug 22, 2018 at 2:03 PM Seth Hillbrand <s...@hillbrand.org > <mailto: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 <mailto: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 <mailto: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 <mailto: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 > <mailto: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 <mailto: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 > <mailto: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 > <mailto: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> > <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 > <mailto: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 > <mailto: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 > <mailto: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