Redesign reinstantiation handlers

joeseibel commented 5 years ago

The current state of instantiation and reinstantation handlers is confusing and it seems that it was created in an ad-hoc manner. We should think about what would make sense.

There are three handlers for instantiation: InstantiateHandler, ReinstantiateAadl, and ReinstantiateHandler. We should use more meaningful names so that it can be obvious what the difference between ReinstantiateAadl and ReinstantiateHandler is. Some of the functionality is confusing as well.

InstantiateHandler: This is in the outline view's context menu with the label "Instantiate" and is invoked on one ComponentImplementation.
ReinstantiateAadl: This is in the main menu with the label "Reinstantiate All Instance Models". The behavior is dependent on the selection. If the selection contains instance IFiles, then only those instance files are reinstantiated. This is confusing because the behavior does not match the label and it will not reinstantiate all instance models in the workspace. If the selection does not contain any instance files, then all instances models in the workspace will be reinstantiate. This handler needs to change because the behavior is confusing and not obvious to the user. "Reinstantiate All" should not care about the selection.
ReinstantiateHandler: This is in the navigator's context menu with the label "Reinstantiate" and is invoked on one IFile which is an instance file. This could be modified to allow multiple instance files to be selected.

One functionality to think about is reinstantiating all instances in a selected IContainer or working set. There could be a dialog showing which files were reinstantiated or indicating that there were no instance files contained in the selection.

We are also not consistent with how progress is handled. InstantiateHandler is run in a ProgressMonitorDialog. ReinstantiateAadl is run in a WorkspaceJob. ReinstantiateHandler is executed directly without any progress reporting.

lwrage commented 4 years ago

Note: Should support reinstantiation from navigator if a directory/project/working set is selected. In this case find all instance models in directory/project/working set and reinstantiate all of them.

AaronGreenhouse commented 4 years ago

What we have now in the UI:

Selecting a single ComponentImplementaton in the "Outline" view shows "Instantiate" in the context menu.
- There is no option to instantiate multiple items
Selecting a single .aaxl file in the "AADL Navigator" view shows "Instantiate" in the context menu.
- There is no option to instantiate multiple items
There is no menu item in the "OSATE" menu bar to instantiate a component.
Selecting a single "ComponentInstace" in the "AADL Navigator" view sows "Reinstantiate" in the context menu.
- There is no option to instantiate multiple items
We have "Reinstantiate All Instance Models" in the "OSATE" menu bar at all times.

AaronGreenhouse commented 4 years ago

What we probably want:

Selecting 1 or more ComponentImplementation objects in the "Outline" view enables "Instantiate Component" in the context menu and "Instantiate Component" in the "OSATE" menu bar. Each selected component is separately instantiated.
Selecting 1 or more ComponentImplementation objects in the "AADL Navigator" view enables "Instantiate Component" in the context menu and "Instantiate Component" in the "OSATE" menu bar. Each selected component is separately instantiated.
Selecting exactly 1 SystemInstance object in the "AADL Navigator", aaxl editor, or "Outline" view, or should
- Enable "Reinstantiate Instance Model" in the "OSATE" menu bar
- Enable "Reinstantiate Instance Model" in the appropriate context menu
Selecting exactly 1 .aaxl file in the "AADL Navigator" should
- Enable "Reinstantiate Instance Model" in the "OSATE" menu bar
- Enable "Reinstantiate Instance Model" in the appropriate context menu
Selecting 2 or more .aaxl files or SystemInstance objects in the "AADL Navigator" should
- Enable "Reinstantiate Instance Model" in the "OSATE" menu bar
- Enable "Reinstantiate Instance Model" in the context menu
Selecting 1 or more non-nested projects, directories, or working sets in the "AADL Navigator" should
- Enable "Reinstantiate All Instance Models" in the context menu
- Enable "Reinstantiate All Instance Models" in the "OSATE" menu bar.
When executed, this searches for all instance models in the selected objects and reinstantiates each one.
When nothing is selected none of the instantiate/reinstantiate commands should be enabled in the "OSATE" menu bar.

