Bug 315781

Comment 1 Boris Bokowski 2010-06-29 06:52:54 EDT

Has this ever worked? RATs cannot track what't happening outside of contexts. Going to change this to update enablement when the menu is about to show.

Comment 2 Boris Bokowski 2010-06-29 07:03:17 EDT

Fix released to HEAD. Eric, could you have a look at the new code? HandledMenuItemRenderer.hookMenuAboutToShow - it might be better to hook the listener once, in the parent renderer, but I wasn't sure how best to do that. Also, the dispose logic is worth double-checking... Thanks!

Comment 3 Boris Bokowski 2010-06-29 07:04:46 EDT

Paul, do toolbar items update their enablement at all?  I couldn't find the code anymore that periodically, based on a timerExec, checks the enablement state.

Comment 4 Paul Webster 2010-06-29 08:08:12 EDT

Everybody (menu items and tool items) depends on a RAT and calling their @CanExecute.  The theory if they depend on something like "selection", "activePart", etc, they'll be updated when it changes.

There is potentially a hook for SWT.Show (although it should be done from the MenuServiceFilter, which is already responding to all SWT.Show).  There is no hook for toolbars.

Let's move this discussion to bug 318296

PW

Comment 5 Oleg Besedin 2010-06-29 09:30:29 EDT

(In reply to comment #4)
> Let's move this discussion to bug 318296

That bug is not valid - the #invoke() calls are one-time things, they do not track changes in the arguments. (The #inject() can be used to tracking links.)

Comment 6 Paul Webster 2010-06-29 09:39:15 EDT

Created attachment 172998 [details]
Save v02

This refactors Boris' change to the MenuServiceFilter which is already managing SWT.Show events.

PW

Comment 7 Paul Webster 2010-06-29 11:20:21 EDT

OK, we're having the general enabled discussion here :-)

The menu items currently use MenuServiceFilter and evaluate their enabled state on SWT.Show.  Right now, the tool items generate RAT when they are created:

final IEclipseContext lclContext = getContext(item);
lclContext.runAndTrack(new RunAndTrack() {
   @Override
   public boolean changed(IEclipseContext context) {
       if (newItem.isDisposed()) {
           return false;
       }
       EHandlerService service = lclContext.get(EHandlerService.class);
       ParameterizedCommand cmd = item.getWbCommand();
       if (cmd == null) {
           cmd = generateParameterizedCommand(item, lclContext);
       }
       if (cmd == null) {
           return false;
       }
       item.setEnabled(service.canExecute(cmd));
       return true;
   }
});

The hope was that the lookup of the handler and the calling of @CanExecute
would cause this to be re-evaluated if:

1) the handler changes and
2) any of the parameters (like "selection" or "activePart") changes

PW

Comment 8 Oleg Besedin 2010-06-29 11:26:54 EDT

Created attachment 173016 [details]
JUnit for #invoke() done inside RAT

(In reply to comment #7)
> The hope was that the lookup of the handler and the calling of @CanExecute
> would cause this to be re-evaluated if:
> 1) the handler changes and
> 2) any of the parameters (like "selection" or "activePart") changes
> PW

I think that works; I added a new JUnit InvokeInRATTest to try this and it seems to be working fine.

Comment 9 Boris Bokowski 2010-06-29 18:14:39 EDT

(In reply to comment #7)
>        item.setEnabled(service.canExecute(cmd));

> The hope was that the lookup of the handler and the calling of @CanExecute
> would cause this to be re-evaluated if:
> 
> 1) the handler changes and
> 2) any of the parameters (like "selection" or "activePart") changes

Yes, it will be re-evaluated if the handler changes, but the call above (service.canExecute) goes to client code, and we don't really know what it is doing. In the example of File>Save, it ended up calling SaveAction which keeps a boolean whether it is enabled as part of its state, not in the context.

I guess talking about this f2f would make sense, tomorrow morning.

Comment 10 Paul Webster 2010-07-05 11:21:37 EDT

We moved back to 1) calling @CanExecute on a menu about to show and 2) calling @CanExecute in a timer for tool items.  We've removed the RATs

PW