Hi Ian, Sounds good. Carry on. ;)
Cheers, Jeff. > On 13 Aug 2019, at 22:34, Ian McInerney <[email protected]> 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 <[email protected] > <mailto:[email protected]>> 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 <[email protected] >> <mailto:[email protected]>> 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 <[email protected] >> <mailto:[email protected]>> 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 <[email protected] >>> <mailto:[email protected]>> wrote: >>> >>> Following on from the discussion in this merge request >>> (https://code.launchpad.net/~imcinerney/kicad/+git/kicad/+merge/371156 >>> <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 >>> <https://launchpad.net/~kicad-developers> >>> Post to : [email protected] >>> <mailto:[email protected]> >>> Unsubscribe : https://launchpad.net/~kicad-developers >>> <https://launchpad.net/~kicad-developers> >>> More help : https://help.launchpad.net/ListHelp >>> <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