AaronGreenhouse commented 4 years ago

I think the correct way to run all these items is as a WorkspaceJob. This is the way to execute items in the Eclipse workspace and prevent them concurrently making changes.

lwrage commented 4 years ago

Also, all this needs to be cancellable because instantiation may take a long time.

lwrage commented 4 years ago

How will we reinstantiate all instance model in the workspace?

AaronGreenhouse commented 4 years ago

Okay, yes, I forgot about just reinstantiating everything in the workspace. How about

Selecting 1 or more non-nested projects, directories, or working sets in the "AADL Navigator" should
- Enable "Reinstantiate All Instance Models in Selection" in the context menu
- Enable "Reinstantiate All Instance Models in Selection" in the "OSATE" menu bar.
When executed, this searches for all instance models in the selected objects and reinstantiates each one.
There should always be a "Reinstantiate All Instance Models in Workspace" command in the "AADL Navigator" context menu and the "OSATE" menu bar.

If you don't like the really long menu item names we could have two sub menus:

Reinstantitae All Instance Models > In Selection and
Reinstantitae All Instance Models > In Workspace

lwrage commented 4 years ago

Never mind, we can always select everything (Ctrl-A) in the navigator and reinstantiate. Maybe it would it be clearer to just have on Reinstantiate instead of Reinstantiate and Reinstantiate all.

AaronGreenhouse commented 4 years ago

We can work on this. Right now I'm trying to fix up initial instantiation.

AaronGreenhouse commented 4 years ago

Created new InstantiateComponentHandler. It's used by "Instantiate Component" in

the outline view
the AADL Navigator
the "OSATE" drop down menu

It instantiates 1 or more component implementations. Error messages are bundled up and reported at the end, although I'm not sure how to test this. Should support cancellation, but my test models aren't complicated enough for the progress monitor to show and be cancelled.

lwrage commented 4 years ago

You can make instantiation take long by adding an array of modal components such that you end up with a huge number of SOMs. Don't forget to increase the SOM limit preference setting.

AaronGreenhouse commented 4 years ago

May be too much work to allow selection of SystemInstance objects. Things are not really set up to handle and test for that, and there isn't much of an advantage to it considering that an aaxl2 file contains exactly one system instance.

Going to add a "Reinstantiate Instance" command to the context menu of the AADL Navigator and to the OSATE menu. Will be active when 1 or more system instance files are selected.

AaronGreenhouse commented 4 years ago

Did the above. Based on the same general code as InstantiateComponent. May be able to create a common superclass after all this is working.

Next step is to deal with "Reinstantiate All"

AaronGreenhouse commented 4 years ago

