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

Users need a way to override contributed property sets #1134

Closed
lwrage opened this issue Apr 16, 2018 · 43 comments · Fixed by #1935
Closed

Users need a way to override contributed property sets #1134

lwrage opened this issue Apr 16, 2018 · 43 comments · Fixed by #1935

Comments

@lwrage
Copy link
Contributor

lwrage commented Apr 16, 2018

Contributed property sets (and packages) used to be copied into the workspace, which we considered bad practice. In #527 we changed that, such that contributed AADL files are read from the contributing plugins. However, now it is impossible to fix errors in such a file without updating OSATE. There should be some way to update just a single contributed file.

We have a specialized mechanism that works for AADL_Project.aadl only.

Who updates the file contents? End user or update mechanism?
What's the scope of the change? Workspace or installation?

@Etienne13
Copy link
Contributor

As mentioned in #1669, solving this issue would help to experiment AADL tools that need to go beyond some limitations in the current version of some property sets (including modeling ARINC653 systems on multicores).

@Etienne13
Copy link
Contributor

I now face this issue again because of the "pok_properties.aadl" file, which is contributed by the osate-ocarina plugin and my own plugin (RAMSES) with two different contents. My current workaround is to uninstall osate-ocarina but is is not satisfactory for my end-users: each time they install RAMSES for a new version of OSATE, they have to uninstall osate-ocarina.

Here is a proposal for this issue: define an order of contributed property sets such that property set N+1 overrides property set N;

  • the default order is computed according to the history of installed plugins: the latest is pushed back in the queue and thus overrides the previous ones
  • the order can be viewed and changed by end-users

This would be a solution at the installation level which could be changed at the workspace level (or at the project level if needed).

It would not allow to mix (merge/override) property set contents but at least property set files...

@jjhugues
Copy link
Contributor

jjhugues commented Jul 5, 2019 via email

@Etienne13
Copy link
Contributor

Hi Jérome,

yes, that would at least solve my problem for now.

Thanks!
Etienne.

@lwrage
Copy link
Contributor Author

lwrage commented Jul 9, 2019

Add workspace scoped preference to configure which contributed AADL is used. Details TBD.

@AaronGreenhouse
Copy link
Contributor

Need more info here.

Processing of contributed AADL files seems to be handled in several places, making this potentially tricky, including:

  • org.osate.xtext.aadl2.properties.ui.builder.PropertiesToBeBuiltComputerContribution --- I think this inserts the files into the build process?
  • org.osate.xtext.aadl2.ui.containers.Aadl2ProjectsStateHelper --- Not really sure what this does
  • org.osate.xtext.aadl2.ui.resource.Aadl2Storage2UriMapper --- Not really sure what this does
  • org.osate.ui.navigator.AadlContributionContentProvider --- Adds the contributed files to the AADL navigator tree

Current uses all rely on the method PluginSupportUtil.getContributedAadl(). This method reads all the contributions from the plugin.xml files. If there are duplicates here, then they will simply be propogated. If we want to override a contributed file with a file from the OSATE workspace, then it will be missed here. We would have to replace uses of this method with a new method that returns a manipulated list of contributed files. (This new method would not be applicable to replacing getCongtributedAadlAsClasspath() that is used by stand-alone (non-Eclipse) applications. They would have to there own mechanism for this.)

@AaronGreenhouse
Copy link
Contributor

AaronGreenhouse commented Jul 18, 2019

What is the plan here? I think what is wanted is a way to override a contributed resource with a resource from the workspace, in effect, treating that workspace file as a contributed resource.

I don't think we need to deal with the case of multiple contributed resources with the same name.

So we want an Eclipse workspace preference (and associated UI pane) that allows a file from the workspace to be inserted into the list of contributed resources and possibly replace an existing resource.

Probably better (simpler to think about) is this:

  • The preference allows contributed resources to be selected to be ignored (not reported)
  • The preference allows files from the workspace to be added to the list of contributed resources.
  • The preference panel should not allow the overall list of "contributed resources" - "ignore resources" + "workspace contributions" to contain packages/property sets with duplicate names.

@AaronGreenhouse
Copy link
Contributor

Looked at the special case of AADL_Project.aadl, which can be replaced by an identically named file in the workspace. This works by having the path of the AADL_Project file that should be used saved in a workspace preference. Aadl2ProjectsStateHelper uses the value of this preference instead of the path of any AADL_Project file that is in the contributed resources.

@AaronGreenhouse
Copy link
Contributor

The problem with the current approach (see above) is that the UI doesn't reflect what is happening at all.

I think that ideally in the AADL Navigator, we should have "Plug-in Contributions" and "Workspace Contributions". The first should show anything contributed by a plug-in minus any item that is removed via the workspace preferences. The second show any contributions from the workspace (as set in the preferences).

