Comment 1 Oliver Pütter 2013-12-03 10:34:07 EST

If all parts within the workbench are closed the method setPart of the SelectionAggregator is called with parameter part = null. In this case the active part has to be set to null. Otherwise there is an inconsistent state of the selection aggregator if one part is reopened. The selection of the newly opened part would not be tracked correctly.

Comment 2 Oliver Pütter 2013-12-03 10:36:32 EST

Possible fix:

@Inject
void setPart(@Optional @Named(IServiceConstants.ACTIVE_PART) final MPart part) {
   if (part == null) {
	activePart = null;
	context.set(IServiceConstants.ACTIVE_SELECTION, null);
   } else if (activePart != part) {
	activePart = part;
	IEclipseContext partContext = part.getContext();
	if (partContext.containsKey(OUT_POST_SELECTION)) {
	    Object selection = partContext.get(OUT_POST_SELECTION);
	    context.set(IServiceConstants.ACTIVE_SELECTION, selection);
	} else if (partContext.containsKey(OUT_SELECTION)) {
	    Object selection = partContext.get(OUT_SELECTION);
	    context.set(IServiceConstants.ACTIVE_SELECTION, selection);
	}
	track(part);
   }
}

Comment 3 Oliver Pütter 2013-12-04 02:56:24 EST

Created attachment 237991 [details]
Patch

Comment 4 Lars Vogel 2013-12-04 07:14:07 EST

Please always link you Gerrit review into the bug report.

Comment 5 Oliver Pütter 2013-12-04 07:26:26 EST

Gerrit review

https://git.eclipse.org/r/#/c/19303/

Comment 6 Oliver Pütter 2013-12-04 07:27:13 EST

pushed to master and 4.3 maintenance

Comment 7 Lars Vogel 2013-12-04 07:29:13 EST

(In reply to Oliver Pütter from comment #6)
> pushed to master and 4.3 maintenance

I think the process is that the committer decide what is pushed down to the maintenance branch. So IMHO it is sufficient if you a fix for master.

Comment 8 Dani Megert 2013-12-05 07:04:09 EST

(In reply to Oliver Pütter from comment #1)
> If all parts within the workbench are closed the method setPart of the
> SelectionAggregator is called with parameter part = null. In this case the
> active part has to be set to null. Otherwise there is an inconsistent state
> of the selection aggregator if one part is reopened. The selection of the
> newly opened part would not be tracked correctly.

Olivier, do you have steps that allow to see the bug and verify the fix?

Comment 9 Oliver Pütter 2013-12-05 08:26:22 EST

I cannot submit any lines of code due to restrictions from my company.
 
We implement a part with a tree viewer on which a context menu is registered. In the corresponding handlers we evaluate the active selection injected with IServiceConstants.ACTIVE_SELECTION in a method annotated with @CanExecute

If all parts are closed SelectionAggregator.setPart is at last called with part = NULL, but activePart is NOT set to NULL in the current implementation. If you reopen the part SelectionAggregator.setPart is called with part <> NULL but activePart is also <> NULL and IServiceConstants.ACTIVE_SELECTION is not set.

So far I did not find a use case in the current IDE to reproduce the bug.

Comment 10 Paul Webster 2013-12-16 10:12:13 EST

Created attachment 238377 [details]
Example with one view and ESelectionService

Oliver, can you work with this?  I'm waiting for selection changes in a long lived object off of MWindow.  It works in the beginning, but 1) closing the view doesn't send a null and 2) re-opening the view doesn't provide any selection events at all.

PW

Comment 11 Oliver Pütter 2013-12-16 11:10:25 EST

Yes. This is exact the behaviour.

Comment 12 Paul Webster 2013-12-16 14:08:05 EST

So with your fix, re-opening the view allows it to work again?

PW

Comment 13 Oliver Pütter 2013-12-17 02:52:49 EST

Yes. Your sample works fine with our fix.

Comment 14 Dani Megert 2013-12-17 04:57:44 EST

Submitted with http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=9d4f71484d3b007fe86e292d00c5ecb02b9de5ab

Comment 15 Paul Webster 2014-01-21 14:25:32 EST

In 4.4.0.I20140120-2000

PW