Bug 375741

Comment 1 Paul Webster 2012-04-04 07:22:48 EDT

I scanned your XML and that seems fine.  popup menus should use activeMenuSelection instead of selection, but they can be the same value.


So when you edit an XML file and move the cursor, you have the JPA Structure view select the correct PersistentType or PersistentAttribute.  Then you activate the view, and right-click to bring up the popup menu (which seems to be a selection behind).

Were you just right-clicking on the selected item in the JPA view?  Is it consistent if you activate the view (click on the tab) and then right-click on the selected item?  If you change the selected item in your JPA view?

Does the JPA view register a selection provider with its part site?  Does it use the same selection provider or a different one when it calls registerContextMenu(*)?

PW

Comment 2 Brian Vosburgh 2012-04-04 15:42:09 EDT

Of course, re-creating the problem is not as consistent today. :-)

> I scanned your XML and that seems fine.  popup menus should use
> activeMenuSelection instead of selection, but they can be the same value.

I inherited much of this code and still have figured out where the variable "selection" is documented; so I have no idea why that's what we use. :-) I would appreciate any pointers.

> So when you edit an XML file and move the cursor, you have the JPA Structure
> view select the correct PersistentType or PersistentAttribute.  Then you
> activate the view, and right-click to bring up the popup menu (which seems to
> be a selection behind).

I'm not so sure the pop-up menu is a "selection behind". The JPA Structure View tree's root is a single object of type EntityMappings. If I move the selection (via the cursor in the editor) from EntityMappings to a PersistentType, the pop-up menu will be the correct menu but it will have some of its entries disabled and the selection passed to any still-active handler is a PersistentAttribute. Very odd. I would add that this "disabled" behavior happens *all the time* with the root of the tree, the EntityMappings; all the other nodes are frustratingly intermittent.

> Were you just right-clicking on the selected item in the JPA view?  Is it
> consistent if you activate the view (click on the tab) and then right-click on
> the selected item?  If you change the selected item in your JPA view?

As far as I have noticed. These problems occur if I:

- move the cursor in the editor and right-click directly on the current selection in the tree

- first click on the JPA Structure View's tab then right-click directly on the current selection in the tree

The problems do *not* occur if I:

- first *left*-click directly on the current selection in the tree, *then* right-click

- right-click on *another* node in the tree, other than the current selection

- change the tree selection with the mouse, as opposed to via the cursor in the editor

> Does the JPA view register a selection provider with its part site?  Does it
> use the same selection provider or a different one when it calls
> registerContextMenu(*)?

The JpaStructureView subclasses PageBookView and creates a separate JpaStructurePage for each JPA-related editor (typically editors for .java and .xml files). JpaStructurePage is a subclass of Page and simply holds a TreeViewer. In its override of Page.init(IPageSite) JpaStructurePage sets the site's selection provider to an adapter that simply forwards the selection to/from the TreeViewer. 

I'm not real clear on how our menu works, but here is the code we have for building and registering it:

    MenuManager menuManager = new MenuManager();
    menuManager.setRemoveAllWhenShown(true);
    menuManager.addMenuListener(new MenuListener());
    Tree tree = this.treeViewer.getTree();
    tree.setMenu(menuManager.createContextMenu(tree));
    this.structureView.getSite().registerContextMenu(menuManager, this.treeViewer);

So: No, we do not use the same selection provider when registering the context menu. We simply pass the TreeViewer itself, not our adapter. Also, the menu is registered with the JpaStructureView's site, not the page's site (which was passed to the init(IPageSite) method). Not sure what we're doing here.... Again, I would appreciate any pointers to relevant documentation. :-)

Comment 3 Paul Webster 2012-04-05 13:43:29 EDT

