Andrew, I merged your patch into the development branch. Thank you for your contribution.
Cheers, Wayne On 8/27/2018 12:26 AM, Andrew Lutsenko wrote: > Awesome! > > I updated the docs for plugin writers. Please see attached file. > > On Sun, Aug 26, 2018 at 7:48 PM Seth Hillbrand <[email protected] > <mailto:[email protected]>> wrote: > > Merged. > > Thank you for your contribution to KiCad! > > -Seth > > Am So., 26. Aug. 2018 um 13:27 Uhr schrieb Andrew Lutsenko > <[email protected] <mailto:[email protected]>>: > > 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 > <[email protected] <mailto:[email protected]>> wrote: > > > > Am Do., 23. Aug. 2018 um 12:38 Uhr schrieb Andrew Lutsenko > <[email protected] <mailto:[email protected]>>: > > 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 > <[email protected] <mailto:[email protected]>> wrote: > > > > Am Do., 23. Aug. 2018 um 00:51 Uhr schrieb Andrew > Lutsenko <[email protected] > <mailto:[email protected]>>: > > 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 > <[email protected] <mailto:[email protected]>> > 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 <[email protected] > <mailto:[email protected]>>: > > 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 <[email protected] > <mailto:[email protected]>> 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 <[email protected] > <mailto:[email protected]>> 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 <[email protected] > <mailto:[email protected]>>: > > 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 > <[email protected] > <mailto:[email protected]>> > 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 > <[email protected] > > <mailto:[email protected]>>: > > 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 > <[email protected] > <mailto:[email protected]>> > 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 : > > [email protected] > > <mailto:[email protected]> > > Unsubscribe : > > https://launchpad.net/~kicad-developers > > More help : > > https://help.launchpad.net/ListHelp > > > > > _______________________________________________ > Mailing list: > > https://launchpad.net/~kicad-developers > Post to : > > [email protected] > > <mailto:[email protected]> > Unsubscribe : > > https://launchpad.net/~kicad-developers > More help : > > https://help.launchpad.net/ListHelp > > > _______________________________________________ > Mailing list: > > https://launchpad.net/~kicad-developers > Post to : > > [email protected] > > <mailto:[email protected]> > Unsubscribe : > > https://launchpad.net/~kicad-developers > More help : > > https://help.launchpad.net/ListHelp > > > _______________________________________________ > Mailing list: https://launchpad.net/~kicad-developers > Post to : [email protected] > Unsubscribe : https://launchpad.net/~kicad-developers > More help : https://help.launchpad.net/ListHelp > _______________________________________________ Mailing list: https://launchpad.net/~kicad-developers Post to : [email protected] Unsubscribe : https://launchpad.net/~kicad-developers More help : https://help.launchpad.net/ListHelp
