Merged. Thank you for your contribution to KiCad!
-Seth Am So., 26. Aug. 2018 um 13:27 Uhr schrieb Andrew Lutsenko < anlutse...@gmail.com>: > 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 <s...@hillbrand.org> wrote: > >> >> >> Am Do., 23. Aug. 2018 um 12:38 Uhr schrieb Andrew Lutsenko < >> anlutse...@gmail.com>: >> >>> 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 <s...@hillbrand.org> >>> wrote: >>> >>>> >>>> >>>> Am Do., 23. Aug. 2018 um 00:51 Uhr schrieb Andrew Lutsenko < >>>> anlutse...@gmail.com>: >>>> >>>>> 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 <s...@hillbrand.org> >>>>> 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 < >>>>>> anlutse...@gmail.com>: >>>>>> >>>>>>> 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 < >>>>>>> anlutse...@gmail.com> 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 <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