Bug 447057

Comment 1 Nathalie Lepine 2014-10-14 06:50:43 EDT

In odesign file, I have 2 representations, a diagram and a tree.
- close all representations and open tree representation (opened in Eclipse Editor View) and change a value in properties view -> OK

- close all representations and open tree an diagram representations (also opened in Eclipse Editor View) and change a value on tree element properties view : OK

- close all representations and open tree diagram. Then move it near model explorer view. Change a value in properties view -> OK 

- close all representations and open tree diagram. Then move it near model explorer view. Open diagram representation (opened in Eclipse Editor View).
Change a value in tree representatipn properties view -> KO

Because of org.eclipse.sirius.ui.tools.api.properties.AbstractEObjectPropertySource.setPropertyValue method
 public void setPropertyValue(final Object id, final Object value) {
        final Identifier identifier = (Identifier) id;

        final IEditorPart part = EclipseUIUtil.getActiveEditor();
        if (part instanceof AbstractDTreeEditor) { 
 ...
 }

public static IEditorPart getActiveEditor() {
        final IWorkbenchPage page = EclipseUIUtil.getActivePage();
        if (page != null) {
            return page.getActiveEditor();
        }
        return null;
    }

Part returned by EclipseUIUtil.getActiveEditor() is the diagram and not the tree.

I am on Kepler, I do not test on E3, it may be an E4 issue.

Comment 2 Pierre-Charles David 2014-10-16 05:19:30 EDT

It seems to work for diagrams, but not for trees and tables.

Comment 3 Pierre-Charles David 2014-10-16 05:35:53 EDT

Looking at the code, there are actually several issues in this code:

* the bug you mention: we rely on EclipseUIUtil.getActiveEditor(), which does not handle the new possibilities offered by e4.
* AbstractEObjectPropertySource, used by table and tree editors, performs a canEditInstance check. For diagrams, we use org.eclipse.sirius.diagram.ui.tools.internal.properties.CompositeEObjectPropertySource, which does not perform this check.
* the CompositeEObjectPropertySource used for diagram however has additional logic which is not present for tables & trees, to launch a refresh in the active editor. Sub-bug 1: it does not seem like table and trees have equivalent code. Sub-bug 2: this specific logic also relies on the broken getActiveEditor(). Sub-bug 3: I'm not even sure this logic is really needed.

More analysis is needed to fully understand the issue, and refactor the code to use a single, robust implementation for all dialects.

Comment 4 Belqassim Djafer 2014-12-05 03:37:09 EST

currently working on it

Comment 5 Belqassim Djafer 2014-12-05 04:29:10 EST

The issue is more complicated than it seemed to me.
Work interrupted

Comment 6 Eclipse Genie 2015-03-09 10:28:57 EDT

New Gerrit change created: https://git.eclipse.org/r/43416

Comment 7 Belqassim Djafer 2015-03-16 09:05:18 EDT

Created attachment 251586 [details]
Use case

To reproduce :
1- Import the attached project in the workspace.
2- Open "new Tree" and "new Diagram" representations.
3- Move "new Tree" representation near to the model explorer view.
4- Change a value on tree element properties view -> KO !

Comment 8 Eclipse Genie 2015-03-18 06:04:39 EDT

Gerrit change https://git.eclipse.org/r/43416 was merged to [master].
Commit: http://git.eclipse.org/c/sirius/org.eclipse.sirius.git/commit/?id=798d97d52eca4f06c216b442028f797ed0a6c5b6

Comment 9 Pierre-Charles David 2015-03-18 06:39:27 EDT

Marking as fixed as the original scenario is now OK. I'm taking a note to open separate ticket(s) later for the other problems identified in comment 3 which were not addressed.

Comment 10 Belqassim Djafer 2015-04-10 10:59:12 EDT

Verified on Sirius 3.0.0M6

Comment 11 Pierre-Charles David 2015-06-24 11:16:22 EDT

Available in Sirius 3.0.0. See https://wiki.eclipse.org/Sirius/3.0.0.