OSDN Git Service

XML Comment Formatting Fixes
authorTor Norbye <tnorbye@google.com>
Mon, 3 Oct 2011 16:15:23 +0000 (09:15 -0700)
committerTor Norbye <tnorbye@google.com>
Mon, 3 Oct 2011 21:19:02 +0000 (14:19 -0700)
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
   <!-- First
   Second -->
into
   <!--
   First
   Second
   -->
instead of
   <!--
        First
   Second
   -->
as the code did up until now. (The current handling was there to make
    <!-- This is
         a comment -->
format into
    <!--
         This is
         a comment
    -->
)

Unit tests.

Change-Id: Id98faadf3731b82880b37ff852c7c0787bb196f8

eclipse/plugins/com.android.ide.eclipse.adt/src/com/android/ide/eclipse/adt/AdtUtils.java
eclipse/plugins/com.android.ide.eclipse.adt/src/com/android/ide/eclipse/adt/internal/editors/AndroidXmlAutoEditStrategy.java
eclipse/plugins/com.android.ide.eclipse.adt/src/com/android/ide/eclipse/adt/internal/editors/formatting/XmlPrettyPrinter.java
eclipse/plugins/com.android.ide.eclipse.tests/src/com/android/ide/eclipse/adt/internal/editors/formatting/XmlPrettyPrinterTest.java
eclipse/plugins/com.android.ide.eclipse.tests/unittests/com/android/ide/eclipse/adt/AdtUtilsTest.java

index ba6712b..17090fd 100644 (file)
@@ -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
index 0ca03d1..b5cfad3 100644 (file)
@@ -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();
     }
index b1592d4..a884532 100644 (file)
@@ -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 <!-- \n\n--> - 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:
+                        //     <!-- This is the
+                        //     comment block -->
+                        // such that it doesn't turn it into
+                        //     <!--
+                        //          This is the
+                        //     comment block
+                        //     -->
+                        // In this case we instead want
+                        //     <!--
+                        //     This is the
+                        //     comment block
+                        //     -->
+                        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());
index 0d6c19d..7436b2f 100644 (file)
@@ -446,7 +446,7 @@ public class XmlPrettyPrinterTest extends TestCase {
                 "\n" +
                 "    <!--\n" +
                 "         Deprecated strings - Move the identifiers to this section, mark as DO NOT TRANSLATE,\n" +
-                "         and remove the actual text.  These will be removed in a bulk operation\n" +
+                "         and remove the actual text.  These will be removed in a bulk operation.\n" +
                 "    -->\n" +
                 "    <!-- Do Not Translate.  Unused string. -->\n" +
                 "    <string name=\"meeting_invitation\"></string>\n" +
@@ -490,4 +490,47 @@ public class XmlPrettyPrinterTest extends TestCase {
                 "</resources>");
     }
 
+    public void testCommentHandling() throws Exception {
+        checkFormat(
+                XmlFormatPreferences.create(), XmlFormatStyle.LAYOUT,
+                "<foo >\n" +
+                "\n" +
+                "    <!-- abc\n" +
+                "         def\n" +
+                "         ghi -->\n" +
+                "\n" +
+                "    <!-- abc\n" +
+                "    def\n" +
+                "    ghi -->\n" +
+                "    \n" +
+                "<!-- abc\n" +
+                "def\n" +
+                "ghi -->\n" +
+                "\n" +
+                "</foo>",
+
+                "<foo >\n" +
+                "\n" +
+                "    <!--\n" +
+                "         abc\n" +
+                "         def\n" +
+                "         ghi\n" +
+                "    -->\n" +
+                "\n" +
+                "\n" +
+                "    <!--\n" +
+                "    abc\n" +
+                "    def\n" +
+                "    ghi\n" +
+                "    -->\n" +
+                "\n" +
+                "\n" +
+                "    <!--\n" +
+                "abc\n" +
+                "def\n" +
+                "ghi\n" +
+                "    -->\n" +
+                "\n" +
+                "</foo>");
+    }
 }
index 1d203b0..65b374b 100644 (file)
@@ -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"));