eclipse-xtext / eclipse-xtext/xtext

How to correctly modify the order of containment references in an XTextEditor?

Open
#2,592 7 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
Java
Stars
831
Forks
330
Avg merge
3d 7h
Merged PRs (30d)
12

Description

There is a long description of the problem here:
https://www.eclipse.org/forums/index.php?t=msg&th=1089958&goto=1776135&#msg_1776135

The gist of it is as follows:

  1. The objective is to re-order contained elements in a DSL-specific resource:
  • via a service API to provide re-ordering support for model-to-model transformations in general
  • via a context menu in the DSL-specific XTextEditor that calls the service API
  1. At the UI level, the service is invoked via XTextEditor?.document?.modify(...)
    which is executed within org.eclipse.xtext.ui.editor.model.edit.ReconcilingUnitOfWork.exec().

  2. As @cdietrich observed, the Serializer is invoked after the modifications have been made.
    I observed that if I leave the org.eclipse.xtext.nodemodel.INode annotations on the elements,
    then seems that some essential whitespace is lost. The effect of this is that the stings of adjacent tokens are no longer separated by whitespace and thus will no longer be parsed as distinct tokens but instead as single one. For my DSL, this produces several parse errors.

To avoid confusion between the updated order of the elements relative to their stale INode annotations, I systematically deleted all INode annotations during the modification.
In my limited experimentation, this seems to work; however, this raises several questions:

  1. What is the proper protocol for shuffling the order of containment references?

For example, looking at the logic of DefaultTextEditComposer.endRecording(),
I figured that I ought to explicitly mark the resource as being modified in my handler like this:

public class NormalizeOMLContentsOrder extends AbstractHandler {
	
	override def Object execute(ExecutionEvent event) throws ExecutionException {
		val XtextEditor editor = EditorUtils.getActiveXtextEditor(event)
		val IXtextDocument doc = editor?.document
		if (null !== doc) {
			doc.modify(normalizeOMLResource)
		}
		null
	}
	
	protected static val IUnitOfWork.Void<XtextResource> normalizeOMLResource = new IUnitOfWork.Void<XtextResource>() {

		override def void process(XtextResource state) throws Exception {
			state.contents.filter(Extent).forEach [ ext |
				OMLExtensions.normalize(ext) // This does the shuffling...
			]
			state.modified = true // this seems important for DefaultTextEditComposer.endRecording()
		}
	}
	
}

I'm wondering whether it would make sense to provide a special IUnitOfWork for this kind of shuffling modification.

  1. Deleting the INode annotations seems overkill to me.

One of the side-effects is that all whitespace is lost.
In my DSL, comments are whitespace because they have no semantic significance.
However, for authoring purposes (instead of M2M-generated content), comments are important.
As much as possible, I'd like to preserve the relative position of comments w.r.t. the commented elements. For example, if by convention I wanted to say that a comment appears before an element, I'd like the shuffling operation to shuffle the comments as well.
I don't know how to do that.

  1. What could go wrong if one one deletes the INode annotations anyway?

With undo/redo, I managed to get in a broken state:

