Awesome! I updated the docs for plugin writers. Please see attached file.
On Sun, Aug 26, 2018 at 7:48 PM Seth Hillbrand <[email protected]> wrote: > Merged. > > Thank you for your contribution to KiCad! > > -Seth > > Am So., 26. Aug. 2018 um 13:27 Uhr schrieb Andrew Lutsenko < > [email protected]>: > >> 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 <[email protected]> >> wrote: >> >>> >>> >>> 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 >>>>>>>>>>>>> >>>>>>>>>>>>
0001-Update-pcbnew-plugins.md-with-info-about-icons.patch
Description: Binary data
_______________________________________________ Mailing list: https://launchpad.net/~kicad-developers Post to : [email protected] Unsubscribe : https://launchpad.net/~kicad-developers More help : https://help.launchpad.net/ListHelp
