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