The current implementation avoids creating lines with leading whitespace (when words are + * separated by at most one whitespace character), allows lines to be broken after hyphens, and, + * if {@code allowWhitespaceBeyondEnd} is true, allows one whitespace character to extend beyond + * the preferred break width to make use of all available horizontal space. Very long + * unbreakable words may extend beyond the preferred break offset regardless of the setting of + * {@code allowWhitespaceBeyondEnd}. + * + *
It was previously considered to allow an arbitrary number of whitespace characters to + * trail off the end of each wrap line, rather than just one. In the end, it turned out to be + * better to limit this to just one character, as this conveniently avoids the need to ever + * position the visual text caret outside the word-wrapped editor viewport (except in cases of + * very long unbreakable words). + * + * @param paragraph a long line of text to be broken, i.e. a paragraph, or the remainder of a + * paragraph if some of its initial lines of wrapped text have already been laid out + * @param preferredMaximumBreakOffset the preferred maximum break offset + * @param allowWhitespaceBeyondEnd if true, allow one whitespace character to extend beyond + * {@code preferredMaximumBreakOffset} even when this could be avoided by choosing a + * smaller break offset + */ + static int adjustBreakOffsetToWord(CharSequence paragraph, + final int preferredMaximumBreakOffset, boolean allowWhitespaceBeyondEnd) + { + if (preferredMaximumBreakOffset < 0) { + throw new IllegalArgumentException(); + } + if (preferredMaximumBreakOffset > paragraph.length()) { + throw new IllegalArgumentException(); + } + /* BreakIterator.getLineInstance already seems to have a cache; creating a new instance here + is just the cost of BreakIterator.clone(). So don't bother trying to cache the BreakIterator + here. */ + BreakIterator bi = BreakIterator.getLineInstance(Locale.US); + /* Use CharSequenceCharacterIterator to avoid copying the entire paragraph string every + time. */ + bi.setText(new CharSequenceCharacterIterator(paragraph)); + + int ret; + if (preferredMaximumBreakOffset == 0) { + // Skip forward to next boundary. + ret = 0; + } else if ( + allowWhitespaceBeyondEnd && preferredMaximumBreakOffset < paragraph.length() && + Character.isWhitespace(paragraph.charAt(preferredMaximumBreakOffset))) + { + // Allow one whitespace character to extend beyond the preferred break offset. + return preferredMaximumBreakOffset + 1; + } else { + // Skip backwards to previous boundary. + ret = bi.isBoundary(preferredMaximumBreakOffset) + ? preferredMaximumBreakOffset + : bi.preceding(preferredMaximumBreakOffset); + if (ret == BreakIterator.DONE) { + return preferredMaximumBreakOffset; + } + } + if (ret == 0) { + // Skip forward to next boundary (for words longer than the preferred break offset). + ret = preferredMaximumBreakOffset > 0 && bi.isBoundary(preferredMaximumBreakOffset) + ? preferredMaximumBreakOffset + : bi.following(preferredMaximumBreakOffset); + if (ret == BreakIterator.DONE) { + ret = preferredMaximumBreakOffset; + } + /* The line-based break iterator will include whitespace trailing a word as well. Strip + this off so we can apply our own policy here. */ + int retBeforeTrim = ret; + while (ret > preferredMaximumBreakOffset && + Character.isWhitespace(paragraph.charAt(ret - 1))) + { + ret--; + } + /* If allowWhitespaceBeyondEnd is true, allow at most one whitespace character to trail + the word at the end. */ + if ((allowWhitespaceBeyondEnd || ret == 0) && retBeforeTrim > ret) { + ret++; + } + } + return ret; + } } diff --git a/ide/editor.lib2/src/org/netbeans/modules/editor/lib2/view/WrapInfo.java b/ide/editor.lib2/src/org/netbeans/modules/editor/lib2/view/WrapInfo.java index 0446647998da..9e48bd56cac2 100644 --- a/ide/editor.lib2/src/org/netbeans/modules/editor/lib2/view/WrapInfo.java +++ b/ide/editor.lib2/src/org/netbeans/modules/editor/lib2/view/WrapInfo.java @@ -122,7 +122,7 @@ void paintWrapLines(ParagraphViewChildren children, ParagraphView pView, allocBounds.x += endPart.width; } // Paint wrap mark - if (i != lastWrapLineIndex) { // but not on last wrap line + if (lineContinuationTextLayout != null && i != lastWrapLineIndex) { // but not on last wrap line PaintState paintState = PaintState.save(g); try { ViewUtils.applyForegroundColor(g, null, textComponent); diff --git a/ide/editor.lib2/src/org/netbeans/modules/editor/lib2/view/WrapInfoUpdater.java b/ide/editor.lib2/src/org/netbeans/modules/editor/lib2/view/WrapInfoUpdater.java index 497873f69e9c..335d4382ee14 100644 --- a/ide/editor.lib2/src/org/netbeans/modules/editor/lib2/view/WrapInfoUpdater.java +++ b/ide/editor.lib2/src/org/netbeans/modules/editor/lib2/view/WrapInfoUpdater.java @@ -24,6 +24,7 @@ import java.util.List; import java.util.logging.Level; import java.util.logging.Logger; +import javax.swing.text.BadLocationException; import javax.swing.text.View; import org.netbeans.lib.editor.util.swing.DocumentUtilities; @@ -101,14 +102,16 @@ void initWrapInfo() { wrapTypeWords = (docView.op.getLineWrapType() == LineWrapType.WORD_BOUND); float visibleWidth = docView.op.getVisibleRect().width; TextLayout lineContinuationTextLayout = docView.op.getLineContinuationCharTextLayout(); + final float lineContTextLayoutAdvance = + lineContinuationTextLayout == null ? 0f : lineContinuationTextLayout.getAdvance(); // Make reasonable minimum width so that the number of visual lines does not double suddenly // when user would minimize the width too much. Also have enough space for line continuation mark - availableWidth = Math.max(visibleWidth - lineContinuationTextLayout.getAdvance(), + availableWidth = Math.max(visibleWidth - lineContTextLayoutAdvance, docView.op.getDefaultCharWidth() * 4); logMsgBuilder = LOG.isLoggable(Level.FINE) ? new StringBuilder(100) : null; if (logMsgBuilder != null) { logMsgBuilder.append("Building wrapLines: availWidth=").append(availableWidth); // NOI18N - logMsgBuilder.append(", lineContCharWidth=").append(lineContinuationTextLayout.getAdvance()); // NOI18N + logMsgBuilder.append(", lineContCharWidth=").append(lineContTextLayoutAdvance); // NOI18N logMsgBuilder.append("\n"); // NOI18N } try { @@ -131,7 +134,7 @@ void initWrapInfo() { WordInfo wordInfo = getWordInfo(viewOrPartStartOffset, wrapLineStartOffset); if (wordInfo != null) { // Attempt to break the view (at word boundary) so that it fits. - ViewSplit split = breakView(viewOrPart, false); + ViewSplit split = breakView(viewOrPart, true); if (split != null) { addPart(split.startPart); finishWrapLine(); @@ -176,18 +179,49 @@ void initWrapInfo() { } if (regularBreak) { - ViewSplit split = breakView(viewOrPart, false); + /* Use allowWider=true here, so that long words are allowed to extend beyond + the preferred wrap width. Turning it off would just give up on breaking + entirely for the rest of the paragraph, yielding an even longer physical + line. */ + ViewSplit split = breakView(viewOrPart, true); if (split != null) { addPart(split.startPart); viewOrPart = split.endPart; - finishWrapLine(); } else { // break failed if (!wrapLineNonEmpty) { addViewOrPart(viewOrPart); viewOrPart = fetchNextView(); } - finishWrapLine(); } + /* Keep the NewlineView that follows each paragraph together with the + paragraph's last wrap line. Otherwise, the NewlineView might wrap to the + next physical line if the last wrap line of the paragraph happens to be + exactly as long as the availableWidth. This would make the text caret, if + positioned at the end of a paragraph, end up being visually positioned on + the beginning of the next line instead of at the end of the current one. */ + if (viewOrPart != null && viewOrPart.view instanceof NewlineView) { + /* One exception: If the wrap line ends with a space, it's actually + better to allow the NewlineView, and thus the text caret, to wrap to the + next physical line, since this is where the user's next typed character + will end up. This also avoids the need to position the caret outside the + viewport in a few cases (due to the text wrapping policy of allowing + whitespace characters at the end of each wrap line to extend beyond the + preferred wrap width). */ + boolean wrapLineEndsWithSpace = false; + try { + int newlineOffset = viewOrPart.view.getStartOffset(); + wrapLineEndsWithSpace = newlineOffset > 0 && + Character.isWhitespace(viewOrPart.view.getDocument() + .getText(newlineOffset - 1, 1).charAt(0)); + } catch (BadLocationException e) { + // Ignore. + } + if (!wrapLineEndsWithSpace) { + addViewOrPart(viewOrPart); + viewOrPart = fetchNextView(); + } + } + finishWrapLine(); } } } while (childIndex < pView.getViewCount()); @@ -601,6 +635,10 @@ int wordStartOffset() { return wordStartOffset; } + @Override + public String toString() { + return "WordInfo(" + wordStartOffset() + ", " + wordEndOffset() + ")"; + } } diff --git a/ide/editor.lib2/test/unit/src/org/netbeans/modules/editor/lib2/view/AdjustBreakOffsetToWordTest.java b/ide/editor.lib2/test/unit/src/org/netbeans/modules/editor/lib2/view/AdjustBreakOffsetToWordTest.java new file mode 100644 index 000000000000..278753093155 --- /dev/null +++ b/ide/editor.lib2/test/unit/src/org/netbeans/modules/editor/lib2/view/AdjustBreakOffsetToWordTest.java @@ -0,0 +1,284 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.netbeans.modules.editor.lib2.view; + +import java.util.Objects; +import org.junit.Assert; +import org.junit.Test; + +/** + * Test for {@link HighlightsViewUtils#adjustBreakOffsetToWord(CharSequence,int,boolean)}. + */ +public class AdjustBreakOffsetToWordTest { + private void testOne(String original, String expected, boolean allowWhitespaceBeyondEnd) { + TextWithCaret originalTwC = TextWithCaret.fromEncoded(original); + TextWithCaret expectedTwC = TextWithCaret.fromEncoded(expected); + TextWithCaret actualTwC = new TextWithCaret(originalTwC.text, + HighlightsViewUtils.adjustBreakOffsetToWord(originalTwC.text, originalTwC.caret, + allowWhitespaceBeyondEnd)); + Assert.assertEquals(expectedTwC, actualTwC); + } + + private void testOne(String original, String expected) { + testOne(original, expected, false); + testOne(original, expected, true); + } + + private void testOne(String original, String + expectedDisallowWhitespaceBeyondEnd, String expectedAllowWhitespaceBeyondEnd) + { + testOne(original, expectedDisallowWhitespaceBeyondEnd, false); + testOne(original, expectedAllowWhitespaceBeyondEnd, true); + } + + @Test + public void testLongWord() { + // Forward-skipping is necessary. + testOne("|this is a test", "this| is a test", "this |is a test"); + testOne("t|his is a test", "this| is a test", "this |is a test"); + testOne("th|is is a test", "this| is a test", "this |is a test"); + testOne("thi|s is a test", "this| is a test", "this |is a test"); + testOne("this| is a test", "this| is a test", "this |is a test"); + } + + @Test + public void testBasic() { + // Forward-skipping is not necessary. + testOne("this |is a test", "this |is a test"); + testOne("this i|s a test", "this |is a test"); + /* If !allowWhitespaceBeyondEnd, break must backtrack to before "is" to avoid the new line + starting with a whitespace. */ + testOne("this is| a test", "this |is a test", "this is |a test"); + testOne("this is |a test", "this is |a test"); + testOne("this is a| test", "this is |a test", "this is a |test"); + testOne("this is a |test", "this is a |test"); + testOne("this is a t|est", "this is a |test"); + testOne("this is a te|st", "this is a |test"); + testOne("this is a tes|t", "this is a |test"); + testOne("this is a test|", "this is a test|"); + } + + @Test + public void testDashInFirstWord() { + testOne("|foo-test bar is", "foo-|test bar is"); + testOne("f|oo-test bar is", "foo-|test bar is"); + testOne("fo|o-test bar is", "foo-|test bar is"); + testOne("foo|-test bar is", "foo-|test bar is"); + testOne("foo-|test bar is", "foo-|test bar is"); + testOne("foo-t|est bar is", "foo-|test bar is"); + testOne("foo-te|st bar is", "foo-|test bar is"); + testOne("foo-tes|t bar is", "foo-|test bar is"); + testOne("foo-test| bar is", "foo-|test bar is", "foo-test |bar is"); + testOne("foo-test |bar is", "foo-test |bar is"); + testOne("foo-test b|ar is", "foo-test |bar is"); + } + + @Test + public void testDashInNonFirstWord() { + testOne("this is| foo-test bar", "this |is foo-test bar", "this is |foo-test bar"); + testOne("this is |foo-test bar", "this is |foo-test bar"); + testOne("this is f|oo-test bar", "this is |foo-test bar"); + testOne("this is fo|o-test bar", "this is |foo-test bar"); + testOne("this is foo|-test bar", "this is |foo-test bar"); + testOne("this is foo-|test bar", "this is foo-|test bar"); + testOne("this is foo-t|est bar", "this is foo-|test bar"); + testOne("this is foo-te|st bar", "this is foo-|test bar"); + testOne("this is foo-tes|t bar", "this is foo-|test bar"); + testOne("this is foo-test| bar", "this is foo-|test bar", "this is foo-test |bar"); + testOne("this is foo-test |bar", "this is foo-test |bar"); + testOne("this is foo-test b|ar", "this is foo-test |bar"); + } + + @Test + public void testJustWhitespace() { + testOne("|", "|"); + testOne("| ", " |"); + testOne(" |", " |"); + testOne("| ", " | "); + testOne(" | ", " | ", " |"); + testOne(" |", " |"); + testOne("| ", " | "); + testOne(" | ", " | ", " | "); + testOne(" | ", " | ", " |"); + testOne(" |", " |"); + } + + @Test + public void testTrailingNewline() { + // A newline character should just be treated like any other whitespace here. + testOne("|this is a test\n", "this| is a test\n", "this |is a test\n"); + testOne("t|his is a test\n", "this| is a test\n", "this |is a test\n"); + testOne("th|is is a test\n", "this| is a test\n", "this |is a test\n"); + testOne("thi|s is a test\n", "this| is a test\n", "this |is a test\n"); + testOne("this| is a test\n", "this| is a test\n", "this |is a test\n"); + testOne("this |is a test\n", "this |is a test\n"); + testOne("this i|s a test\n", "this |is a test\n"); + testOne("this is| a test\n", "this |is a test\n", "this is |a test\n"); + testOne("this is |a test\n", "this is |a test\n"); + testOne("this is a| test\n", "this is |a test\n", "this is a |test\n"); + testOne("this is a |test\n", "this is a |test\n"); + testOne("this is a t|est\n", "this is a |test\n"); + testOne("this is a te|st\n", "this is a |test\n"); + testOne("this is a tes|t\n", "this is a |test\n"); + testOne("this is a test|\n", "this is a |test\n", "this is a test\n|"); + testOne("this is a test\n|", "this is a test\n|"); + } + + @Test + public void testSpacesBetween() { + // Multiple whitespace characters between first and second word. + testOne("th|is is a test", "this| is a test", "this | is a test"); + testOne("thi|s is a test", "this| is a test", "this | is a test"); + testOne("this| is a test", "this| is a test", "this | is a test"); + testOne("this | is a test", "this | is a test", "this | is a test"); + testOne("this | is a test", "this | is a test", "this |is a test"); + testOne("this |is a test", "this |is a test"); + testOne("this i|s a test", "this |is a test"); + testOne("this is| a test", "this |is a test", "this is |a test"); + testOne("this is |a test", "this is |a test"); + // Multiple whitespace characters between words not including the first word. + testOne("this |is aaaa test", "this |is aaaa test"); + testOne("this i|s aaaa test", "this |is aaaa test"); + testOne("this is| aaaa test", "this |is aaaa test", "this is | aaaa test"); + testOne("this is | aaaa test", "this |is aaaa test", "this is | aaaa test"); + testOne("this is | aaaa test", "this |is aaaa test", "this is |aaaa test"); + testOne("this is |aaaa test", "this is |aaaa test"); + testOne("this is a|aaa test", "this is |aaaa test"); + testOne("this is aa|aa test", "this is |aaaa test"); + testOne("this is aaa|a test", "this is |aaaa test"); + testOne("this is aaaa| test", "this is |aaaa test", "this is aaaa |test"); + testOne("this is aaaa |test", "this is aaaa |test"); + } + + @Test + public void testTrailingSpaces() { + // One trailing whitespace character. + testOne("this is a test| ", "this is a |test ", "this is a test |"); + testOne("this is a test |", "this is a test |"); + // Two trailing whitespace characters. + testOne("this is a tes|t ", "this is a |test ", "this is a |test "); + testOne("this is a test| ", "this is a |test ", "this is a test | "); + testOne("this is a test | ", "this is a |test ", "this is a test |"); + testOne("this is a test |", "this is a test |", "this is a test |"); + // Long line with one trailing whitespace character. + testOne("|testtest ", "testtest| ", "testtest |"); + testOne("t|esttest ", "testtest| ", "testtest |"); + testOne("testte|st ", "testtest| ", "testtest |"); + testOne("testtes|t ", "testtest| ", "testtest |"); + testOne("testtest| ", "testtest| ", "testtest |"); + testOne("testtest |", "testtest |", "testtest |"); + // Long line with two trailing whitespace characters. + testOne("|testtest ", "testtest| ", "testtest | "); + testOne("t|esttest ", "testtest| ", "testtest | "); + testOne("testte|st ", "testtest| ", "testtest | "); + testOne("testtes|t ", "testtest| ", "testtest | "); + testOne("testtest| ", "testtest| ", "testtest | "); + testOne("testtest | ", "testtest | ", "testtest |"); + testOne("testtest |", "testtest |"); + } + + @Test + public void testAdjustBreakOffsetToWordLeadingSpace() { + // One leading space + testOne("| this is a test", " |this is a test"); + testOne(" |this is a test", " |this is a test"); + testOne(" t|his is a test", " |this is a test"); + testOne(" th|is is a test", " |this is a test"); + testOne(" thi|s is a test", " |this is a test"); + testOne(" this| is a test", " |this is a test", " this |is a test"); + testOne(" this |is a test", " this |is a test"); + testOne(" this i|s a test", " this |is a test"); + // Two leading spaces + testOne("| this is a test", " | this is a test"); + testOne(" | this is a test", " | this is a test", " |this is a test"); + testOne(" |this is a test", " |this is a test"); + testOne(" t|his is a test", " |this is a test"); + testOne(" th|is is a test", " |this is a test"); + testOne(" thi|s is a test", " |this is a test"); + testOne(" this| is a test", " |this is a test", " this |is a test"); + testOne(" this |is a test", " this |is a test"); + testOne(" this i|s a test", " this |is a test"); + } + + private static final class TextWithCaret { + public final String text; + public final int caret; + + public TextWithCaret(String text, int caret) { + this(text, caret, true); + } + + private TextWithCaret(String text, int caret, boolean check) { + if (text == null) { + throw new NullPointerException(); + } + if (caret < 0 || caret > text.length()) { + throw new IllegalArgumentException(); + } + this.text = text; + this.caret = caret; + + if (check && !equals(fromEncoded(toString()))) { + throw new AssertionError(); + } + } + + /** + * @param encoded string containing a single '|' character indicating the caret position + */ + public static TextWithCaret fromEncoded(String encoded) { + final StringBuilder psb = new StringBuilder(); + Integer caret = null; + for (char ec : encoded.toCharArray()) { + if (ec == '|') { + if (caret != null) { + throw new IllegalArgumentException("Multiple caret positions specified"); + } + caret = psb.length(); + } else { + psb.append(ec); + } + } + if (caret == null) { + throw new IllegalArgumentException("No caret position specified"); + } + return new TextWithCaret(psb.toString(), caret, false); + } + + @Override + public String toString() { + return text.substring(0, caret) + '|' + text.substring(caret); + } + + @Override + public boolean equals(Object obj) { + if (!(obj instanceof TextWithCaret)) { + return false; + } + TextWithCaret other = (TextWithCaret) obj; + return this.text.equals(other.text) + && this.caret == other.caret; + } + + @Override + public int hashCode() { + return Objects.hash(text, caret); + } + } +}