@AaronGreenhouse
Copy link
Contributor

Steps

  1. Review the workspace preferences mechanism again.
  2. Figure out how to save the preference information that I need (List of suppressed contributions, list of workspace contributions)
  3. Create UI preference pane of the preferences
  4. Update PluginSupportUtil.getContributedAadl() and friends

@AaronGreenhouse
Copy link
Contributor

@AaronGreenhouse
Copy link
Contributor

AaronGreenhouse commented Jul 24, 2019

To avoid having to parse lists, etc., I think the best way to manage the preferences for the plug-in contributed resources is use the URI of the contributed resource as part of the preference name, and to treat them all as a set of booleans. Name should be something like "contributed.resource.isVisible.URI"

Going to alter my steps a bit. Going to work on just suppressing contributed resources first:

  1. Create preference pane for suppressing resources
  2. Update PluginSupportUtil.getContributedAadl() and friends
  3. Verify that the AADL Navigator (which uses getContributedAadl() is updated. May need to generate refresh events of some sort?
  4. Verify that the builder process ignores the suppressed resources. Can test this by having packages that make reference to contributed resources and seeing what happens when I make them go away.

@AaronGreenhouse
Copy link
Contributor

This is useful to know: https://www.codeaffine.com/2016/03/01/swt-scrolledcomposite/

@AaronGreenhouse
Copy link
Contributor

Note: preferences are not available in the stand-alone (headless) environment. Will need to provide a clean way to handle this in headless mode, but after I get it working in Eclipse.

@AaronGreenhouse
Copy link
Contributor

AaronGreenhouse commented Jul 26, 2019

Come back to updating Aadl2Storage2UriMapper, ContributedAadlEditorInputFactory

@AaronGreenhouse
Copy link
Contributor

Added preference pane "Contributed Resources". Can select with contributed resources are visible. Added method PredeclaredProperties.getVisibleContributedResources() that filters out the invisible resources. Updated the callsites of PluginResourceUtil.getContributedAadl() to use this new method as appropriate.

The preference pane triggers a workspace build when in performOk(). This seems to make everything work okay, but

  1. Issue Error and warning icon decorations in AADL Navigator are not updated #1824 has not been merged yet which makes things confusing, and
  2. I need to be more selective about when to perform the build, such as when the user didn't actually change anything.

@AaronGreenhouse
Copy link
Contributor

Updated the preference pane to allow workspace contributions.

Still need to

  1. Save these in the preferences
  2. Incorporate these into the visible contributed resources
  3. Have the preference pane check for duplicates, both duplicate URIs and packages with the same name.

@AaronGreenhouse
Copy link
Contributor

Did (1), above.

Should do (3) next, and then (2)

@AaronGreenhouse
Copy link
Contributor

Regarding (3), above:

  • It is not possible to have duplicate URIs in the plug-in contributions, or to have workspace contributions whose URI duplicates a plug-in contribution URI. The URIs effectively exist in different name spaces.
  • It is not possible to add duplicates in an "add" operation because the ResourceSelectionDialog doesn't allow it.
  • Therefore we only have to check that none of the URIs being added duplicate those that are already in the workspace contribution list.

The check for duplicate names must occur across all the contributed resources.

  • It is possible for plug-in contributions to have the same name.
  • It is possible to add identically named contributions in a single "add" operation
  • It is possible to add a workspace contribution with the same name as a plug-in contribution.

Notably, this check needs to be done when the pane is opened to make sure that the plug-in contributions don't have duplicates.

@AaronGreenhouse
Copy link
Contributor

Look into what the Restore Defaults button is really supposed to do.

@AaronGreenhouse
Copy link
Contributor

Silly me, it's not necessary to test for duplicate URI explicitly because duplicate URI will have duplicate package name.

@AaronGreenhouse
Copy link
Contributor

The preference pane is done and seems to work. The UI is bit clunky and ugly, but it's not worth making too pretty.

Need to incorporate the workspace contributions into the contributions returned by PredeclaredProperties.getVisibleContributedResources().

I expect there is going to be some craziness in project A if that project contributes a resource R. Just have to try it and see what happens.

In the AADL Navigator I would ideally like to have a Workspace Contributions item just like we have a Plug-in Contributions item. It's that too time-consuming to add just put them under Plug-in Contributions.

@AaronGreenhouse
Copy link
Contributor

Things are not working correctly. I updated the classes Aadl2ProjectsStateHelper and PropertiesToBeBuiltComputerContribution. The workspace rebuilds after the preferences are updated, and they use the new values for the contributions, but references are not always resolved correctly, and usually get weird exceptions:

ENTRY org.apache.log4j 4 0 2019-08-05 16:37:38.501
!MESSAGE org.eclipse.xtext.builder.clustering.CopiedResourceDescription  - java.lang.IllegalStateException: getReferenceDescriptions platform:/resource/00foobar/Other.aadl

!STACK 0
java.lang.IllegalStateException: getReferenceDescriptions platform:/resource/00foobar/Other.aadl
	at org.eclipse.xtext.builder.clustering.CopiedResourceDescription.getReferenceDescriptions(CopiedResourceDescription.java:84)
	at org.eclipse.xtext.resource.DescriptionUtils.collectOutgoingReferences(DescriptionUtils.java:29)
	at org.eclipse.xtext.ui.editor.DirtyStateEditorSupport.isReparseRequired(DirtyStateEditorSupport.java:658)
	at org.eclipse.xtext.ui.editor.DirtyStateEditorSupport$UpdateEditorStateJob$1.exec(DirtyStateEditorSupport.java:161)
	at org.eclipse.xtext.ui.editor.DirtyStateEditorSupport$UpdateEditorStateJob$1.exec(DirtyStateEditorSupport.java:1)
	at org.eclipse.xtext.resource.OutdatedStateManager.exec(OutdatedStateManager.java:91)
	at org.eclipse.xtext.ui.editor.model.XtextDocument$XtextDocumentLocker.internalReadOnly(XtextDocument.java:527)
	at org.eclipse.xtext.ui.editor.model.XtextDocument$XtextDocumentLocker.readOnly(XtextDocument.java:499)
	at org.eclipse.xtext.ui.editor.model.XtextDocument.readOnly(XtextDocument.java:138)
	at org.eclipse.xtext.ui.editor.DirtyStateEditorSupport$UpdateEditorStateJob.run(DirtyStateEditorSupport.java:146)
	at org.eclipse.core.internal.jobs.Worker.run(Worker.java:63)

Here platform:/resource/00foobar/Other.aadl is file from the workspace that has been set to be contributed in the preferences.

@AaronGreenhouse
Copy link
Contributor

I need to make sure that files are not processed twice, once because they are contributed and then again in their home project. This is in the old Aadl2ProjectStateHandler

	public String initHandle(URI uri) {
		if (uri.lastSegment().contentEquals(PredeclaredProperties.AADL_PROJECT)) {
			if (uri.toString().contentEquals(PredeclaredProperties.getAADLProjectPreference())) {
				return AADL_PROJECT_HANDLE;
			} else {
				return null;
			}
		} else if (CONTRIBUTED_AADL.contains(uri)) {
			return CONTRIBUTED_HANDLE;
		} else {
			return super.initHandle(uri);
		}
	}

I did look at this, but somehow kept missing the significance of return null.

I need to update so we can check a file name against all the contributed names.

@AaronGreenhouse
Copy link
Contributor

Updated the AADL Navigator to have separate Plug-in Contributions and Workspace Contributions headings.

@AaronGreenhouse
Copy link
Contributor

Updated the OSATE user guide.

@lwrage lwrage modified the milestones: 2.5.2, 2.6.0 Aug 19, 2019
@lwrage lwrage modified the milestones: 2.6.0, 2.6.1 Oct 25, 2019
@lwrage lwrage modified the milestones: 2.6.1, 2.7.0 Dec 5, 2019
@lwrage lwrage modified the milestones: 2.7.0, 2.7.1 Jan 18, 2020
@lwrage lwrage modified the milestones: 2.7.1, 2.8.0 Mar 6, 2020
@lwrage lwrage modified the milestones: 2.8.0, 2.8.1 Jun 25, 2020
@AaronGreenhouse
Copy link
Contributor

Current approach is too complicated. Should just present a list of all the currently contributed property sets and allow the user to choose which one to override. Overriding set must have the same name.

No "workspace contributions" in the AADL navigator. Instead show overridden property sets in italic. Show information in the status bar about overridding.

@AaronGreenhouse
Copy link
Contributor

Redid this.

Property pane now shows a list of all the contributed resources. You can select one, and click on an override button. This allows you select a file with the same name in the workspace to replace it with. Alternatively, can use the "restore" button to return it to normal.

Within the AADL navigator, the location of the overriding (workspace) resource is used to locate the resource under "Plug-in Contributions". But selecting and overridden resource shows the location of the original and replacement uris in the description line.

(It is too hard to force the navigator to show the location of the original file, but still use the location of the replacement resource internally.)

Still need to update the help docs, but waiting for some more feedback before I do that.

@AaronGreenhouse
Copy link
Contributor

AaronGreenhouse commented Sep 15, 2020

More comments from @lwrage:

  • AADL navigator should always show the location of the plug-in contributed file; The status bar should show the location of the overriding file (if any). The status bar should show the contributing plug-in in the normal case.

  • AADL navigator should show in italics if the resource is overridden

  • Browser launched by the preference pane should use the filter to only show eligible files (if possible). Otherwise, at least filter out closed projects and non-AADL projects.

  • Ideally the list in the preference pane should be a tree that resembles the AADL Navigator. (Hand this off to @asazonova )

  • May need a IResourceChangeListener of some sort to handle the case of the location of the overriding resource being changed. Not sure where this listener should come from and how to make it installed in a timely manner.

  • When revising the help/docs make sure to mention the recommendation that overriding files should be in their own projects without any other files that depend on them. The projects should be depended upon by any other projects.

@AaronGreenhouse
Copy link
Contributor

It is not possible to change the text style of the navigator labels. The existing (old) way of doing things does not change the text style either. We could add a decorator to the icon, but then I have to get artistic and that seems hard. We can add to the label of the item though, either postfix " (overridden)", or just enclosing the name in brackets of some sort.

@AaronGreenhouse
Copy link
Contributor

Updated so that a contributed property set that is overridden is bracketed by "<...>" in the AADL Navigator.

Changed the selection dialog in the contributed resource preference pane to only show acceptable overriding resources.

@AaronGreenhouse
Copy link
Contributor

Updated the status bar to show the contributing plug-in

@AaronGreenhouse
Copy link
Contributor

Updated so that the AADL navigator always shows a subtree based on the contributed resources. If a URI is overridden, the label is marked " (overridden)" and the status line shows the overriding uri.

@AaronGreenhouse
Copy link
Contributor

AaronGreenhouse commented Sep 16, 2020

Need an IResourceChangeListener installed by PluginSupportPlugin that deals with overriding workspace resources being moved or deleted. (Looking at ProjectRenameHandler for inspiration.)

@AaronGreenhouse
Copy link
Contributor

AaronGreenhouse commented Sep 17, 2020

PluginSupportPlugin adds a IResourceChangeListener` to watch for changes to the workspace resources that override plug-in contributions. If a file is moved, then the overrides mapping is updated. If a file is deleted or renamed then it is removed the mapping.

Had to be careful to shutdown the listener during the "project closing and opening" phase otherwise it breaks everything because closing a project counts as deleting a file.

Also had to be careful to avoid spurious rebuilds of the state.

@AaronGreenhouse
Copy link
Contributor

Need to redo the display in the preference pane. Should pass that task to @asazonova .

@AaronGreenhouse
Copy link
Contributor

For @asazonova:

ContributedResourcesPreferencePage currently uses a bland ListViewer to display the list of contributed resources:

Screen Shot 2020-09-21 at 2.49.52 PM.png

We should switch to a tree viewer that is more similar to the AADL navigator:

Screen Shot 2020-09-21 at 2.50.21 PM.png

The only catch is that when a resource is overridden by a workspace file the label should change to indicate this. Right now I have an "(Overridden)" suffix. If you have a better idea that is good too.

@AaronGreenhouse
Copy link
Contributor

Had a problem today where the overridding wasn't working. Seems that my .prefs file in the workspace was messed up. When I deleted the prefs for org.osate.pluginsupport and tried again, everything worked fine.

I updated PredeclaredProperties.setOverriddenResources() to clean up the old settings first. Hopefully this prevents problems in the future.

	public static synchronized void setOverriddenResources(final Map<URI, URI> replaced) {
		selfUpdating = true;

		/*
		 * First clean up the old settings. This isn't strictly necessary, but things can bet confusing if the
		 * number of overrides shrinks but the old key and value preferences are still left hanging around.
		 */
		final int oldSize = preferenceStore.getInt(NUMBER_OF_WORKSPACE_OVERRIDES);
		for (int i = 0; i < oldSize; i++) {
			final String keyName = WORKSPACE_OVERRIDE_KEY_PREFIX + i;
			preferenceStore.setToDefault(keyName);

			final String valueName = WORKSPACE_OVERRIDE_VALUE_PREFIX + i;
			preferenceStore.setToDefault(valueName);
		}
		preferenceStore.setToDefault(NUMBER_OF_WORKSPACE_OVERRIDES);

		/* Now set the new values */
		final int size = replaced.size();
		preferenceStore.setValue(NUMBER_OF_WORKSPACE_OVERRIDES, size);
		int i = 0;
		for (Map.Entry<URI, URI> entry : replaced.entrySet()) {
			final String keyName = WORKSPACE_OVERRIDE_KEY_PREFIX + i;
			preferenceStore.setValue(keyName, entry.getKey().toString());

			final String valueName = WORKSPACE_OVERRIDE_VALUE_PREFIX + i;
			preferenceStore.setValue(valueName, entry.getValue().toString());
		}
		isChanged = true;
		selfUpdating = false;
	}

@AaronGreenhouse
Copy link
Contributor

Merged tree viewer changes from Anna. Added a double-click listener to the tree: double-clicking a directory will expand/collapse the tree. Double-clicking a file node is like clicking the "override" button.

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.

4 participants