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