kicad-developers team mailing list archive

[Wayne Stambaugh <stambaughw@xxxxxxxxx>]

+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 <jeff@xxxxxxxxx
> <mailto:jeff@xxxxxxxxx>> 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.McInerney@xxxxxxxx
>>     <mailto:Ian.S.McInerney@xxxxxxxx>> 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 <jeff@xxxxxxxxx
>>     <mailto:jeff@xxxxxxxxx>> 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.McInerney@xxxxxxxx <mailto:Ian.S.McInerney@xxxxxxxx>>
>>>         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@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
>