From 2c07fc348d85836bff4d1f9ca656d96898a0833c Mon Sep 17 00:00:00 2001 From: Tor Norbye Date: Mon, 3 Oct 2011 09:15:23 -0700 Subject: [PATCH] XML Comment Formatting Fixes. DO NOT MERGE. This changeset fixes issue 20452 related to XML comment handling. There are several changes. First it fixes a truncation bug where the last character in a comment could get erased. Second it makes sure we don't end up with double blank lines, since there were cases where the comment formatter would insert a newline (to preserve newlines after comments in the original document) and a subsequent element or close tag would also insert a newline (which is where the normal blank lines are added). Now the code will peek at the write buffer to make sure we haven't already added a blank line. Third, it attempts to handle multiline comments a bit better such that the code which preserves indentation of the first comment line will dedent down to the minimum indentation level of the block comment. This means that we will format into instead of as the code did up until now. (The current handling was there to make format into ) Unit tests. Change-Id: I7201b55a2c824c3c0b43287e853bc9b75d74ed7f --- .../src/com/android/ide/eclipse/adt/AdtUtils.java | 35 +++++++++ .../editors/AndroidXmlAutoEditStrategy.java | 2 +- .../editors/formatting/XmlPrettyPrinter.java | 90 +++++++++++++++++++++- .../editors/formatting/XmlPrettyPrinterTest.java | 45 ++++++++++- .../com/android/ide/eclipse/adt/AdtUtilsTest.java | 23 ++++++ 5 files changed, 189 insertions(+), 6 deletions(-) diff --git a/eclipse/plugins/com.android.ide.eclipse.adt/src/com/android/ide/eclipse/adt/AdtUtils.java b/eclipse/plugins/com.android.ide.eclipse.adt/src/com/android/ide/eclipse/adt/AdtUtils.java index ba6712b4c..17090fd3a 100644 --- a/eclipse/plugins/com.android.ide.eclipse.adt/src/com/android/ide/eclipse/adt/AdtUtils.java +++ b/eclipse/plugins/com.android.ide.eclipse.adt/src/com/android/ide/eclipse/adt/AdtUtils.java @@ -46,6 +46,41 @@ public class AdtUtils { } /** + * Returns true if the given sequence ends with the given suffix (case + * sensitive). + * + * @param sequence the character sequence to be checked + * @param suffix the suffix to look for + * @return true if the given sequence ends with the given suffix + */ + public static boolean endsWith(CharSequence sequence, CharSequence suffix) { + return endsWith(sequence, sequence.length(), suffix); + } + + /** + * Returns true if the given sequence ends at the given offset with the given suffix (case + * sensitive) + * + * @param sequence the character sequence to be checked + * @param endOffset the offset at which the sequence is considered to end + * @param suffix the suffix to look for + * @return true if the given sequence ends with the given suffix + */ + public static boolean endsWith(CharSequence sequence, int endOffset, CharSequence suffix) { + if (endOffset < suffix.length()) { + return false; + } + + for (int i = endOffset - 1, j = suffix.length() - 1; j >= 0; i--, j--) { + if (sequence.charAt(i) != suffix.charAt(j)) { + return false; + } + } + + return true; + } + + /** * Strips the whitespace from the given string * * @param string the string to be cleaned up diff --git a/eclipse/plugins/com.android.ide.eclipse.adt/src/com/android/ide/eclipse/adt/internal/editors/AndroidXmlAutoEditStrategy.java b/eclipse/plugins/com.android.ide.eclipse.adt/src/com/android/ide/eclipse/adt/internal/editors/AndroidXmlAutoEditStrategy.java index 0ca03d105..b5cfad302 100644 --- a/eclipse/plugins/com.android.ide.eclipse.adt/src/com/android/ide/eclipse/adt/internal/editors/AndroidXmlAutoEditStrategy.java +++ b/eclipse/plugins/com.android.ide.eclipse.adt/src/com/android/ide/eclipse/adt/internal/editors/AndroidXmlAutoEditStrategy.java @@ -281,7 +281,7 @@ public class AndroidXmlAutoEditStrategy implements IAutoEditStrategy { * @throws BadLocationException if the offset is invalid */ public static int findLineStart(IDocument document, int offset) throws BadLocationException { - offset = Math.min(offset, document.getLength() - 1); + offset = Math.max(0, Math.min(offset, document.getLength() - 1)); IRegion info = document.getLineInformationOfOffset(offset); return info.getOffset(); } diff --git a/eclipse/plugins/com.android.ide.eclipse.adt/src/com/android/ide/eclipse/adt/internal/editors/formatting/XmlPrettyPrinter.java b/eclipse/plugins/com.android.ide.eclipse.adt/src/com/android/ide/eclipse/adt/internal/editors/formatting/XmlPrettyPrinter.java index b1592d488..a88453249 100644 --- a/eclipse/plugins/com.android.ide.eclipse.adt/src/com/android/ide/eclipse/adt/internal/editors/formatting/XmlPrettyPrinter.java +++ b/eclipse/plugins/com.android.ide.eclipse.adt/src/com/android/ide/eclipse/adt/internal/editors/formatting/XmlPrettyPrinter.java @@ -22,6 +22,7 @@ import static com.android.ide.eclipse.adt.internal.editors.resources.descriptors import static com.android.ide.eclipse.adt.internal.editors.resources.descriptors.ResourcesDescriptors.STRING_ELEMENT; import static com.android.ide.eclipse.adt.internal.editors.resources.descriptors.ResourcesDescriptors.STYLE_ELEMENT; +import com.android.ide.eclipse.adt.AdtUtils; import com.android.ide.eclipse.adt.internal.editors.layout.gle2.DomUtilities; import org.eclipse.wst.xml.core.internal.document.DocumentTypeImpl; @@ -400,7 +401,7 @@ public class XmlPrettyPrinter { index--; } - end = recentNewline == -1 ? index : recentNewline; + end = recentNewline == -1 ? index + 1 : recentNewline; if (start >= end) { // It's a blank comment like - just clean it up if (!isSuffixComment) { @@ -450,21 +451,85 @@ public class XmlPrettyPrinter { if (!startsWithNewline) { Node previous = node.getPreviousSibling(); if (previous != null && previous.getNodeType() == Node.TEXT_NODE) { + int TAB_SIZE = 4; // TODO: Look up Eclipse settings String prevText = previous.getNodeValue(); int indentation = COMMENT_BEGIN.length(); for (int i = prevText.length() - 1; i >= 0; i--) { char c = prevText.charAt(i); if (c == '\n') { break; - } else if (c == '\t') { - indentation += 4; // TODO: Look up Eclipse settings } else { - indentation++; + indentation += (c == '\t') ? TAB_SIZE : 1; } } + + // See if the next line after the newline has indentation; if it doesn't, + // leave things alone. This fixes a case like this: + // + // such that it doesn't turn it into + // + // In this case we instead want + // + int minIndent = Integer.MAX_VALUE; + String[] lines = trimmed.split("\n"); //$NON-NLS-1$ + // Skip line 0 since we know that it doesn't start with a newline + for (int i = 1; i < lines.length; i++) { + int indent = 0; + String line = lines[i]; + for (int j = 0; j < line.length(); j++) { + char c = line.charAt(j); + if (!Character.isWhitespace(c)) { + // Only set minIndent if there's text content on the line; + // blank lines can exist in the comment without affecting + // the overall minimum indentation boundary. + if (indent < minIndent) { + minIndent = indent; + } + break; + } else { + indent += (c == '\t') ? TAB_SIZE : 1; + } + } + } + + if (minIndent < indentation) { + indentation = minIndent; + + // Subtract any indentation that is already present on the line + String line = lines[0]; + for (int j = 0; j < line.length(); j++) { + char c = line.charAt(j); + if (!Character.isWhitespace(c)) { + break; + } else { + indentation -= (c == '\t') ? TAB_SIZE : 1; + } + } + } + for (int i = 0; i < indentation; i++) { mOut.append(' '); } + + if (indentation < 0) { + boolean prefixIsSpace = true; + for (int i = 0; i < -indentation && i < trimmed.length(); i++) { + if (!Character.isWhitespace(trimmed.charAt(i))) { + prefixIsSpace = false; + break; + } + } + if (prefixIsSpace) { + trimmed = trimmed.substring(-indentation); + } + } } } @@ -706,15 +771,32 @@ public class XmlPrettyPrinter { } private boolean newlineAfterElementOpen(Element element, int depth, boolean isClosed) { + if (hasBlankLineAbove()) { + return false; + } + // In resource files we keep the child content directly on the same // line as the element (unless it has children). in other files, separate them return isClosed || !keepElementAsSingleLine(depth, element); } private boolean newlineBeforeElementClose(Element element, int depth) { + if (hasBlankLineAbove()) { + return false; + } + return depth == 0 && !mPrefs.removeEmptyLines; } + private boolean hasBlankLineAbove() { + if (mOut.length() < 2 * mLineSeparator.length()) { + return false; + } + + return AdtUtils.endsWith(mOut, mLineSeparator) && + AdtUtils.endsWith(mOut, mOut.length() - mLineSeparator.length(), mLineSeparator); + } + private boolean newlineAfterElementClose(Element element, int depth) { return element.getParentNode().getNodeType() == Node.ELEMENT_NODE && !keepElementAsSingleLine(depth - 1, (Element) element.getParentNode()); diff --git a/eclipse/plugins/com.android.ide.eclipse.tests/src/com/android/ide/eclipse/adt/internal/editors/formatting/XmlPrettyPrinterTest.java b/eclipse/plugins/com.android.ide.eclipse.tests/src/com/android/ide/eclipse/adt/internal/editors/formatting/XmlPrettyPrinterTest.java index 0d6c19d31..7436b2ff1 100644 --- a/eclipse/plugins/com.android.ide.eclipse.tests/src/com/android/ide/eclipse/adt/internal/editors/formatting/XmlPrettyPrinterTest.java +++ b/eclipse/plugins/com.android.ide.eclipse.tests/src/com/android/ide/eclipse/adt/internal/editors/formatting/XmlPrettyPrinterTest.java @@ -446,7 +446,7 @@ public class XmlPrettyPrinterTest extends TestCase { "\n" + " \n" + " \n" + " \n" + @@ -490,4 +490,47 @@ public class XmlPrettyPrinterTest extends TestCase { ""); } + public void testCommentHandling() throws Exception { + checkFormat( + XmlFormatPreferences.create(), XmlFormatStyle.LAYOUT, + "\n" + + "\n" + + " \n" + + "\n" + + " \n" + + " \n" + + "\n" + + "\n" + + "", + + "\n" + + "\n" + + " \n" + + "\n" + + "\n" + + " \n" + + "\n" + + "\n" + + " \n" + + "\n" + + ""); + } } diff --git a/eclipse/plugins/com.android.ide.eclipse.tests/unittests/com/android/ide/eclipse/adt/AdtUtilsTest.java b/eclipse/plugins/com.android.ide.eclipse.tests/unittests/com/android/ide/eclipse/adt/AdtUtilsTest.java index 1d203b04f..65b374b4f 100644 --- a/eclipse/plugins/com.android.ide.eclipse.tests/unittests/com/android/ide/eclipse/adt/AdtUtilsTest.java +++ b/eclipse/plugins/com.android.ide.eclipse.tests/unittests/com/android/ide/eclipse/adt/AdtUtilsTest.java @@ -31,6 +31,29 @@ public class AdtUtilsTest extends TestCase { assertFalse(AdtUtils.endsWithIgnoreCase("foo", "fo")); } + public void testEndsWith() { + assertTrue(AdtUtils.endsWith("foo", "foo")); + assertTrue(AdtUtils.endsWith("foobar", "obar")); + assertTrue(AdtUtils.endsWith("foobar", "bar")); + assertTrue(AdtUtils.endsWith("foobar", "ar")); + assertTrue(AdtUtils.endsWith("foobar", "r")); + assertTrue(AdtUtils.endsWith("foobar", "")); + + assertTrue(AdtUtils.endsWith(new StringBuilder("foobar"), "bar")); + assertTrue(AdtUtils.endsWith(new StringBuilder("foobar"), new StringBuffer("obar"))); + assertTrue(AdtUtils.endsWith("foobar", new StringBuffer("obar"))); + + assertFalse(AdtUtils.endsWith("foo", "fo")); + assertFalse(AdtUtils.endsWith("foobar", "Bar")); + assertFalse(AdtUtils.endsWith("foobar", "longfoobar")); + } + + public void testEndsWith2() { + assertTrue(AdtUtils.endsWith("foo", "foo".length(), "foo")); + assertTrue(AdtUtils.endsWith("foo", "fo".length(), "fo")); + assertTrue(AdtUtils.endsWith("foo", "f".length(), "f")); + } + public void testStripWhitespace() { assertEquals("foo", AdtUtils.stripWhitespace("foo")); assertEquals("foobar", AdtUtils.stripWhitespace("foo bar")); -- 2.11.0