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]>: > 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]> > 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]> >> 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]>: >>> >>>> 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]> >>>> 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]>: >>>>> >>>>>> 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]> 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 : [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 >>>>>> >>>>>
_______________________________________________ Mailing list: https://launchpad.net/~kicad-developers Post to : [email protected] Unsubscribe : https://launchpad.net/~kicad-developers More help : https://help.launchpad.net/ListHelp
