Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Redesign reinstantiation handlers #1553

Closed
joeseibel opened this issue Sep 26, 2018 · 29 comments · Fixed by #2189
Closed

Redesign reinstantiation handlers #1553

joeseibel opened this issue Sep 26, 2018 · 29 comments · Fixed by #2189
Assignees
Milestone

Comments

@joeseibel
Copy link
Contributor

@joeseibel joeseibel commented Sep 26, 2018

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
Copy link
Contributor

@lwrage lwrage commented Jan 27, 2020

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
Copy link
Contributor

@AaronGreenhouse AaronGreenhouse commented Jan 27, 2020

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
Copy link
Contributor

@AaronGreenhouse AaronGreenhouse commented Jan 27, 2020

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
Copy link
Contributor

@AaronGreenhouse AaronGreenhouse commented Jan 28, 2020

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
Copy link
Contributor

@lwrage lwrage commented Jan 28, 2020

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

@lwrage
Copy link
Contributor

@lwrage lwrage commented Jan 28, 2020

How will we reinstantiate all instance model in the workspace?

@AaronGreenhouse
Copy link
Contributor

@AaronGreenhouse AaronGreenhouse commented Jan 28, 2020

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
Copy link
Contributor

@lwrage lwrage commented Jan 28, 2020

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
Copy link
Contributor

@AaronGreenhouse AaronGreenhouse commented Jan 28, 2020

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

@AaronGreenhouse
Copy link
Contributor

@AaronGreenhouse AaronGreenhouse commented Jan 30, 2020

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
Copy link
Contributor

@lwrage lwrage commented Jan 30, 2020

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
Copy link
Contributor

@AaronGreenhouse AaronGreenhouse commented Jan 30, 2020

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
Copy link
Contributor

@AaronGreenhouse AaronGreenhouse commented Jan 30, 2020

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
Copy link
Contributor

@AaronGreenhouse AaronGreenhouse commented Jan 30, 2020

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

@lwrage
Copy link
Contributor

@lwrage lwrage commented Jan 31, 2020

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

@AaronGreenhouse
Copy link
Contributor

@AaronGreenhouse AaronGreenhouse commented Jan 31, 2020

No problem.

@AaronGreenhouse
Copy link
Contributor

@AaronGreenhouse AaronGreenhouse commented Jan 31, 2020

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
Copy link
Contributor

@AaronGreenhouse AaronGreenhouse commented Jan 31, 2020

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
Copy link
Contributor

@AaronGreenhouse AaronGreenhouse commented Jan 31, 2020

Fixed the above.

@AaronGreenhouse
Copy link
Contributor

@AaronGreenhouse AaronGreenhouse commented Jan 31, 2020

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
Copy link
Contributor

@AaronGreenhouse AaronGreenhouse commented Jan 31, 2020

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.

@lwrage lwrage removed this from the 2.7.0 milestone Jan 31, 2020
@lwrage lwrage added this to the 2.7.1 milestone Jan 31, 2020
@AaronGreenhouse
Copy link
Contributor

@AaronGreenhouse AaronGreenhouse commented Feb 4, 2020

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

Currently fixing up the results dialog box.

@AaronGreenhouse
Copy link
Contributor

@AaronGreenhouse AaronGreenhouse commented Feb 4, 2020

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
Copy link
Contributor

@AaronGreenhouse AaronGreenhouse commented Feb 5, 2020

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

@AaronGreenhouse
Copy link
Contributor

@AaronGreenhouse AaronGreenhouse commented Feb 6, 2020

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

@AaronGreenhouse
Copy link
Contributor

@AaronGreenhouse AaronGreenhouse commented Feb 10, 2020

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
Copy link
Contributor

@AaronGreenhouse AaronGreenhouse commented Feb 10, 2020

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
Copy link
Contributor

@AaronGreenhouse AaronGreenhouse commented Feb 10, 2020

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
Copy link
Contributor

@AaronGreenhouse AaronGreenhouse commented Feb 11, 2020

Updated user guide.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants