← Back to team overview

kicad-developers team mailing list archive

Re: [PATCH] Refactor COMMON_ACTIONS into a base and derived class

 

Hi Orson,

I've attached a follow-up patch that moves all TOOL_ACTIONs out of
pcb_actions.cpp and creates a COMMON_TOOLS class for storing
cross-application tools.  I was not able to move all zoom/grid tools
because of dependencies on pcbnew that need to be resolved -- I did not
want to take on the refactoring needed to fix this in this patch, but I
plan on looking at it in the near future.

Best,
Jon

On Mon, Feb 20, 2017 at 11:13 AM, Maciej Sumiński <maciej.suminski@xxxxxxx>
wrote:

> Thank you John, we really appreciate your efforts.
>
> Regards,
> Orson
>
> On 02/20/2017 03:50 PM, Jon Evans wrote:
> > Hi Orson,
> >
> > I can definitely pull the pcb_actions into their respective files, I will
> > do that and send another patch.
> >
> > Best,
> > Jon
> >
> > On Mon, Feb 20, 2017 at 4:25 AM, John Beard <john.j.beard@xxxxxxxxx>
> wrote:
> >
> >> HI Orson,
> >>
> >> I think that sounds like a sensible idea. Having a huge central list
> >> of actions has a bit of a code smell for me, as it's a big header than
> >> then needs including everywhere. Smaller lists that are included along
> >> with their tool's headers (if needed), or even actions that are
> >> totally hidden in implementations for when the action only works
> >> during an interactive tool feels much nicer.
> >>
> >> The file now called common_actions.cpp (maybe pcb_actions.cpp or
> >> something in future) would need to include most of these headers so it
> >> can still map legacy event IDs, but that's how it should be - a file
> >> that needs lots, includes lots.
> >>
> >> Cheers,
> >>
> >> John
> >>
> >>
> >> On Mon, Feb 20, 2017 at 4:58 PM, Maciej Sumiński
> >> <maciej.suminski@xxxxxxx> wrote:
> >>> Hi Jon,
> >>>
> >>> I see the point of your patch, as COMMON_ACTIONS are now a bit misused.
> >>> They should not keep majority of the TOOL_ACTIONs, as many of them are
> >>> pcbnew specific, but there are still actions that will be shared with
> >>> other applications (e.g. zoom & grid control, move/rotate/flip).
> >>>
> >>> For some time I was also wondering whether it would not be better to
> >>> move the actions to their corresponding tools, as is done e.g. in
> >>> pcbnew/router/router_tool.cpp (ACT_* objects), and leave only truly
> >>> generic actions in {COMMON,PCB}_ACTIONS.
> >>>
> >>> What do you think about splitting the current set to PCB_ACTIONS and
> >>> COMMON_ACTIONS, perhaps moving some of them to the tools source files?
> >>>
> >>> Regards,
> >>> Orson
> >>>
> >>> On 02/17/2017 04:56 AM, Jon Evans wrote:
> >>>> Hi all,
> >>>>
> >>>> More preparation for GerbView GAL port: this patch pulls a virtual
> >> ACTIONS
> >>>> class out of pcbnew and renames the COMMON_ACTIONS to PCB_ACTIONS for
> >>>> clarity.
> >>>>
> >>>> Best,
> >>>> Jon
> >>>>
> >>>>
> >>>>
> >>>> _______________________________________________
> >>>> 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
> >>>>
> >>>
> >>>
> >>>
> >>> _______________________________________________
> >>> 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
> >>>
> >>
> >> _______________________________________________
> >> 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