From 810e4135f02e48c05420298bf159e62d29ea4cc6 Mon Sep 17 00:00:00 2001 From: peter Date: Thu, 3 Sep 2015 08:18:03 +0200 Subject: [PATCH] fix PsiToDocumentSynchronizer handling of overlapping ranges --- .../psi/impl/PsiToDocumentSynchronizer.java | 241 ++++++------------ .../openapi/editor/impl/RangeMarkerTest.java | 35 ++- .../com/intellij/codeInsight/XmlTagTest.java | 15 ++ 3 files changed, 115 insertions(+), 176 deletions(-) diff --git a/platform/core-impl/src/com/intellij/psi/impl/PsiToDocumentSynchronizer.java b/platform/core-impl/src/com/intellij/psi/impl/PsiToDocumentSynchronizer.java index ef5398db1f0c..22861d1d4ff6 100644 --- a/platform/core-impl/src/com/intellij/psi/impl/PsiToDocumentSynchronizer.java +++ b/platform/core-impl/src/com/intellij/psi/impl/PsiToDocumentSynchronizer.java @@ -22,12 +22,16 @@ import com.intellij.openapi.diagnostic.Logger; import com.intellij.openapi.editor.Document; import com.intellij.openapi.editor.ex.DocumentEx; import com.intellij.openapi.project.Project; +import com.intellij.openapi.util.Condition; import com.intellij.openapi.util.Key; import com.intellij.openapi.util.Pair; +import com.intellij.openapi.util.TextRange; import com.intellij.openapi.util.text.StringUtil; import com.intellij.psi.*; import com.intellij.psi.impl.source.tree.ForeignLeafPsiElement; +import com.intellij.util.containers.ContainerUtil; import com.intellij.util.messages.MessageBus; +import com.intellij.util.text.ImmutableText; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; import org.jetbrains.annotations.TestOnly; @@ -120,7 +124,7 @@ public class PsiToDocumentSynchronizer extends PsiTreeChangeAdapter { public void performAtomically(@NotNull PsiFile file, @NotNull Runnable runnable) { assert !isInsideAtomicChange(file); file.putUserData(PSI_DOCUMENT_ATOMIC_ACTION, Boolean.TRUE); - + try { runnable.run(); } @@ -247,7 +251,7 @@ public class PsiToDocumentSynchronizer extends PsiTreeChangeAdapter { ApplicationManager.getApplication().assertIsDispatchThread(); final DocumentChangeTransaction documentChangeTransaction = removeTransaction(document); if(documentChangeTransaction == null) return false; - final PsiElement changeScope = documentChangeTransaction.getChangeScope(); + final PsiElement changeScope = documentChangeTransaction.myChangeScope; try { mySyncDocument = document; @@ -275,28 +279,12 @@ public class PsiToDocumentSynchronizer extends PsiTreeChangeAdapter { try { boolean isReadOnly = !document.isWritable(); ex.setReadOnly(false); - final Set> affectedFragments = documentChangeTransaction.getAffectedFragments(); - for (final Pair pair : affectedFragments) { - final StringBuffer replaceBuffer = pair.getSecond(); - final MutableTextRange range = pair.getFirst(); - if (replaceBuffer.length() == 0) { - ex.deleteString(range.getStartOffset(), range.getEndOffset()); - } - else if (range.getLength() == 0) { - ex.insertString(range.getStartOffset(), replaceBuffer); - } - else { - ex.replaceString(range.getStartOffset(), - range.getEndOffset(), - replaceBuffer); - } + + for (Map.Entry entry : documentChangeTransaction.myAffectedFragments.descendingMap().entrySet()) { + ex.replaceString(entry.getKey().getStartOffset(), entry.getKey().getEndOffset(), entry.getValue()); } ex.setReadOnly(isReadOnly); - //if(documentChangeTransaction.getChangeScope() != null) { - // LOG.assertTrue(document.getText().equals(documentChangeTransaction.getChangeScope().getText()), - // "Psi to document synchronization failed (send to IK)"); - //} } finally { ex.unSuppressGuardedExceptions(); @@ -323,40 +311,36 @@ public class PsiToDocumentSynchronizer extends PsiTreeChangeAdapter { public static class DocumentChangeTransaction{ - private final Set> myAffectedFragments = new TreeSet>(new Comparator>() { + private final TreeMap myAffectedFragments = new TreeMap(new Comparator() { @Override - public int compare(final Pair o1, - final Pair o2) { - return o1.getFirst().getStartOffset() - o2.getFirst().getStartOffset(); + public int compare(TextRange o1, TextRange o2) { + return o1.getStartOffset() - o2.getStartOffset(); } }); - private final Document myDocument; private final PsiFile myChangeScope; + private ImmutableText myDocText; + private ImmutableText myPsiText; public DocumentChangeTransaction(@NotNull Document doc, @NotNull PsiFile scope) { - myDocument = doc; myChangeScope = scope; + myDocText = ImmutableText.valueOf(doc.getImmutableCharSequence()); + myPsiText = myDocText; } @NotNull - public Set> getAffectedFragments() { + public Map getAffectedFragments() { return myAffectedFragments; } - @NotNull - public PsiFile getChangeScope() { - return myChangeScope; - } - - public void replace(int initialStart, int length, @NotNull String replace) { + public void replace(int psiStart, int length, @NotNull String replace) { // calculating fragment // minimize replace int start = 0; int end = start + length; final int replaceLength = replace.length(); - final String chars = getText(start + initialStart, end + initialStart); - if (chars.equals(replace)) return; + final CharSequence chars = myPsiText.subSequence(psiStart, psiStart + length); + if (StringUtil.equals(chars, replace)) return; int newStartInReplace = 0; int newEndInReplace = replaceLength; @@ -385,12 +369,13 @@ public class PsiToDocumentSynchronizer extends PsiTreeChangeAdapter { } } + start += psiStart; + end += psiStart; + //[mike] dirty hack for xml: //make sure that deletion of in: doesn't remove t/>< //which is perfectly valid but invalidates range markers - start += initialStart; - end += initialStart; - final CharSequence charsSequence = myDocument.getCharsSequence(); + final CharSequence charsSequence = myPsiText; while (start < charsSequence.length() && end < charsSequence.length() && start > 0 && charsSequence.subSequence(start, end).toString().endsWith("><") && charsSequence.charAt(start - 1) == '<') { start--; @@ -399,147 +384,71 @@ public class PsiToDocumentSynchronizer extends PsiTreeChangeAdapter { newEndInReplace--; } - replace = replace.substring(newStartInReplace, newEndInReplace); - length = end - start; - - final Pair fragment = getFragmentByRange(start, length); - final StringBuffer fragmentReplaceText = fragment.getSecond(); - final int startInFragment = start - fragment.getFirst().getStartOffset(); - - // text range adjustment - final int lengthDiff = replace.length() - length; - final Iterator> iterator = myAffectedFragments.iterator(); - boolean adjust = false; - while (iterator.hasNext()) { - final Pair pair = iterator.next(); - if (adjust) pair.getFirst().shift(lengthDiff); - if (pair == fragment) adjust = true; - } - - fragmentReplaceText.replace(startInFragment, startInFragment + length, replace); + updateFragments(start, end, replace.substring(newStartInReplace, newEndInReplace)); } - private String getText(final int start, final int end) { - int currentOldDocumentOffset = 0; - int currentNewDocumentOffset = 0; - StringBuilder text = new StringBuilder(); - Iterator> iterator = myAffectedFragments.iterator(); - while (iterator.hasNext() && currentNewDocumentOffset < end) { - final Pair pair = iterator.next(); - final MutableTextRange range = pair.getFirst(); - final StringBuffer buffer = pair.getSecond(); - final int fragmentEndInNewDocument = range.getStartOffset() + buffer.length(); + private void updateFragments(int start, int end, @NotNull String replace) { + int docStart = psiToDocumentOffset(start); + int docEnd = psiToDocumentOffset(end); - if(range.getStartOffset() <= start && fragmentEndInNewDocument >= end){ - return buffer.substring(start - range.getStartOffset(), end - range.getStartOffset()); + TextRange startRange = findFragment(docStart); + TextRange endRange = findFragment(docEnd); + + myPsiText = myPsiText.delete(start, end).insert(start, replace); + + TextRange newFragment = new TextRange(startRange != null ? startRange.getStartOffset() : docStart, + endRange != null ? endRange.getEndOffset() : docEnd); + CharSequence newReplacement = myPsiText.subSequence(documentToPsiOffset(newFragment.getStartOffset(), false), + documentToPsiOffset(newFragment.getEndOffset(), true) + replace.length() - (end - start)); + + for (Iterator iterator = myAffectedFragments.keySet().iterator(); iterator.hasNext(); ) { + if (iterator.next().intersects(newFragment)) { + iterator.remove(); } - - if(range.getStartOffset() >= start){ - final int effectiveStart = Math.max(currentNewDocumentOffset, start); - text.append(myDocument.getCharsSequence(), - effectiveStart - currentNewDocumentOffset + currentOldDocumentOffset, - Math.min(range.getStartOffset(), end) - currentNewDocumentOffset + currentOldDocumentOffset); - if(end > range.getStartOffset()){ - text.append(buffer.substring(0, Math.min(end - range.getStartOffset(), buffer.length()))); - } - } - - currentOldDocumentOffset += range.getEndOffset() - currentNewDocumentOffset; - currentNewDocumentOffset = fragmentEndInNewDocument; } - - if(currentNewDocumentOffset < end){ - final int effectiveStart = Math.max(currentNewDocumentOffset, start); - text.append(myDocument.getCharsSequence(), - effectiveStart - currentNewDocumentOffset + currentOldDocumentOffset, - end- currentNewDocumentOffset + currentOldDocumentOffset); - } - - return text.toString(); + myAffectedFragments.put(newFragment, newReplacement); } - private Pair getFragmentByRange(int start, final int length) { - final StringBuffer fragmentBuffer = new StringBuffer(); - int end = start + length; + private TextRange findFragment(final int docOffset) { + return ContainerUtil.find(myAffectedFragments.keySet(), new Condition() { + @Override + public boolean value(TextRange range) { + return range.containsOffset(docOffset); + } + }); + } - // restoring buffer and remove all subfragments from the list - int documentOffset = 0; - int effectiveOffset = 0; - - Iterator> iterator = myAffectedFragments.iterator(); - while (iterator.hasNext() && effectiveOffset <= end) { - final Pair pair = iterator.next(); - final MutableTextRange range = pair.getFirst(); - final StringBuffer buffer = pair.getSecond(); - int effectiveFragmentEnd = range.getStartOffset() + buffer.length(); - - if(range.getStartOffset() <= start && effectiveFragmentEnd >= end) return pair; - - if(effectiveFragmentEnd >= start){ - final int effectiveStart = Math.max(effectiveOffset, start); - if(range.getStartOffset() > start){ - fragmentBuffer.append(myDocument.getCharsSequence(), - effectiveStart - effectiveOffset + documentOffset, - Math.min(range.getStartOffset(), end)- effectiveOffset + documentOffset); - } - if(end >= range.getStartOffset()){ - fragmentBuffer.append(buffer); - end = end > effectiveFragmentEnd ? end - (buffer.length() - range.getLength()) : range.getEndOffset(); - effectiveFragmentEnd = range.getEndOffset(); - start = Math.min(start, range.getStartOffset()); - iterator.remove(); - } + private int psiToDocumentOffset(int offset) { + for (Map.Entry entry : myAffectedFragments.entrySet()) { + int lengthAfter = entry.getValue().length(); + TextRange range = entry.getKey(); + if (range.getStartOffset() + lengthAfter < offset) { + offset += range.getLength() - lengthAfter; + continue; } - documentOffset += range.getEndOffset() - effectiveOffset; - effectiveOffset = effectiveFragmentEnd; + // for offsets inside replaced ranges, return the starts of the original affected fragments in document + return Math.min(range.getStartOffset(), offset); } + return offset; + } - if(effectiveOffset < end){ - final int effectiveStart = Math.max(effectiveOffset, start); - fragmentBuffer.append(myDocument.getCharsSequence(), - effectiveStart - effectiveOffset + documentOffset, - end- effectiveOffset + documentOffset); + private int documentToPsiOffset(int offset, boolean greedyRight) { + int delta = 0; + for (Map.Entry entry : myAffectedFragments.entrySet()) { + int lengthAfter = entry.getValue().length(); + TextRange range = entry.getKey(); + // for offsets inside affected fragments, return either start or end of the updated range + if (range.containsOffset(offset)) { + return range.getStartOffset() + delta + (greedyRight ? lengthAfter : 0); + } + if (range.getStartOffset() > offset) { + break; + } + delta += lengthAfter - range.getLength(); } - - MutableTextRange newRange = new MutableTextRange(start, end); - final Pair pair = Pair.create(newRange, fragmentBuffer); - for (Pair affectedFragment : myAffectedFragments) { - MutableTextRange range = affectedFragment.getFirst(); - assert end <= range.getStartOffset() || range.getEndOffset() <= start : "Range :"+range+"; Added: "+newRange; - } - myAffectedFragments.add(pair); - return pair; + return offset + delta; } } - public static class MutableTextRange { - private final int myLength; - private int myStartOffset; - - public MutableTextRange(final int startOffset, final int endOffset) { - myStartOffset = startOffset; - myLength = endOffset - startOffset; - } - - public int getStartOffset() { - return myStartOffset; - } - - public int getEndOffset() { - return myStartOffset + myLength; - } - - public int getLength() { - return myLength; - } - - public String toString() { - return "[" + getStartOffset() + ", " + getEndOffset() + "]"; - } - - public void shift(final int lengthDiff) { - myStartOffset += lengthDiff; - } - } } diff --git a/platform/platform-tests/testSrc/com/intellij/openapi/editor/impl/RangeMarkerTest.java b/platform/platform-tests/testSrc/com/intellij/openapi/editor/impl/RangeMarkerTest.java index 885865143033..40b50616c332 100644 --- a/platform/platform-tests/testSrc/com/intellij/openapi/editor/impl/RangeMarkerTest.java +++ b/platform/platform-tests/testSrc/com/intellij/openapi/editor/impl/RangeMarkerTest.java @@ -420,6 +420,25 @@ public class RangeMarkerTest extends LightPlatformTestCase { assertValidMarker(marker, 3, 6); } + public void testPsi2DocTwoReplacements() { + RangeMarker marker = createMarker("fooFooFoo fooFooFoo", 10, 19); + synchronizer.startTransaction(getProject(), document, psiFile); + synchronizer.replaceString(document, 0, 9, "xxx"); + synchronizer.replaceString(document, 4, 13, "xxx"); + synchronizer.commitTransaction(document); + assertValidMarker(marker, 4, 7); + } + + public void testPsi2DocThreeOverlappingReplacements() { + createMarker("abc", 0, 0); + synchronizer.startTransaction(getProject(), document, psiFile); + synchronizer.replaceString(document, 0, 1, "xy"); + synchronizer.replaceString(document, 3, 4, "yz"); + synchronizer.replaceString(document, 0, 5, "xxx"); + synchronizer.commitTransaction(document); + assertEquals("xxx", document.getText()); + } + public void testPsi2DocMergeReplaceAfterAdd() throws Exception { StringBuilder buffer = new StringBuilder("0123456789"); RangeMarker marker = createMarker(buffer.toString(), 2, 5); @@ -434,8 +453,7 @@ public class RangeMarkerTest extends LightPlatformTestCase { synchronizer.replaceString(document, 3, 5, "bb"); buffer.replace(3, 5, "bb"); final PsiToDocumentSynchronizer.DocumentChangeTransaction transaction = synchronizer.getTransaction(document); - final Set> affectedFragments = transaction.getAffectedFragments(); - assertEquals(affectedFragments.size(), 2); + assertSize(2, transaction.getAffectedFragments().keySet()); synchronizer.commitTransaction(document); @@ -457,8 +475,7 @@ public class RangeMarkerTest extends LightPlatformTestCase { buffer.insert(i, "" + i); } final PsiToDocumentSynchronizer.DocumentChangeTransaction transaction = synchronizer.getTransaction(document); - final Set> affectedFragments = transaction.getAffectedFragments(); - assertEquals(1, affectedFragments.size()); + assertSize(1, transaction.getAffectedFragments().keySet()); synchronizer.commitTransaction(document); @@ -473,19 +490,18 @@ public class RangeMarkerTest extends LightPlatformTestCase { RangeMarker marker = createMarker(buffer.toString(), 2, 5); synchronizer.startTransaction(getProject(), document, psiFile); final PsiToDocumentSynchronizer.DocumentChangeTransaction transaction = synchronizer.getTransaction(document); - final Set> affectedFragments = transaction.getAffectedFragments(); - + assertNotNull(transaction); for (int i = 0; i < 10; i++) { synchronizer.insertString(document, i, "" + i); buffer.insert(i, "" + i); } - assertEquals(1, affectedFragments.size()); + assertSize(1, transaction.getAffectedFragments().keySet()); synchronizer.replaceString(document, 0, 20, "0123456789"); buffer.replace(0, 20, "0123456789"); - assertEquals(1, affectedFragments.size()); + assertSize(1, transaction.getAffectedFragments().keySet()); synchronizer.commitTransaction(document); @@ -509,8 +525,7 @@ public class RangeMarkerTest extends LightPlatformTestCase { buffer.insert(7, "d"); final PsiToDocumentSynchronizer.DocumentChangeTransaction transaction = synchronizer.getTransaction(document); - final Set> affectedFragments = transaction.getAffectedFragments(); - assertEquals(3, affectedFragments.size()); + assertSize(3, transaction.getAffectedFragments().keySet()); synchronizer.commitTransaction(document); diff --git a/xml/tests/src/com/intellij/codeInsight/XmlTagTest.java b/xml/tests/src/com/intellij/codeInsight/XmlTagTest.java index 4d1c166badf4..d263c5d44f5c 100644 --- a/xml/tests/src/com/intellij/codeInsight/XmlTagTest.java +++ b/xml/tests/src/com/intellij/codeInsight/XmlTagTest.java @@ -15,6 +15,7 @@ */ package com.intellij.codeInsight; +import com.intellij.ide.highlighter.XmlFileType; import com.intellij.openapi.application.ApplicationManager; import com.intellij.openapi.application.Result; import com.intellij.openapi.command.CommandProcessor; @@ -1006,4 +1007,18 @@ public class XmlTagTest extends LightCodeInsightTestCase { } }); } + + public void testSetName() { + XmlFile file = (XmlFile)PsiFileFactory.getInstance(getProject()).createFileFromText("dummy.xml", XmlFileType.INSTANCE, "1", 0, true); + final XmlTag tag = file.getDocument().getRootTag(); + final Document document = file.getViewProvider().getDocument(); + ApplicationManager.getApplication().runWriteAction(new Runnable() { + @Override + public void run() { + tag.setName("xxx"); + assertEquals("1", tag.getText()); + assertEquals("1", document.getText()); + } + }); + } }