| Thread Previous • Date Previous • Date Next • Thread Next |
Andrew,
I merged your patch into the development branch. Thank you for your
contribution.
Cheers,
Wayne
On 8/27/2018 12:26 AM, Andrew Lutsenko wrote:
> Awesome!
>
> I updated the docs for plugin writers. Please see attached file.
>
> On Sun, Aug 26, 2018 at 7:48 PM Seth Hillbrand <seth@xxxxxxxxxxxxx
> <mailto:seth@xxxxxxxxxxxxx>> wrote:
>
> Merged.
>
> Thank you for your contribution to KiCad!
>
> -Seth
>
> Am So., 26. Aug. 2018 um 13:27 Uhr schrieb Andrew Lutsenko
> <anlutsenko@xxxxxxxxx <mailto:anlutsenko@xxxxxxxxx>>:
>
> 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
> <seth@xxxxxxxxxxxxx <mailto:seth@xxxxxxxxxxxxx>> wrote:
>
>
>
> Am Do., 23. Aug. 2018 um 12:38 Uhr schrieb Andrew Lutsenko
> <anlutsenko@xxxxxxxxx <mailto:anlutsenko@xxxxxxxxx>>:
>
> 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
> <seth@xxxxxxxxxxxxx <mailto:seth@xxxxxxxxxxxxx>> wrote:
>
>
>
> Am Do., 23. Aug. 2018 um 00:51 Uhr schrieb Andrew
> Lutsenko <anlutsenko@xxxxxxxxx
> <mailto:anlutsenko@xxxxxxxxx>>:
>
> 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
> <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
>
| Thread Previous • Date Previous • Date Next • Thread Next |
