kicad-developers team mailing list archive
-
kicad-developers team
-
Mailing list archive
-
Message #37218
Re: [PATCH] RFC: toolbar button support for action plugins
On 8/23/2018 3:50 AM, Andrew Lutsenko wrote:
> 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.
>
> 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.
We do not use github for kicad source development. It is merely a
mirror for user convenience. Please do not open any issues on github
for the kicad source mirror.
Cheers,
Wayne
>
> Andrew
>
> On Wed, Aug 22, 2018 at 2:03 PM Seth Hillbrand <seth@xxxxxxxxxxxxx
> <mailto:seth@xxxxxxxxxxxxx>> 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
> <anlutsenko@xxxxxxxxx <mailto:anlutsenko@xxxxxxxxx>>:
>
> 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
> <anlutsenko@xxxxxxxxx <mailto:anlutsenko@xxxxxxxxx>> 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
> <seth@xxxxxxxxxxxxx <mailto:seth@xxxxxxxxxxxxx>> 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 <anlutsenko@xxxxxxxxx
> <mailto:anlutsenko@xxxxxxxxx>>:
>
> 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
> <seth@xxxxxxxxxxxxx <mailto:seth@xxxxxxxxxxxxx>> 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 <anlutsenko@xxxxxxxxx
> <mailto:anlutsenko@xxxxxxxxx>>:
>
> 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 <cko@xxxxxxxxx
> <mailto:cko@xxxxxxxxx>> 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>
> <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@xxxxxxxxxxxxxxxxxxx
> <mailto:kicad-developers@xxxxxxxxxxxxxxxxxxx>
> > Unsubscribe :
> https://launchpad.net/~kicad-developers
> > More help :
> https://help.launchpad.net/ListHelp
> >
>
> _______________________________________________
> Mailing list:
> https://launchpad.net/~kicad-developers
> Post to :
> kicad-developers@xxxxxxxxxxxxxxxxxxx
> <mailto:kicad-developers@xxxxxxxxxxxxxxxxxxx>
> Unsubscribe :
> https://launchpad.net/~kicad-developers
> More help :
> https://help.launchpad.net/ListHelp
>
> _______________________________________________
> Mailing list:
> https://launchpad.net/~kicad-developers
> Post to :
> kicad-developers@xxxxxxxxxxxxxxxxxxx
> <mailto:kicad-developers@xxxxxxxxxxxxxxxxxxx>
> Unsubscribe :
> https://launchpad.net/~kicad-developers
> More help :
> https://help.launchpad.net/ListHelp
>
>
> _______________________________________________
> Mailing list: https://launchpad.net/~kicad-developers
> Post to : kicad-developers@xxxxxxxxxxxxxxxxxxx
> Unsubscribe : https://launchpad.net/~kicad-developers
> More help : https://help.launchpad.net/ListHelp
>
Follow ups
References
-
[PATCH] RFC: toolbar button support for action plugins
From: Andrew Lutsenko, 2018-08-15
-
Re: [PATCH] RFC: toolbar button support for action plugins
From: Clemens Koller, 2018-08-16
-
Re: [PATCH] RFC: toolbar button support for action plugins
From: Andrew Lutsenko, 2018-08-17
-
Re: [PATCH] RFC: toolbar button support for action plugins
From: Seth Hillbrand, 2018-08-17
-
Re: [PATCH] RFC: toolbar button support for action plugins
From: Andrew Lutsenko, 2018-08-17
-
Re: [PATCH] RFC: toolbar button support for action plugins
From: Seth Hillbrand, 2018-08-17
-
Re: [PATCH] RFC: toolbar button support for action plugins
From: Andrew Lutsenko, 2018-08-17
-
Re: [PATCH] RFC: toolbar button support for action plugins
From: Andrew Lutsenko, 2018-08-22
-
Re: [PATCH] RFC: toolbar button support for action plugins
From: Seth Hillbrand, 2018-08-22
-
Re: [PATCH] RFC: toolbar button support for action plugins
From: Andrew Lutsenko, 2018-08-23
