Am Do., 23. Aug. 2018 um 00:51 Uhr schrieb Andrew Lutsenko < [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]> 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]>: >> >>> 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
