search

osate / osate2

Open Source AADL2 Tool Environment
http://osate.org
Eclipse Public License 2.0
39 stars 8 forks source link

Move outline tree computation to background thread #2320

Closed lwrage closed 4 years ago

lwrage commented 4 years ago

Summary

Outline tree computation is currently done in a foreground thread, which can block the UI. Xtext has support for updating the outline in the background.

See https://www.eclipse.org/forums/index.php/t/1103639/

related to #1832

Environment

lwrage commented 4 years ago

There seems to be no official documentation. Get help on the TMF forum if needed.

AaronGreenhouse commented 4 years ago

So actually, are talking about the AADL textual outline or the AAXL instance model outline or both?

AaronGreenhouse commented 4 years ago

Okay comments on DefaultOutlineTreeProvider say this:

/**
 * The default implementation of an {@link IOutlineTreeProvider}.
 * If you would like to run your outline in Background, please have a look at {@link BackgroundOutlineTreeProvider}.
 * Please note that this would have implications to you LabelProvider implementation. Please read the JavaDoc in the mentioned class carefully.
 * 
 * @author Jan Koehnlein - Initial contribution and API
 */

and BackgroundOutlineTreeProvider says this:

 * It is essential that the {@link ILabelProvider} implements {@link ILabelProviderImageDescriptorExtension} and that
 * the method {@link ILabelProviderImageDescriptorExtension#getImageDescriptor(Object)} does not need to be executed in
 * the {@link Display} thread, e.g. does not create {@link Image} objects internally.

This matches up with the comments in https://www.eclipse.org/forums/index.php/t/1103639/ that stress the importance of using ImageDescriptor.

lwrage commented 4 years ago

Outline for the AADL text editor.

On Mon, Sep 21, 2020 at 4:33 PM AaronGreenhouse notifications@github.com wrote:

So actually, are talking about the AADL textual outline or the AAXL instance model outline or both?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/osate/osate2/issues/2320#issuecomment-696358638, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAFXFZ5NSGLBDJEKJD6AVOTSG62CNANCNFSM4M4FLUIA .

AaronGreenhouse commented 4 years ago

So need to change Aadl2OutlineTreeProvider to extend BackgroundOutlineTreeProvider, but first I need to fix Aadl2LabelProvider to also extend ILabelProviderImageDescriptorExtension.

AaronGreenhouse commented 4 years ago

Ugh. SO despite what the comments on DefaultOutlineTreeProvider suggest, you cannot just swap BackgroundOutlineTreeProvider for DefaultOutlineTreeProvider (modulo the image descriptor issue) because of this nonsense:


    protected PolymorphicDispatcher<Void> createChildrenDispatcher = PolymorphicDispatcher.createForSingleTarget(
            "_createChildren", 2, 2, this);

    protected PolymorphicDispatcher<Void> createNodeDispatcher = PolymorphicDispatcher.createForSingleTarget(
            "_createNode", 2, 2, this);

    protected PolymorphicDispatcher<Object> textDispatcher = PolymorphicDispatcher.createForSingleTarget("_text", 1, 1,
            this);

    protected PolymorphicDispatcher<Image> imageDispatcher = PolymorphicDispatcher.createForSingleTarget("_image", 1,
            1, this);

    protected PolymorphicDispatcher<Boolean> isLeafDispatcher = PolymorphicDispatcher.createForSingleTarget("_isLeaf",
            1, 1, this);

And the background tree wants to inject an OutlineNodeFactory

AaronGreenhouse commented 4 years ago

Good news: Aadl2LabelProvider already extends ILabelProviderImageDescriptorExtension!

AaronGreenhouse commented 4 years ago

I think I have this working. Certainly it doesn't seem to be breaking anything, but I'm having a hard time telling if it makes a difference.

Some strange stuff I noticed though: The original Aadl2OutlineTreeProvider has 

    protected boolean _isLeaf(IntegerLiteral bpa) {

        if (bpa.eContainer() instanceof RecordValue) {
            return false;
        }
        return false;
    }

In particular this method always returns false. I am wondering if this is an error. Should the return value of the conditional really be true? There are two other methods that do:

    protected boolean _isLeaf(BasicPropertyAssociation bpa) {

        if (bpa.eContainer() instanceof RecordValue) {
            return true;
        }
        return false;
    }

    protected boolean _isLeaf(ReferenceValue bpa) {

        if (bpa.eContainer() instanceof RecordValue) {
            return true;
        }
        return false;
    }
AaronGreenhouse commented 4 years ago

The workspace has the following additional extensions of DefaultOutlineTreeProvider

I see one annex: ErrorModelOutlineTreeProvider

Some are clearly from the mini languages.

Some I have no idea: xbase, mode aware

The following are trivial extensions and can be just as well trivial extensions of BackgroundOutlineTreeProvider:

ReqSpecOutlineTreeProvider doesn't have a lot going on and should be easy to update.

ErrorModelOutlineTreeProvider shouldn't be too challenging to update either.

AaronGreenhouse commented 4 years ago

Just make that ErrorModelOutlineTreeProvider can run in the background, and also make sure that the AADL outline checks if the annex outline provider is backgroundable.

AaronGreenhouse commented 4 years ago

Updated the ErrorModelOutlineTreeProvider.

Replaced the old Aadl2OutlineTreeProvider with the contents of Aadl2BackgroundOutlineTreeProvider. This better preserves the history of the situation.