org.eclipse.e4.core.di.InjectionException: java.lang.IllegalStateException: Cannot replace an obj that has no associated node
	at org.eclipse.e4.core.internal.di.MethodRequestor.execute(MethodRequestor.java:65)
	at org.eclipse.e4.core.internal.di.InjectorImpl.invokeUsingClass(InjectorImpl.java:305)
	at org.eclipse.e4.core.internal.di.InjectorImpl.invoke(InjectorImpl.java:239)
	at org.eclipse.e4.core.contexts.ContextInjectionFactory.invoke(ContextInjectionFactory.java:132)
	at org.eclipse.e4.core.commands.internal.HandlerServiceHandler.execute(HandlerServiceHandler.java:152)
	at org.eclipse.core.commands.Command.executeWithChecks(Command.java:494)
	at org.eclipse.core.commands.ParameterizedCommand.executeWithChecks(ParameterizedCommand.java:487)
	at org.eclipse.e4.core.commands.internal.HandlerServiceImpl.executeHandler(HandlerServiceImpl.java:210)
	at org.eclipse.e4.ui.workbench.renderers.swt.HandledContributionItem.executeItem(HandledContributionItem.java:431)
	at org.eclipse.e4.ui.workbench.renderers.swt.AbstractContributionItem.handleWidgetSelection(AbstractContributionItem.java:446)
	at org.eclipse.e4.ui.workbench.renderers.swt.AbstractContributionItem.lambda$2(AbstractContributionItem.java:472)
	at org.eclipse.swt.widgets.EventTable.sendEvent(EventTable.java:86)
	at org.eclipse.swt.widgets.Display.sendEvent(Display.java:4257)
	at org.eclipse.swt.widgets.Widget.sendEvent(Widget.java:1502)
	at org.eclipse.swt.widgets.Widget.sendEvent(Widget.java:1525)
	at org.eclipse.swt.widgets.Widget.sendEvent(Widget.java:1510)
	at org.eclipse.swt.widgets.Widget.notifyListeners(Widget.java:1314)
	at org.eclipse.swt.widgets.Display.runDeferredEvents(Display.java:4081)
	at org.eclipse.swt.widgets.Display.readAndDispatch(Display.java:3698)
	at org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine$5.run(PartRenderingEngine.java:1150)
	at org.eclipse.core.databinding.observable.Realm.runWithDefault(Realm.java:336)
	at org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine.run(PartRenderingEngine.java:1039)
	at org.eclipse.e4.ui.internal.workbench.E4Workbench.createAndRunUI(E4Workbench.java:153)
	at org.eclipse.ui.internal.Workbench.lambda$3(Workbench.java:680)
	at org.eclipse.core.databinding.observable.Realm.runWithDefault(Realm.java:336)
	at org.eclipse.ui.internal.Workbench.createAndRunWorkbench(Workbench.java:594)
	at org.eclipse.ui.PlatformUI.createAndRunWorkbench(PlatformUI.java:148)
	at org.eclipse.ui.internal.ide.application.IDEApplication.start(IDEApplication.java:151)
	at org.eclipse.equinox.internal.app.EclipseAppHandle.run(EclipseAppHandle.java:196)
	at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.runApplication(EclipseAppLauncher.java:134)
	at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.start(EclipseAppLauncher.java:104)
	at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:388)
	at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:243)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at org.eclipse.equinox.launcher.Main.invokeFramework(Main.java:653)
	at org.eclipse.equinox.launcher.Main.basicRun(Main.java:590)
	at org.eclipse.equinox.launcher.Main.run(Main.java:1499)
	at org.eclipse.equinox.launcher.Main.main(Main.java:1472)
Caused by: java.lang.IllegalStateException: Cannot replace an obj that has no associated node
	at org.eclipse.xtext.serializer.impl.Serializer.serializeReplacement(Serializer.java:200)
	at org.eclipse.xtext.ui.editor.model.edit.DefaultTextEditComposer.getObjectEdits(DefaultTextEditComposer.java:176)
	at org.eclipse.xtext.ui.editor.model.edit.DefaultTextEditComposer.getTextEdit(DefaultTextEditComposer.java:146)
	at org.eclipse.xtext.ui.editor.model.edit.DefaultTextEditComposer.endRecording(DefaultTextEditComposer.java:129)
	at org.eclipse.xtext.ui.editor.model.edit.ReconcilingUnitOfWork.exec(ReconcilingUnitOfWork.java:57)
	at org.eclipse.xtext.ui.editor.model.edit.ReconcilingUnitOfWork.exec(ReconcilingUnitOfWork.java:1)
	at org.eclipse.xtext.resource.OutdatedStateManager.exec(OutdatedStateManager.java:91)
	at org.eclipse.xtext.ui.editor.model.XtextDocument$XtextDocumentLocker.modify(XtextDocument.java:428)
	at org.eclipse.xtext.ui.editor.model.XtextDocument.internalModify(XtextDocument.java:162)
	at org.eclipse.xtext.ui.editor.model.XtextDocument.modify(XtextDocument.java:155)
	at gov.nasa.jpl.imce.oml.dsl.ui.handlers.NormalizeOMLContentsOrder.execute(NormalizeOMLContentsOrder.java:46)
	at org.eclipse.ui.internal.handlers.HandlerProxy.execute(HandlerProxy.java:291)
	at org.eclipse.ui.internal.handlers.E4HandlerProxy.execute(E4HandlerProxy.java:92)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at org.eclipse.e4.core.internal.di.MethodRequestor.execute(MethodRequestor.java:55)
	... 40 more

I'm not sure how to prevent this from happening.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with XtextDocument.modify(), ReconcilingUnitOfWork.exec(), DefaultTextEditComposer.endRecording(), and Serializer.serializeReplacement(), following how reordered containment references and removed INode annotations are handled. Determine the supported protocol for reordering while preserving whitespace and comments, and define behavior that avoids the reported undo/redo IllegalStateException.

Written by the indexing model from the issue text.

Assessment

Tech stack
java
Domain
tooling
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Needs clarification
Newbie friendliness
25/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.