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.

Sign up for GitHub

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

Jump to bottom

Fix editor sync bug, when model file was edited outside of eclipse. #3108

Merged
merged 1 commit into from
Sep 5, 2024
Merged

Fix editor sync bug, when model file was edited outside of eclipse. #3108

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
73 changes: 73 additions & 0 deletions ...longrunning/org/eclipse/xtext/ui/tests/editor/DirtyStateEditorSupportIntegrationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,28 +8,36 @@
*/
package org.eclipse.xtext.ui.tests.editor;

import java.io.File;
import java.io.FileWriter;
import java.util.ArrayList;
import java.util.List;
import java.util.Objects;

import org.eclipse.core.resources.IFile;
import org.eclipse.core.resources.ResourcesPlugin;
import org.eclipse.core.runtime.NullProgressMonitor;
import org.eclipse.core.runtime.jobs.Job;
import org.eclipse.emf.common.util.URI;
import org.eclipse.jface.text.IUndoManager;
import org.eclipse.swt.SWT;
import org.eclipse.swt.custom.StyledText;
import org.eclipse.swt.widgets.Display;
import org.eclipse.swt.widgets.Event;
import org.eclipse.ui.IFileEditorInput;
import org.eclipse.ui.editors.text.FileDocumentProvider;
import org.eclipse.xtext.resource.IEObjectDescription;
import org.eclipse.xtext.resource.IResourceDescription;
import org.eclipse.xtext.resource.IResourceServiceProvider;
import org.eclipse.xtext.ui.editor.XtextEditor;
import org.eclipse.xtext.ui.editor.XtextSourceViewer;
import org.eclipse.xtext.ui.editor.model.IXtextDocument;
import org.eclipse.xtext.ui.refactoring.ui.SyncUtil;
import org.eclipse.xtext.ui.testing.AbstractEditorTest;
import org.eclipse.xtext.ui.testing.util.IResourcesSetupUtil;
import org.junit.Assert;
import org.junit.Before;
import org.junit.Ignore;
import org.junit.Test;

import com.google.common.collect.Iterables;
Expand Down Expand Up @@ -116,7 +124,72 @@ public void testUndoRedo() throws Exception {
.getQualifiedName().getLastSegment());
Iterables.getLast(events);
}

/*
* @see https://github.com/eclipse/xtext/issues/2385
*/
@Test public void testModifyFileInExternEditor() throws Exception {
IXtextDocument document = editor.getDocument();

Display.getDefault().readAndDispatch();

assertNotEquals(document.get(), "");
assertTrue(editor.getDocumentProvider() instanceof FileDocumentProvider);

FileDocumentProvider fileDocumentProvider = (FileDocumentProvider)editor.getDocumentProvider();

getWorkbench().getActiveWorkbenchWindow().getActivePage().getViewReferences()[0].getView(false).setFocus();

File externalEditFile = ((IFileEditorInput)editor.getEditorInput()).getFile().getLocation().toFile();
try (FileWriter fw = new FileWriter(externalEditFile)) {
fw.write("");
}

assertFalse(fileDocumentProvider.isSynchronized(editor.getEditorInput()));

editor.setFocus();
Job.getJobManager().join(ResourcesPlugin.FAMILY_AUTO_REFRESH, null);
syncUtil.yieldToQueuedDisplayJobs(new NullProgressMonitor());
syncUtil.waitForReconciler(editor);
syncUtil.waitForBuild(new NullProgressMonitor());

assertTrue(fileDocumentProvider.isSynchronized(editor.getEditorInput()));

assertEquals(document.get(), "");
}

/**
* This test needs a manual button click.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment sounds strange: does a human have to click on the button to make the test proceed?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exact, this is the reason the test is "ignored".
Unfortunately a confirmation dialog pops up during the test, so the test is there to demonstrate the fix.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mine is not meant to be a review, but I don't understand what the test does: if it's ignored, it doesn't test anything.

I don't think clicking on a dialog button is (easily) doable in a plug-in test, but it's easy in a SWTBot test.

Maybe a SWTBot test to effectively verify the fix would be better.

But, again, I did not follow the whole bug and fix.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, therefore there is one automated test and one that needs manual interaction.
Not sure if SWTBot is used in Xtext code.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We use SWTBot tests, just a few, to test Xtend dialogs.

My main concern is that there's no automatic test for verifying the fix, again, if I understand correctly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is one test which is executed automatically, which tests the basic functionality.. The ignored test is testing the case when the editor is dirty and the file is out of sync case.. I didn't saw an example how to automate a click on a button, without introducing a new dependency.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test is green without the changes in XtextEditor. Do you have an idea how to assert the necessity of the the overridden performSave?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@szarnekow : also the manual test is green without changes?

Copy link

@memin20 memin20 Sep 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For this the ignored test has to be unignored and executed manually..

I am Mehmet, Just writing on my phone because my doctors appointment..

*/
@Ignore
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I get this dialog:

