+1 from me. On 8/13/2019 5:34 PM, Ian McInerney wrote: > Jeff, > > This change would not affect the handling of the menu action events, it > would simply be in charge of updating the UI of the menus/taskbars to > reflect the current state of the tools. I think the biggest advantage > for doing it this way is to unify the method by which the state of all > menus/toolbars is updated. Currently, the CONDITIONAL_MENU handles the > state update for those menus but the programmer must handle it for all > other items. When a new item is added to both a menu and a toolbar they > then have to ensure it is updated properly in both places. > > What this is proposing is to unify all the update logic into something > that is abstracted away from the programmer. They simply supply the > enabling condition (as we already do for the CONDITIONAL_MENU), and the > framework handles it for them. This eliminates the need to have > SyncToolbars() at all, and the items are updated when wxWidgets wants > them not when our framework feels it is needed. > > This might remove the need to call Resolve after creating the menus, but > I haven't experimented with it. It should definitely remove the need to > call SyncToolbars() after their creation/modification though. > > I do think your suggestion about renaming CONDITIONAL_MENU to > CONTEXT_MENU is another reason to do this for the ACTION_MENUs, so then > we have a definite class demarcation between a context menu and a normal > menu. The context menu would still inherit from the normal menu, but it > would include the ability to hide the items. > > -Ian > > On Tue, Aug 13, 2019 at 12:18 AM Jeff Young <j...@rokeby.ie > <mailto:j...@rokeby.ie>> wrote: > > I’m not sure what we’re buying with the wxUpdateUIEvents then. > Context menus will still need to run the existing Evaluate() stuff > (for visibility and for the highlight hacks), at which point it > might as well do the enabling/checking too. And menu-bar menus will > also still have to run menu dispatch (for the menu-vs.-accelerator > hack). I suppose it would allow us to remove some of the GTK hacks > (such as having to resolve the menus immediately after building them). > > If we do push down the lamda stuff into ACTION_MENU so that it can > bind to wxUpdateUIEvents, we should move the menu-bar menus to > ACTION_MENU and then rename CONDITIONAL_MENU to CONTEXT_MENU. > > Cheers, > Jeff. > > >> On 12 Aug 2019, at 23:06, Ian McInerney <ian.s.mciner...@ieee.org >> <mailto:ian.s.mciner...@ieee.org>> wrote: >> >> Jeff, >> >> I was thinking we would only move the update for the enable and >> check attributes to the wxUpdateUIEvent mechanism and leave the >> visibility to the conditional menu to deal with in its Evaluate() >> function as it currently does. There is no way to get wxWidgets to >> hide items for us in menus (and GTK doesn't even seem to have a >> mechanism for it, so it will probably never be possible to >> outsource it to wxWidgets), so we will always need to handle that >> ourselves and the Evaluate() command seems to be doing a decent >> job of it. >> >> -Ian >> >> On Mon, Aug 12, 2019 at 10:19 PM Jeff Young <j...@rokeby.ie >> <mailto:j...@rokeby.ie>> wrote: >> >> Hi Ian, >> >> It sounds promising, but there are some potential pitfalls. >> wxWidgets only supports two bits: enabled and checked. >> CONDITIONAL_MENU supports 2-1/2: the enabled flag means >> visible if the is_context_menu flag is set. This is probably >> a subtlety that does us no good: it would be more direct to >> just have ACTION_MENU support 3 bits (visible, enabled and >> checked) and get rid of CONDITIONAL_MENU. >> >> However, that then begs the question of how do we handle the >> visible bit through the wxUpdateUIEvent mechanism? Maybe we >> just use the wxUpdateUIEvent as a trigger to run the >> equivalent of Evaluate()? Or maybe we just have >> wxUpdateUIEvent handle the two bits, and continue to handle >> the visible bit through the existing mechanism? (Note that at >> least some of the existing mechanism has to stay anyway >> because we use it to determine if a command is an accelerator >> or a menu pick.) >> >> Generally speaking we *could* move everything to ACTIONs. The >> stuff that’s still in the old system at present is just the >> stuff that didn’t look worth moving. So if there’s anything >> that needs synchronizing that isn’t currently an ACTION we >> should just make it an ACTION. (There are two caveats to this >> due to wxWidgets crankiness: Quit and Close. But neither of >> them need synchronizing.) >> >> Cheers, >> Jeff. >> >> >>> On 12 Aug 2019, at 20:45, Ian McInerney >>> <ian.s.mciner...@ieee.org <mailto:ian.s.mciner...@ieee.org>> >>> wrote: >>> >>> Following on from the discussion in this merge request >>> >>> (https://code.launchpad.net/~imcinerney/kicad/+git/kicad/+merge/371156), >>> I thought a little about if the current framework could be >>> adapted to use the wxUpdateUIEvent handlers to do the >>> synchronization, and I think it can be. Here is my thinking >>> of how it can be done, and I would appreciate comments about >>> this idea. >>> >>> From my understanding there are three classes that will >>> create menu/toolbar items that need to be synced: >>> ACTION_MENU, ACTION_TOOLBAR, and CONDITIONAL_MENU. The first >>> change would be to make the Add functions for all of these >>> classes take a SelectionConditions argument that will be used >>> to define the enable/checkmark status of the item (currently >>> only CONDITIONAL_MENU takes these). >>> >>> First question, do we need to synchronize any items that >>> aren't action-based? If we will only synchronize action-based >>> items, then only those Add functions would need to be extended. >>> >>> The TOOL_MANAGER would then handle the majority of the work. >>> The Add functions would call into the TOOL_MANAGER requesting >>> an event handler be created, passing the selection conditions >>> as well. >>> >>> To create the event handler, the TOOL_MANAGER would do the >>> following: >>> 1) Consult a list that associates actions with their >>> selection conditions (being on the list would indicate the >>> action already has a handler in the window containing the >>> tool manager). >>> 2) If the action is on the list, compare the provided >>> selection condition with what is already in the list, and if >>> they are different from each other, assert (this will require >>> selection conditions used in multiple places in the same tool >>> manager for the same action to point to the same object). >>> This way the code is kept synchronized as well. >>> 3) If it is not on the list, then call Bind and bind an event >>> handler for the event. The selection condition would be >>> passed into the Bind as the user data so that it can be used >>> in the handler. This handler would be bound to the parent of >>> the tool manager. >>> >>> There would be two handlers, one for checkmark entries and >>> one for enable/disable entries, and the correct one would be >>> bound based on the type of menu item. These would modify the >>> aEvent object to actually check/enable/disable the UI element >>> (like what the original handlers did). >>> >>> I believe this will remove the need for the SyncToolbars >>> functions (and the need for the tool manager to explicitly >>> call this, which had caused some issues in the past if I >>> remember correctly). It will also mean that the conditional >>> menu's Evaluate function would no longer need to do the >>> checking/enabling itself, since that is handled in the event >>> handler. >>> >>> Thoughts? >>> >>> -Ian >>> _______________________________________________ >>> Mailing list: https://launchpad.net/~kicad-developers >>> Post to : kicad-developers@lists.launchpad.net >>> <mailto: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
