Bug 358158

Comment 1 Oleg Besedin 2011-09-21 11:00:19 EDT

The last part of HandlerSelectionFunction#compute() does not seem to be needed:

			// "super call"
			IEclipseContext parent = context.getParent();
			if (parent == null) {
				return null;
			}
			return parent.get(HandlerServiceImpl.H_ID + commandId);


It seems to me that not only it uses up a bit of extra CPU, but also results might be incorrect. For instance, if I am a dialog that does not have a handler for "paste", but my parent is an editor that does have a handler, then "paste" contents instead of the dialog will go to the editor.

Paul, is that desirable?

Comment 2 Oleg Besedin 2011-09-21 11:00:54 EDT

(It was added in the bug 318930 )

Comment 3 Oleg Besedin 2011-09-21 16:42:39 EDT

Correction: when writing the bug I used results from profiler. 

In some cases those results get skewed by profiler instrumenting the code. Check with direct calls to System.nanoTime shows that HandlerSelectionFunction#compute() uses about 25% of the time in the {open view, close view} sequence.

Comment 4 Paul Webster 2011-09-23 15:27:49 EDT

(In reply to comment #1)
>             IEclipseContext parent = context.getParent();
>             if (parent == null) {
>                 return null;
>             }
>             return parent.get(HandlerServiceImpl.H_ID + commandId);
> 

I'm not sure why it wouldn't just be "return context.get(HandlerServiceImpl.H_ID + commandId);", unless there originally was a conflict between a handler and a List of handlers at that ID.

PW

Comment 5 Oleg Besedin 2011-09-26 11:20:12 EDT

Also this code in PartSite:

	private void initializeDefaultServices() {
		IHandlerService handlerService = new LegacyHandlerService(e4Context, new ActivePartExpression(part));

This creates a default "And" expression for every handler that says that it is available for the given part.

However, the handler is added to the context of the part.

So, for every 3.x construct the "And" expression won't be needed, unless there are other bugs in the code.

It only could play a role for 4.x-style nested parts, and then it is not clear if the behavior it creates will be a desired behavior. (Logically, this will disable any handlers specified on the 4.x-style "container" parts which is probably not what we want).

What if we change this to:

		IHandlerService handlerService = new LegacyHandlerService(e4Context, null);

Comment 6 Remy Suen 2011-09-26 11:27:47 EDT

(In reply to comment #5)
> What if we change this to:
> 
>         IHandlerService handlerService = new LegacyHandlerService(e4Context,
> null);

I added this code to fix bug 355118. The bug does not come back if 'null' is passed.

Comment 7 Oleg Besedin 2011-09-26 11:32:05 EDT

Similarly to the comment 5, the default expression created in
LegacyHandlerService(IEclipseContext context):

            defaultExpression = new WorkbenchWindowExpression(window);

should not be necessary.

Comment 8 Paul Webster 2012-01-23 14:29:24 EST

Created attachment 209935 [details]
Main Menu visibility should not care about menu item enablement v01

Comment 9 Paul Webster 2012-01-23 14:36:29 EST

I've released an incremental improvement to switching parts:

http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=92f1e719d4d47017dd15c5ff4705b64870fdda69

PW

Comment 10 Paul Webster 2012-02-14 11:41:13 EST

Released http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=b31b27014f607553b86c22b01d95616f142d2260

try and only run EvaluationReferences when the variable of interest: value changes.

PW

Comment 11 Oleg Besedin 2012-02-15 11:56:37 EST

Using current code base, HandlerSelectionFunction#compute() takes about 7% of the time in the open view / close view cycle.

Comment 12 Oleg Besedin 2012-03-28 14:42:24 EDT

(In reply to comment #11)
> Using current code base, HandlerSelectionFunction#compute() takes about 7% of
> the time in the open view / close view cycle.