Screenshot 2024-09-04 at 08 52 51

I'd expect that Replace editor content sets the editor content to the text in the file. But apparently it'll override the file change on disk. That's not ok.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

really? I can't test It yet, as far as i remember it discards the changes in the editor.. Its not supposed to write anything on the disc..

Copy link

@memin20 memin20 Sep 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw. You have to klick "ignore file changes" and then the changes in XtextEditor is necessary..

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

really? I can't test It yet, as far as i remember it discards the changes in the editor.. Its not supposed to write anything on the disc..

Yes, I tried this a couple of times and added additional assertions to confirm my observation.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be that this is due to the doSave() though. Testing again.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it is. @iloveeclipse can you confirm that it works for you guys?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It works for me..

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it is. @iloveeclipse can you confirm that it works for you guys?

Mehmet is in my team, and thanks for asking.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent. So let's move this forward. Thank you for the fix and the patience with the review process @mehmet-karaman

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@szarnekow Thank you for your review and yours hints..

@Test public void testModifyDirtyFileInExternEditor() throws Exception {
IXtextDocument document = editor.getDocument();

Display.getDefault().readAndDispatch();

assertNotEquals(document.get(), "");
document.set("/* Hello World! */");
assertEquals("/* Hello World! */", document.get());
assertTrue(editor.isDirty());

getWorkbench().getActiveWorkbenchWindow().getActivePage().getViewReferences()[0].getView(false).setFocus();

File externalEditFile = ((IFileEditorInput)editor.getEditorInput()).getFile().getLocation().toFile();
try (FileWriter fw = new FileWriter(externalEditFile)) {
fw.write("");
}
editor.setFocus();
// Dialog opens and the "Ignore file change" button has to be clicked.
Job.getJobManager().join(ResourcesPlugin.FAMILY_AUTO_REFRESH, null);
syncUtil.yieldToQueuedDisplayJobs(new NullProgressMonitor());
syncUtil.waitForReconciler(editor);
assertEquals(document.get(), "/* Hello World! */");

assertTrue(editor.isDirty());
editor.doSave(new NullProgressMonitor());
assertFalse(editor.isDirty());
}

private IEObjectDescription getFirstExportedObjectInLastEventDelta() {
return Iterables.getFirst(Iterables.getLast(events).getDeltas().get(0).getNew().getExportedObjects(), null);
}
Expand Down
16 changes: 16 additions & 0 deletions org.eclipse.xtext.ui/src/org/eclipse/xtext/ui/editor/XtextEditor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,8 @@
import org.eclipse.ui.PlatformUI;
import org.eclipse.ui.editors.text.TextEditor;
import org.eclipse.ui.texteditor.DefaultMarkerAnnotationAccess;
import org.eclipse.ui.texteditor.IDocumentProvider;
import org.eclipse.ui.texteditor.IDocumentProviderExtension3;
import org.eclipse.ui.texteditor.ITextEditor;
import org.eclipse.ui.texteditor.ITextEditorActionConstants;
import org.eclipse.ui.texteditor.ITextEditorActionDefinitionIds;
Expand Down Expand Up @@ -327,6 +329,20 @@ public void doSave(IProgressMonitor progressMonitor) {
callback.afterSave(this);
}

@Override
protected void performSave(boolean overwrite, IProgressMonitor progressMonitor) {
super.performSave(overwrite || isSynchronized(), progressMonitor);
}


private boolean isSynchronized() {
IResource resource = getResource();
if (resource != null) {
return resource.isSynchronized(IResource.DEPTH_ZERO);
}
return false;
}

@Override
protected void updateState(final IEditorInput input) {
new DisplayRunnable() {
Expand Down
14 changes: 0 additions & 14 deletions org.eclipse.xtext.ui/src/org/eclipse/xtext/ui/editor/model/XtextDocumentProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -408,20 +408,6 @@ public long getModificationStamp(Object element) {
}
}

@Override
public boolean isSynchronized(Object element) {
ElementInfo info = getElementInfo(element);
long synchronizationStamp;
if (info instanceof FileInfo) {
synchronizationStamp = ((FileInfo) info).fModificationStamp;
} else if (info instanceof URIInfo) {
synchronizationStamp = ((URIInfo) info).synchronizationStamp;
} else {
return super.isSynchronized(element);
}
return synchronizationStamp == getModificationStamp(element);
}

@Override
public boolean isModifiable(Object element) {
if (isWorkspaceExternalEditorInput(element)) {
Expand Down
Loading