Bug 329646

Comment 1 Brian de Alwis 2010-11-09 18:30:17 EST

I've tried fiddling with this a bit more, and discovered that this problem is an event-ordering problem -- or rather, an injection-ordering problem.  

I discovered that the problem only seems to occur when the windows are created on startup.  If I re-open a previously-closed window, then its menus appear correctly with the shortcut sequences.

So I tried stepping into the sequence binding code a bit, and found an injection-ordering problem in BindingServiceImpl.  Here's the order I saw when debugging using the shipped .jar from the I builds:

1. On startup, HandledMenuItemRenderer#setItemText()'s is called to set the text for a menu item.  It fetches the EBindingService for the window's context.  

1a) This fetch causes the creation of a new BindingServiceImpl instance.

1b) BindingServiceImpl's #setContextIds() is called with a 1-element set containing the org.eclipse.ui.contexts.dialogAndWindows context.  But because contextManager == null, the instance's contextSet is set to an empty set.

1c) BindingServiceImpl's setContextManager() is then called with the contextManager.

2. HMIR#setItemText() calls the EBS#getBestSequenceFor().  Because the EBS's contextSet is empty, there are no tables to consult and so it returns null, and consequently no shortcut is shown.


If I peer at the injector's requests for the BindingServiceImpl, it has 4 items to inject: 2 fields (the IEC field "context" and BindingTableManager field "manager") and 2 methods (setContextIds() and setContextManager()).  When I run from the pre-built .jar from the I-builds, the fields are injected first, followed by the two methods, and setContextIds() *always* comes before setContextManager().  But when I run from source, the setContextManager() *always* comes before setContextIds().

The easiest patch is to mark the contextManager field as @Inject so as to ensure it is always injected before setContextIds() is called.   setContextManager() may not be necessary any more -- perhaps there's a better place to deal with setting the default ContextSet comparator?

Alternatively setContextIds() could take the ContextManager as an argument too?

Comment 2 Oleg Besedin 2010-11-10 09:00:06 EST

(In reply to comment #1)
> ... setContextIds() *always* comes before
> setContextManager().  But when I run from source, the setContextManager()
> *always* comes before setContextIds().
> The easiest patch is to mark the contextManager field as @Inject so as to
> ensure it is always injected before setContextIds() is called.  

In general, injection order is:
- fields first,
- then followed by the methods
- then @PostConstruct methods, if any

So, couple more possibilities:

- Use method injection with methods that take multiple arguments; or
- Use @PostConstruct to "finalize" functionality that sis based on multiple injected values.

Comment 3 Remy Suen 2010-11-11 06:42:30 EST

Might be the same problem as bug 320994. I often have my outer not having keybindings but my inner wouldn't have the problem.

Comment 4 Paul Webster 2010-11-22 15:06:03 EST

Created attachment 183597 [details]
restructuring of command definitions w.r.t. model v01

I'm working on a lifecycle restructuring that I hope will fix this problem.

From the compatibility layer point of view, break things into 3 pieces.

1) compat layer command model processor:  This turns the extension point into a model that can be consumed

2) e4 core commands addon: make sure the command and handler service are available and properly initialized

3) e4 modeled workbench commands addon: read the commands model and feed it into the e4 command service

For now there's also an extra step in the compatibility layer to feed in some extra runtime information not supported by the model, but that is small potatoes.

vast improvements:

1) the order is controlled very nicely

2) there's no more "add this in now and remove on shutdown" in the compatibility layer.


PW

Comment 5 Paul Webster 2010-11-23 08:29:51 EST

Created attachment 183662 [details]
restructuring of command definitions w.r.t. model v02

Fixed up all the tests.

PW

Comment 6 Paul Webster 2010-11-23 08:37:08 EST

(In reply to comment #5)
> Created an attachment (id=183662) [details]
> restructuring of command definitions w.r.t. model v02

Released to HEAD, without the .launch changes.

PW

Comment 7 Paul Webster 2010-11-24 10:54:21 EST

See bug 326190 for the general discussion.

PW

Comment 8 Paul Webster 2010-11-24 15:02:13 EST

Created attachment 183799 [details]
restructuring of context and bindings v01

This is a work in progress on restructuring the contexts and bindings, so their responsibilities are separate.

current stop: no 3.x commands to read in the bindings, as model processing happens before add-ons bring the command system up.

PW

Comment 9 Paul Webster 2010-11-29 15:13:08 EST

Created attachment 184076 [details]
Broke out the context v01

Contexts restructing doesn't depend on binding restructing

Released to HEAD
pW

Comment 10 Paul Webster 2010-11-29 15:34:00 EST

Created attachment 184077 [details]
restructing of bindings v04

Restructuring the bindings after the contexts have already been done.

PW

Comment 11 Paul Webster 2010-12-01 13:48:43 EST

I've opened  Bug 331580 and  Bug 331582  to move hard coded E4Workbench processing into the appropriate add on.

PW

Comment 12 Paul Webster 2010-12-01 14:06:21 EST

Created attachment 184287 [details]
restructuring of bindings v05

This includes removing SWT requirement from org.eclipse.e4.ui.workbench

Released to HEAD

PW

Comment 13 Paul Webster 2010-12-01 14:14:24 EST

(In reply to comment #1)
> The easiest patch is to mark the contextManager field as @Inject so as to
> ensure it is always injected before setContextIds() is called.  
> setContextManager() may not be necessary any more -- perhaps there's a better
> place to deal with setting the default ContextSet comparator?

Now to actually deal with Brian's issue :-)

Then new structure gaurantees that the correct ContextManager and that ContextSet.setComparator(*) is done in a different place at the correct time.

So I switched the ContextManager to an @Inject field.  I'll request a build for tonight so we can test it tomorrow.

PW

Comment 14 Paul Webster 2010-12-01 15:26:51 EST

Still needs to be verified.
PW