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

mehmet-karaman · 2024-07-23T10:33:55Z

This bugfix contains the fix for unsync workspace files wrt. the file system (see the
DirtyStateEditorSupportIntegrationTest.testModifyFileInExternEditor) and a one liner for beeing able to save the file when the changes were ignored. (see the ignored test case
DirtyStateEditorSupportIntegrationTest.testModifyDirtyFileInExternEditor).

The issue for this fix can be found here:
#2385

iloveeclipse · 2024-07-23T11:14:42Z

org.eclipse.xtext.ui/src/org/eclipse/xtext/ui/editor/model/XtextDocumentProvider.java

-			synchronizationStamp = ((URIInfo) info).synchronizationStamp;
-		} else {
-			return super.isSynchronized(element);
+		if (element instanceof IFileEditorInput) {


I personally avoid many nested if levels, so I would revert the condition and also use modern instanceof
if (!(element instanceof IFileEditorInput input)){
return super.isSynchronized(element);
}

// continue with the new code
// "input" is already defined here with the right type

Currently the project uses java 11. Am I allowed to upgrade to java 17 or 21?

Am I allowed to upgrade to java 17 or 21?

I would say, definitely not in the context of this ticket, but it is a good question and probably deserves a dedicated ticket in Xtext.

Note that Eclipse Platform is switched to Java 17 in Eclipse 4.28, including ecj (see eclipse-jdt/eclipse.jdt.core#886).

there is already a ticket for drop 11 17 minimum
#2686

iloveeclipse · 2024-07-23T12:07:32Z

There are 7 test failures:

Test Result (7 failures )
org.eclipse.xtend.ide.tests.hover.XtendHoverInEditorTest.testHoverOfJavaTypeInJavadocLink
org.eclipse.xtend.ide.tests.hover.XtendHoverInEditorTest.testHoverOfFQNJavaTypeInJavadocLink
org.eclipse.xtend.ide.tests.hover.XtendHoverInEditorTest.testHoverOfJavaType
org.eclipse.xtend.ide.tests.hover.XtendHoverInEditorTest.testHoverOfFQNJavaType
org.eclipse.xtend.ide.tests.hover.XtendHoverInEditorTest.testHoverOfJavaTypeInJavadocSee
org.eclipse.xtend.ide.tests.hover.XtendHoverInEditorTest.testHoverOfFQNJavaTypeInJavadocSee
org.eclipse.xtend.ide.tests.quickfix.QuickfixTest.missingConcreteMembers2

@cdietrich : are the test failures known?
@mehmet-karaman : could you please check if the tests are failing locally with your patch?

cdietrich · 2024-07-23T13:28:02Z

i am only aware of #3102
the others are new to me

cdietrich · 2024-07-23T13:48:37Z

started j11 build on main
https://ci.eclipse.org/xtext/job/xtext/job/main/1325/

i also wonder if the base branch should be rebase.

cdietrich · 2024-07-23T14:54:44Z

i can reproduce locally, but no idea why
@LorenzoBettini do you know why this
https://ci.eclipse.org/xtext/job/xtext/job/main/1325/consoleFull
seems to run with 17 and not 11?

maybe something is broken with that
why was it working here yesterday:
https://ci.eclipse.org/xtext/job/xtext/job/cd-require-mocha-bump/1/
(reran this one)

and also scheduled
https://ci.eclipse.org/xtext/job/xtext/job/PR-3108/ to run with 17 explicit and 2024-06

LorenzoBettini · 2024-07-23T16:07:24Z

i can reproduce locally, but no idea why @LorenzoBettini do you know why this https://ci.eclipse.org/xtext/job/xtext/job/main/1325/consoleFull seems to run with 17 and not 11?

@cdietrich from what I see

09:52:11  [INFO] Command line:
09:52:11  	[/opt/tools/java/temurin/jdk-11/latest/bin/java

it runs using JDK 11 (of course, the build runs with Java 17, but the toolchain uses JDK11)

mehmet-karaman · 2024-07-24T07:59:21Z

The failing tests are running successful on my branch. I am going to rebase on the latest state and try agian.

mehmet-karaman · 2024-07-24T08:07:37Z

Tests are also successful after rebase.. Pushed the branch again.

iloveeclipse · 2024-07-24T13:49:26Z

Jenkins is fine, new test is executed. Whatever the pipeline is doing and why it is failing is not our beer, it seem to fail on all other recent PR's too.

Xtext committers: Could we proceed with the review?

mehmet-karaman · 2024-08-06T13:30:16Z

@szarnekow @cdietrich Do i have to do something on my side? Does this PR has any problems?

cdietrich · 2024-08-06T13:51:58Z

problem: ustream is never green cause all of the jdt changes.
i want sebastian to have a review cause i cannot judge if it may have bad side effect i dont see.

cdietrich · 2024-08-08T13:55:40Z

@mehmet-karaman can you please rebase

iloveeclipse · 2024-08-09T08:48:15Z

All tests are green now. Cool.

iloveeclipse · 2024-08-09T14:01:31Z

@cdietrich, @szarnekow : could we proceed with the review now?

cdietrich · 2024-08-09T14:03:36Z

as i said i cannot judge it it will break something. and in my 12 and 13th working hour i also have no capacity to think about it.

and sebastian has no time too i assume

LorenzoBettini · 2024-08-09T14:11:52Z

...rc-longrunning/org/eclipse/xtext/ui/tests/editor/DirtyStateEditorSupportIntegrationTest.java

+	}
+
+	/**
+	 * This test needs a manual button click.


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

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.

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.

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

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.

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.

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

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

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

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

org.eclipse.xtext.ui/src/org/eclipse/xtext/ui/editor/XtextEditor.java

org.eclipse.xtext.ui/src/org/eclipse/xtext/ui/editor/model/XtextDocumentProvider.java

github-actions · 2024-08-21T11:08:34Z

Test Results

6 460 files ± 0 6 460 suites ±0 3h 1m 54s ⏱️ - 17m 21s
43 242 tests + 2 42 658 ✅ +1 584 💤 +1 0 ❌ ±0
170 114 runs +17 167 740 ✅ +4 2 358 💤 +4 12 ❌ +5 4 🔥 +4

Results for commit 5fa196f. ± Comparison against base commit 197f6cc.

♻️ This comment has been updated with latest results.

mehmet-karaman · 2024-08-21T11:36:47Z

I am going to rebase against the latest master.

iloveeclipse · 2024-08-28T07:58:27Z

@szarnekow, @cdietrich : anything is missing here? Could we continue with review or merge?

szarnekow · 2024-08-31T12:55:54Z

org.eclipse.xtext.ui/src/org/eclipse/xtext/ui/editor/model/XtextDocumentProvider.java

+
+		// Check has to fit the following method: 
+		// org.eclipse.ui.editors.text.FileDocumentProvider.checkSynchronizationState(long, IResource)
+		boolean isSynchronized = synchronizationStamp == getSynchronizationStamp(input.getFile());


I think I don't understand the idea behind this change.
Line 425 takes the modificationStamp and stores it as a local synchronizationStamp. Now we try to read getSynchronizationStamp(someIFile) which does

if (element instanceof IFileEditorInput) { FileInfo info= (FileInfo) getElementInfo(element); if (info != null) return info.fModificationStamp; } return super.getSynchronizationStamp(element);

it'll always go into the super.getSynchronizationStamp() which returns 0.
Something doesn't really work for me with this code.
Did you mean to call computeModificationStamp instead of getSynchronizationStamp.

If so: There must be something wrong with the tests. They should have caught this.
If not: Please explain (possibly again) in full detail what the idea is.

You're right.. It could never be synchron, the file was reloaded usually. I guess I've to to store the time when the file was loaded and reloaded and compare with isSynchron.. Otherwise it will just compare the real files modification time stamp and the IResource modification timestamp.. But in this case the editor content has to be synchron, not just the file.

I will add a few more asserts to check for isSynchronized() true/false, then we should be more secure with the tests.

I've checked why this override was introduced, but then it doesn't makes a sense to keep this override.
It was introduced to fix the the saving of files which are out of sync: https://bugs.eclipse.org/bugs/show_bug.cgi?id=343894. So If i remove this override, we will have the same problem back, but if I keep my changes in the XtextEditor class and call performSave(true, progressMonitor) .. (example from TextEditor which also forces the save)

So might be someone has fixed the first bug in a wrong place and caused the unsync content problem.

Removed the isSynchronized() overriden method, adjusted the performSave to force override .. Added more assertions to test.

szarnekow · 2024-09-03T06:50:06Z

org.eclipse.xtext.ui/src/org/eclipse/xtext/ui/editor/XtextEditor.java

@@ -327,6 +329,12 @@ public void doSave(IProgressMonitor progressMonitor) {
 		callback.afterSave(this);
 	}

+	@Override
+	protected void performSave(boolean overwrite, IProgressMonitor progressMonitor) {
+		super.performSave(true, progressMonitor);


This is not something we can merge. Always enforcing an overwrite seems to be the wrong attack vector for the problem at hand.

In comparison to the functionality of the org.eclipse.ui.editors.text.TextFileDocumentProvider we see that they force overwrite in the method org.eclipse.ui.editors.text.TextFileDocumentProvider.createFileFromDocument(IProgressMonitor, IFile, IDocument) .

Also JDT forces the write if the file is synchronized..
org.eclipse.jdt.internal.ui.javaeditor.CompilationUnitDocumentProvider.commitWorkingCopy(IProgressMonitor, Object, CompilationUnitInfo, boolean)

I've adjusted my solution similar to JDT .. I hope this is fine now.

memin20 · 2024-09-03T21:13:18Z

I am now on my private MacBook and try to understand why it fails constantly on macOS but works on windows and ubuntu..

This bugfix contains the fix for unsync workspace files wrt. the file system (see the DirtyStateEditorSupportIntegrationTest.testModifyFileInExternEditor) and a one liner for beeing able to save the file when the changes were ignored. (see the ignored test case DirtyStateEditorSupportIntegrationTest.testModifyDirtyFileInExternEditor). The issue for this fix can be found here: eclipse#2385

mehmet-karaman · 2024-09-03T21:49:08Z

Added waitForBuild to solve this async problem on Mac.. Seems that the build takes longer on mac machines .. Locally it worked already.

szarnekow · 2024-09-04T06:56:24Z

...rc-longrunning/org/eclipse/xtext/ui/tests/editor/DirtyStateEditorSupportIntegrationTest.java

+	/**
+	 * This test needs a manual button click.
+	 */
+	@Ignore


When I get this dialog:

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.

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..

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

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.

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

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

It works for me..

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

Mehmet is in my team, and thanks for asking.

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

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

szarnekow

Change looks good.

cdietrich requested a review from szarnekow July 23, 2024 11:08

iloveeclipse reviewed Jul 23, 2024

View reviewed changes

mehmet-karaman force-pushed the fix_xtext_editors_sync_ext_editing branch from 52d207b to f34d2fe Compare July 23, 2024 13:38

mehmet-karaman force-pushed the fix_xtext_editors_sync_ext_editing branch from f34d2fe to 3f5e61d Compare July 24, 2024 08:06

mehmet-karaman force-pushed the fix_xtext_editors_sync_ext_editing branch from 3f5e61d to d7e4fd9 Compare August 9, 2024 07:08

LorenzoBettini reviewed Aug 9, 2024

View reviewed changes

szarnekow reviewed Aug 16, 2024

View reviewed changes

org.eclipse.xtext.ui/src/org/eclipse/xtext/ui/editor/XtextEditor.java Outdated Show resolved Hide resolved

szarnekow reviewed Aug 16, 2024

View reviewed changes

org.eclipse.xtext.ui/src/org/eclipse/xtext/ui/editor/model/XtextDocumentProvider.java Outdated Show resolved Hide resolved

mehmet-karaman force-pushed the fix_xtext_editors_sync_ext_editing branch from d7e4fd9 to a73abd6 Compare August 21, 2024 09:55

mehmet-karaman force-pushed the fix_xtext_editors_sync_ext_editing branch from a73abd6 to be83d08 Compare August 21, 2024 11:38

szarnekow reviewed Aug 31, 2024

View reviewed changes

mehmet-karaman force-pushed the fix_xtext_editors_sync_ext_editing branch from be83d08 to 64275cb Compare September 2, 2024 15:12

szarnekow reviewed Sep 3, 2024

View reviewed changes

mehmet-karaman force-pushed the fix_xtext_editors_sync_ext_editing branch 2 times, most recently from 41b8718 to 9ef866f Compare September 3, 2024 12:01

mehmet-karaman force-pushed the fix_xtext_editors_sync_ext_editing branch from 9ef866f to 5fa196f Compare September 3, 2024 21:45

szarnekow reviewed Sep 4, 2024

View reviewed changes

szarnekow approved these changes Sep 5, 2024

View reviewed changes

szarnekow merged commit 45d8078 into eclipse:main Sep 5, 2024
9 checks passed

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

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

Conversation

mehmet-karaman commented Jul 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iloveeclipse commented Jul 23, 2024

cdietrich commented Jul 23, 2024

cdietrich commented Jul 23, 2024 • edited Loading

cdietrich commented Jul 23, 2024 • edited Loading

LorenzoBettini commented Jul 23, 2024

mehmet-karaman commented Jul 24, 2024

mehmet-karaman commented Jul 24, 2024

iloveeclipse commented Jul 24, 2024

mehmet-karaman commented Aug 6, 2024

cdietrich commented Aug 6, 2024

cdietrich commented Aug 8, 2024

iloveeclipse commented Aug 9, 2024

iloveeclipse commented Aug 9, 2024

cdietrich commented Aug 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

memin20 Sep 4, 2024 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Aug 21, 2024 • edited Loading

Test Results

mehmet-karaman commented Aug 21, 2024

iloveeclipse commented Aug 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

memin20 commented Sep 3, 2024

mehmet-karaman commented Sep 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

memin20 Sep 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

szarnekow left a comment

Choose a reason for hiding this comment

cdietrich commented Jul 23, 2024 •

edited

Loading

cdietrich commented Jul 23, 2024 •

edited

Loading

memin20 Sep 4, 2024 •

edited

Loading

github-actions bot commented Aug 21, 2024 •

edited

Loading

memin20 Sep 4, 2024 •

edited

Loading