(In reply to comment #2)
> As far as I have noticed. These problems occur if I:
> 
> - move the cursor in the editor and right-click directly on the current
> selection in the tree
> 
> - first click on the JPA Structure View's tab then right-click directly on the
> current selection in the tree
> 
> The problems do *not* occur if I:
> 
> - first *left*-click directly on the current selection in the tree, *then*
> right-click
> 
> - right-click on *another* node in the tree, other than the current selection
> 
> - change the tree selection with the mouse, as opposed to via the cursor in the
> editor

Each page in a page book page has a org.eclipse.ui.part.IPage.setFocus() method.  Does your pages implement it?  Arguably it would just do treeViewer.getControl().setFocus().

PW

Comment 4 Paul Webster 2012-04-05 13:52:16 EDT

(In reply to comment #2)
> I'm not real clear on how our menu works, but here is the code we have for
> building and registering it:
> 
>     MenuManager menuManager = new MenuManager();
>     menuManager.setRemoveAllWhenShown(true);
>     menuManager.addMenuListener(new MenuListener());
>     Tree tree = this.treeViewer.getTree();
>     tree.setMenu(menuManager.createContextMenu(tree));
>     this.structureView.getSite().registerContextMenu(menuManager,
> this.treeViewer);
> 

It's curious why the adapter in one place but the treeviewer in the other.  In general your context menu registration follows the standard pattern, although you should be using the page getSite().registerContextMenu(*) instead of going up to the PageBookView (which is what this.structureView is?)

Just some background:  Whatever is registered as the selectionProvider for the site contributes to the window ISelectionService and to org.eclipse.ui.ISources.ACTIVE_CURRENT_SELECTION_NAME ("selection") for commands/handlers and expressions.

Context menus can use the same selection provider or a different one when they register with the framework (as it's possible to have multiple context menus in a part).  Whatever is registered contributes to org.eclipse.ui.ISources.ACTIVE_MENU_SELECTION_NAME ("activeMenuSelection") for commands/handlers and expressions for the time that the context menu is open.


PW

Comment 5 Brian Vosburgh 2012-05-04 18:44:02 EDT

Created attachment 215117 [details]
WorkbenchSourceProvider.selectionChanged(...) stack traces

Sorry about the delay.

I have tried a few of the changes you mentioned:

- The Page's setFocus() implementation is present and simply calls setFocus() on its Composite (which holds a single child, the TreeViewer's Tree).

- I changed the registering of the Page's context menu from this:

this.structureView.getSite().registerContextMenu(menuManager, this.treeViewer);

to this:

this.getSite().registerContextMenu(this.structureView.getSite().getId(), menuManager, this.selectionProvider);


I changed the menu contributions from this:

<visibleWhen>
	<with variable="selection">

to this:

<visibleWhen>
	<with variable="activeMenuSelection">

To no avail.

After further investigation, your comment that the pop-up menu is a "selection behind" is not completely off the mark. Although the tree's selection may change a number of times while I move the cursor around in the text editor, the pop-up menu seems to always get passed the selection from the last time I *left*-clicked in the tree. This also means the selection is null when I directly right-click on the tree for the very first time (meaning sometimes the pop-up menu does not pop up at all or, if it does, selecting an action does nothing).

The Page's selection provider seems to be behaving correctly. When the Page has the focus and I change the tree's selection with the mouse, notification makes its way to the SelectionService. It seems like the SelectionService (or whatever supplies the selection to the pop-up menu) does not get the Page's selection when the Page receives the focus via a "direct" right-click on the current tree selection.

And I have verified that the SelectionService adds itself as a listener to the appropriate selection provider when the tree gets the focus via a "direct" right-click.

After digging around (rather blindly), I have investigated WorkbenchSourceProvider a bit (since it seemed to be interested in my view's selection provider). I put a breakpoint in WorkbenchSourceProvider.selectionChanged(...) to see when it is notified that the selection changes, and it looks interesting. If I simply *left*-click on the tree, selectionChanged(...) is called only once, and the new selection is the correct element. If I directly right-click on the tree, selectionChanged(...) is called twice. The first time the new selection is the correct element; the second time the new selection is *incorrect*. It appears to be obsolete. It appears that the invalid selection is taken from an MPart that is passed to SelectionAggregator.setPart(...).

I have attached a text file with the stack traces for these three occurrences.

Is there anywhere else I can look?

Comment 6 Paul Webster 2012-05-07 06:46:44 EDT

Brian, thank you for your investigation.

Oleg, could we be getting hit by the double-selection calls that you just looked at?

PW

Comment 7 Oleg Besedin 2012-05-07 09:58:18 EDT

(In reply to comment #6)
> Oleg, could we be getting hit by the double-selection calls that you just
> looked at?

It is worth checking, That issue should be fixed in 4.2M7:

http://download.eclipse.org/eclipse/downloads/drops4/S-4.2M7-201205031800/

Brian, could you try it using the M7 build?

Comment 8 Brian Vosburgh 2012-05-08 18:32:04 EDT

(In reply to comment #7)
> Brian, could you try it using the M7 build?

I just tried this out on M7 and the behavior is unchanged. As I described in comment 5, WorkbenchSourceProvider.selectionChanged(...) is called twice: once with the correct selection and a second time with the obsolete selection.

Comment 9 Oleg Besedin 2012-05-09 15:30:59 EDT

Brian, what is the good way to install Dali on top of 4.2? I tried standard update sites (http://download.eclipse.org/webtools/updates/, http://download.eclipse.org/datatools/updates) but being hitting "required items could not be found" for several combinations that I tried.

Comment 10 Brian Vosburgh 2012-05-09 16:48:57 EDT

(In reply to comment #9)
> Brian, what is the good way to install Dali on top of 4.2?

I admit to always building my target platform from scratch, unzipping the appropriate files into the same directory.

The WTP M7 download page will point you to the necessary zip files:

http://download.eclipse.org/webtools/downloads/drops/R3.4.0/S-3.4.0M7-20120507135259/

Download and unzip into the same directory the following files:
    Eclipse SDK
    EMF and XSD SDK
    GEF SDK
    DTP SDK
    EMF Transaction
    EMF Validation
    WTP SDK - which is further down on the page and points to:

http://www.eclipse.org/downloads/download.php?file=/webtools/downloads/drops/R3.4.0/S-3.4.0M7-20120507135259/wtp4x-sdk-S-3.4.0M7-20120507135259.zip

This will be your target platform containing Dali.

I will attach an example project....

Comment 11 Brian Vosburgh 2012-05-09 16:55:54 EDT

Created attachment 215367 [details]
example JPA project

The attached is a simple JPA project. Open the JPA perspective once you have the project added to your workspace. In Project Explorer you should see a node under the project labeled "JPA Content", indicating that Dali is up and running OK. Open the file src/META-INF/orm.xml. With the JPA perspective, you should have a JPA Structure view (sharing a pane with the Outline view) that corresponding to the file's content.

Our problems occur when moving the cursor within the orm.xml file and right-clicking in the JPA Structure View.

Comment 12 Oleg Besedin 2012-05-11 11:18:39 EDT

(In reply to comment #11)
> The attached is a simple JPA project. 

Thanks Brian! I can reproduce the problem now.

This  turns out to be an interesting issue.

The bottom line is that SelectionService only propagates 3.x selection from
active parts into context, see SelectonService#setPart(). In there we track an
active part and add/remove listeners to propagate its selection.

In this case, however, the JPA Structure View changes selection while being
inactive. It adjusts its tree selection to the editor and that causes 3.x
selection event to fire but nobody listens to that event on the 4.x side (the
Structure View is inactive).

When the user L-clicks on a tree node in the Structure View, SWT selection
event gets generated and that updates outgoing selection in the context.

However, when user R-clicks on already selected tree node, the SWT selection
event is not generated, and system uses whatever outgoing selection value it
had in the context from the last time. 

(I talked with Bogdan and Silenio and, that is the expected SWT behavior. In a
sense that nobody asked to have it fixed for R-click like it was done for the
L-click.)

Comment 13 Oleg Besedin 2012-05-11 11:19:28 EDT

Created attachment 215486 [details]
Possible patch for review

Comment 14 Brian Vosburgh 2012-05-11 11:33:15 EDT

(In reply to comment #13)
> Created attachment 215486 [details]
> Possible patch for review

Thanks for working on this, Oleg. We are still waiting for WTP (which Dali is a parto of) to move to Git; so I am not set up for applying a Git patch; and so I cannot try out your changes and will have to trust your and your comrades' testing. :-)

Comment 15 Oleg Besedin 2012-05-11 11:55:30 EDT

Also tested multiple windows, seems to be OK with the patch.

Comment 16 Remy Suen 2012-05-11 13:14:42 EDT

(In reply to comment #13)
> Created attachment 215486 [details]
> Possible patch for review

I went over the patch with Oleg and it makes sense to me. +1 for RC1. This change will help keep the 4.x selection in sync with the actual 3.x selection.

Comment 17 Oleg Besedin 2012-05-11 13:42:28 EDT

Thank you!

Change release into Git master:

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

Brian, thank you for providing instructions to reproduce and examples, it made it much easier to figure out what was going on.

Comment 18 Oleg Besedin 2012-05-25 16:11:12 EDT

Verified using I20120518-2145.