(Don't forget to update the user documentation when this is done.)

lwrage commented 4 years ago

I would prefer to have just a single "Reinstantiate" that handles all instance models in the selection.

AaronGreenhouse commented 4 years ago

No problem.

AaronGreenhouse commented 4 years ago

Things go badly when trying to reinstantiate an instance model when the root component instance no longer exists. There is a NullPointerException. This is captured and reported to the user, but the .aaxl2 file still exists and is empty which is a problem.

Need to update the handler jobs to remove any model files where an exception has been thrown like I do for handling an interruption.

AaronGreenhouse commented 4 years ago

That works, and is a good, but there should be a special check that the root component is missing so that we can give a better error message in that case "Model removed because root component no long exists."

AaronGreenhouse commented 4 years ago

Fixed the above.

AaronGreenhouse commented 4 years ago

Oops. Lutz wants a single "Reinstantiate" command that operates on a selection of projects, folders, and files. No separate "Reinstantiate All" command. This is no problem. Easier in the long run.

AaronGreenhouse commented 4 years ago

Deleting problematic instance model files (see above) may not be the best approach. Ideally we should try to preserve the original state and let the user decide what to do with them. Filed a Issue #2183 to deal with this.

For now things are slightly better than before because now we at least tell the user something bad happened and that we are removing the file.

AaronGreenhouse commented 4 years ago

Changed the names of the new commands to just "Reinstantiate" and "Instantiate".

Currently fixing up the results dialog box.

AaronGreenhouse commented 4 years ago

I was having trouble getting the progress bar to show the different models being created. Then I realized that I was doing the whole thing wrong. Each model instantiation should be in its own job. This also allows parallelism in the model creation (at least if the models are in different projects). Gives much better feedback this way. Specific model instantiations can be cancelled.

Had to rework the command and how it creates jobs obviously. There is one "system" job (Job.setSystem(true)) that is created to gather the results and display the dialog at the end.

I have this working for the "Instantiate" command. Need to copy it over to the "Reinstantiate" command.

AaronGreenhouse commented 4 years ago

Updated the "Reinstantiate" command, removed the old handlers, and cleaned things up a little more.

AaronGreenhouse commented 4 years ago

Oops. I haven't updated the user guide yet!

AaronGreenhouse commented 4 years ago

On the advice of Lutz I tried instantiating models from declarative models that are inside folders in a project. This failed!

java.lang.IllegalArgumentException: Attempted to beginRule: F/i1553a/a_folder, does not match outer scope rule: F/i1553a/a_folder/instances
    at org.eclipse.core.runtime.Assert.isLegal(Assert.java:66)
    at org.eclipse.core.internal.jobs.ThreadJob.illegalPush(ThreadJob.java:137)
    at org.eclipse.core.internal.jobs.ThreadJob.push(ThreadJob.java:392)
    at org.eclipse.core.internal.jobs.ImplicitJobs.begin(ImplicitJobs.java:88)
    at org.eclipse.core.internal.jobs.JobManager.beginRule(JobManager.java:298)
    at org.eclipse.core.internal.resources.WorkManager.checkIn(WorkManager.java:124)
    at org.eclipse.core.internal.resources.Workspace.prepareOperation(Workspace.java:2242)
    at org.eclipse.core.internal.resources.Folder.create(Folder.java:90)
    at org.eclipse.core.internal.resources.Folder.create(Folder.java:121)
    at org.eclipse.emf.ecore.resource.impl.PlatformResourceURIHandlerImpl$PlatformResourceOutputStream.createContainer(PlatformResourceURIHandlerImpl.java:76)
    at org.eclipse.emf.ecore.resource.impl.PlatformResourceURIHandlerImpl$PlatformResourceOutputStream.flush(PlatformResourceURIHandlerImpl.java:107)
    at org.eclipse.emf.ecore.resource.impl.PlatformResourceURIHandlerImpl$PlatformResourceOutputStream.close(PlatformResourceURIHandlerImpl.java:89)
    at org.eclipse.emf.ecore.resource.impl.PlatformResourceURIHandlerImpl$WorkbenchHelper$1.close(PlatformResourceURIHandlerImpl.java:175)
    at org.eclipse.emf.ecore.resource.impl.ResourceImpl.save(ResourceImpl.java:1048)
    at org.osate.aadl2.instantiation.InstantiateModel.buildInstanceModelFile(InstantiateModel.java:231)
    at org.osate.ui.handlers.InstantiateComponentHandler$InstantiationJob.runInWorkspace(InstantiateComponentHandler.java:173)
    at org.eclipse.core.internal.resources.InternalWorkspaceJob.run(InternalWorkspaceJob.java:42)
    at org.eclipse.core.internal.jobs.Worker.run(Worker.java:63)

AaronGreenhouse commented 4 years ago

Ah, so this also fails if you instantiate a model from a declarative model in the root of the project as well. The problem, I think, is that I forgot to grant the job permission to create the instances directory.

AaronGreenhouse commented 4 years ago

Fixed this. Had to redo the way I launch jobs because I wanted to make sure the output directories exist before the actual instantiation jobs are launched. Otherwise the scheduling rules necessary for creating the output directory as part of an instantiation would limit the about of concurrency in job execution.

AaronGreenhouse commented 4 years ago

Updated user guide.

osate / osate2

Redesign reinstantiation handlers #1553