Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

IllegalArgumentException when undo-ing then redo-ing #216

Closed
benedictleejh opened this issue Nov 27, 2015 · 25 comments
Closed

IllegalArgumentException when undo-ing then redo-ing #216

benedictleejh opened this issue Nov 27, 2015 · 25 comments
Labels

Comments

@benedictleejh
Copy link

Steps to reproduce:

  1. enter text "testtest"
  2. Highlight the first "test"
  3. style (Press the style button) the first "test"
  4. Highlight the second "test"
  5. style the second "test"
  6. put selection point between the two "test"
  7. type shift-space, space, space
  8. undo, redo

Repo with code: https://github.com/benedictleejh/richtextfx-error

@TomasMikula TomasMikula added the bug label Dec 4, 2015
@TomasMikula TomasMikula added this to the 0.7 milestone Dec 4, 2015
@TomasMikula
Copy link
Member

Thanks for reporting. This is an example of how stricter checking in UndoFX revealed a bug elsewhere.

A little analysis of the problem:

The problem comes down to the fact that an empty paragraph contains one text segment (StyledText) of length 0, but there is a style associated with that text segment nevertheless. The two change events, expected and received, differ in the style of that empty string, and are thus considered different, even though their effect is indistinguishable.

I think the right solution is to represent an empty Paragraph as having no text segments. Though there are probably places where we use the style of an empty segment, so we will have to find a way to not need that style.

@JordanMartinez
Copy link
Contributor

Here's the Java version of benedictleejh's code:

import javafx.application.Application;
import javafx.scene.Scene;
import javafx.scene.control.Button;
import javafx.scene.input.KeyEvent;
import javafx.scene.layout.BorderPane;
import javafx.stage.Stage;
import org.fxmisc.richtext.InlineStyleTextArea;

import java.util.function.Function;

public class BugTest extends Application {

    public static void main(String[] args) {
        launch(args);
    }

    class BoldUnderline {
        boolean styled;
        String style = "-fx-font-weight: bold; -fx-underline: true;";
        String toCss() { return (styled) ? style : ""; }

        BoldUnderline(boolean styled) {
            this.styled = styled;
        }
    }

    InlineStyleTextArea<BoldUnderline, String> richTextBox = new InlineStyleTextArea<>(
            new BoldUnderline(false), BoldUnderline::toCss,
            "", Function.identity()
            );
    {
        richTextBox.setPrefSize(400, 300);
        richTextBox.addEventFilter(KeyEvent.KEY_TYPED, (KeyEvent e) -> {
            if (e.isShiftDown() && e.getCharacter().equals(" ")) {
                richTextBox.setUseInitialStyleForInsertion(true);
                richTextBox.insertText(richTextBox.getCaretPosition(), " ");
                richTextBox.setUseInitialStyleForInsertion(false);

                // consume event to prevent double space insertion
                e.consume();
            }
        });
    }

    @Override
    public void start(Stage primaryStage) {
        Button styleBtn = new Button("Style");
        styleBtn.setOnAction((ae) -> {
            richTextBox.setStyle(
                    richTextBox.getSelection().getStart(),
                    richTextBox.getSelection().getEnd(),
                    new BoldUnderline(true)
            );
        });
        BorderPane root = new BorderPane();
        root.setTop(styleBtn);
        root.setCenter(richTextBox);
        primaryStage.setScene(new Scene(root));
        primaryStage.show();
    }
}

@JordanMartinez
Copy link
Contributor

The problem comes down to the fact that an empty paragraph contains one text segment (StyledText) of length 0, but there is a style associated with that text segment nevertheless. The two change events, expected and received, differ in the style of that empty string, and are thus considered different, even though their effect is indistinguishable.

I understand this part, but I don't understand how you get to this conclusion...

I think the right solution is to represent an empty Paragraph as having no text segments. Though there are probably places where we use the style of an empty segment, so we will have to find a way to not need that style.

.... since the problem seems to be with StyledText as the exception occurs when the area only has one line (and hence one Paragraph), not two. Shouldn't the Paragraph immediately discard the empty StyledText? Why the reference to an empty Paragraph?

Could you explain your reasoning a bit more? This seems to be the last bug before the 0.7 release.

@TomasMikula
Copy link
Member

Shouldn't the Paragraph immediately discard the empty StyledText? Why the reference to an empty Paragraph?

First, since there currently is the invariant that Paragraph always has at least one (possibly empty) StyledText, there is code that relies on it and does not have to check whether the Paragraph is empty, e.g. at Paragraph.java#L221, Paragraph.java#L246, and other places.

Secondly, the lines referred to above also return the style of the possibly empty text segment, which is used by EditableStyledDocument#getStyleForInsertionAt(), which in turn is used by EditableStyledDocument#replaceText().

@JordanMartinez
Copy link
Contributor

Secondly, the lines referred to above also return the style of the possibly empty text segment, which is used by EditableStyledDocument#getStyleForInsertionAt(), which in turn is used by EditableStyledDocument#replaceText().

So, on one hand, the EditableStyledDocument#getStyleForInsertionAt() needs to account for a possibly empty paragraph. Something like the following code....

S getStyleForInsertionAt(Position insertionPos) {
    if(useInitialStyleForInsertion.get()) {
        return initialStyle;
    } else {
        Paragraph<S, PS> par = paragraphs.get(insertionPos.getMajor());
        // return style if paragraph isn't empty
        if (par.length() != 0) {
            return par.getStyleAtPosition(insertionPos.getMinor());
        } else {
            // find the first non-empty previous paragraph
            // and get style of the last character on the line
            for(int i = insertionPos.getMajor() - 1; i >= 0; i--) {
                Paragraph<S, PS> paragraph = paragraphs.get(i);
                if (paragraph.length() != 0) {
                    return paragraph.getStyleAtPosition(paragraph.length());
                }
            }

            // if not returned by now, find the first non-empty next paragraph
            // and get style of first character on the line
            for(int i = insertionPos.getMajor() + 1; i <= paragraphs.size() - 1; i++) {
                Paragraph<S, PS> paragraph = paragraphs.get(i);
                if (paragraph.length() != 0) {
                    return par.getStyleAtPosition(0);
                }
            }
            // if no style found, default to initialStyle
            return initialStyle;
        }
    }
}

....because EditableStyledDocument is the only one with access to other Paragraphs whose styles can be used if the chosen Paragraph is empty.

First, since there currently is the invariant that Paragraph always has at least one (possibly empty) StyledText, there is code that relies on it and does not have to check whether the Paragraph is empty, e.g. at Paragraph.java#L221, Paragraph.java#L246, and other places.

If a Paragraph is empty, it has no access to the previous and next Paragraphs. Even if it had a reference to initialStyle, which it doesn't, using that style wouldn't maintain the previous or next lines' styles and might break the user's expectations. So, an empty Paragraph would need to delegate the Style returned from getStyleAtPosition() to EditableStyledDocument using the above code.

I don't know entirely how the Paragraph objects work in relation to their view, ParagraphBox, but one idea I want to propose (not knowing its full implications) is to create a new class EmptyParagraph. It and Paragraph could implement some interface. Then, EmptyParagraph and Paragraph could be switched out under different circumstances: the empty one to a normal one when text is inserted and the normal one to an empty one when all text on a line is deleted.

Perhaps a better implementation of this idea is to add some code that delegates the getStyleAtPosition method to EditableStyledDocument (by adding a Function argument to its Constructor) when its length is 0.

@TomasMikula
Copy link
Member

Paragraph cannot have a reference to EditableStyledDocument (directly or via a function argument), because the same paragraph may be present in multiple documents at the same time.

Having two implementations of Paragraph, empty and non-empty, might be a good idea. Maybe if the EmptyParagraph had a textStyle field that would be returned from its getStyleAtPosition(0) method, it would solve the problem of what to return from getStyleAtPosition.

@JordanMartinez
Copy link
Contributor

Paragraph cannot have a reference to EditableStyledDocument (directly or via a function argument), because the same paragraph may be present in multiple documents at the same time.

Like when something needs to be replaced/inserted/deleted via ReadOnlyStyledDocument? Good point...

Having two implementations of Paragraph, empty and non-empty, might be a good idea. Maybe if the EmptyParagraph had a textStyle field that would be returned from its getStyleAtPosition(0) method, it would solve the problem of what to return from getStyleAtPosition.

Since we're dealing with a blank line, the Style object returned really doesn't matter (in terms of changing how the line is visually displayed). It only needs to satisfy a code requirement, correct? In that case, all that needs to be returned is a blank, empty, or default version of that Style object.
When text is inserted/deleted/replaced, etc., it can use the code I wrote above in EditableStyledDocument to handle the proper style insertion, right?

If so, it seems like the Style object should have a static method such as toEmptyLineCss() or a constant such as EMPTY_LINE_STYLE which the EmptyParagraph could return as a style in getStyleAtPosition(0).

@JordanMartinez
Copy link
Contributor

If going that route, I think some interface should be created that all style-related objects must implement.

@benedictleejh
Copy link
Author

I'd like to suggest that the empty paragraph should return the default style for the text area. I've run into an issue where if all the text in the text area was styled, deleting all the text and then trying to insert new text would cause the new text to inherit the deleted text's style.

I haven't filed an issue for that since @TomasMikula's explanation of this bug implies that if this was properly fixed, that would get fixed as a side effect. If you do prefer, I could certainly file an issue for that however.

@JordanMartinez
Copy link
Contributor

After testing out my idea, I'm running into a couple of issues.:

Renaming Paragraph to NormalParagraph, creating EmptyParagraph, and creating an interface Paragraph<S, PS> that NormalParagraph and EmptyParagraph implement....

public interface Paragraph<S, PS> extends CharSequence {

    public List<StyledText<S>> getSegments();
    public PS getParagraphStyle();

    public String substring(int from, int to);
    public String substring(int from);
    public Paragraph<S, PS> concat(Paragraph<S, PS> p);
    public Paragraph<S, PS> concat(CharSequence str);
    public Paragraph<S, PS> insert(int offset, CharSequence str);
    public Paragraph<S, PS> subSequence(int start, int end);
    public Paragraph<S, PS> trim(int length);
    public Paragraph<S, PS> subSequence(int start);
    public Paragraph<S, PS> delete(int start, int end);

    public Paragraph<S, PS> restyle(S style);
    public Paragraph<S, PS> restyle(int from, int to, S style);
    public Paragraph<S, PS> restyle(int from, StyleSpans<? extends S> styleSpans);

    public Paragraph<S, PS> setParagraphStyle(PS paragraphStyle);
    public S getStyleOfChar(int charIdx);
    public S getStyleAtPosition(int position);
    public IndexRange getStyleRangeAtPosition(int position);
    public StyleSpans<S> getStyleSpans();
    public StyleSpans<S> getStyleSpans(int from, int to);
}

...EmptyParagraph runs into a few problems:

  • What does EmptyParagraph return in overriding CharSequence's charAt() method?
  • substring() returns an empty string. Will that lead to any problems down the road (like when getting the length of the String)?
  • Methods with a CharSequence argument are problematic (concat(CharSequence str) and insert(CharSequence str)). These methods should create a NormalParagraph. However, what should that paragraph's text style object be? The reference to a blank style shouldn't be used, right?
  • getStyleRangeAtPosition(int position) would return an EMPTY_RANGE ([0, 0]). Will that be problematic elsewhere since it's length is 0?
  • What should be returned from getStyleSpans()? If null is returned, will that screw up code elsewhere?
  • NormalParagraph's hashCode() hashes its paragraphStyle and its List<StyledText<S>> segments, which means each paragraph will be unique. However, EmptyParagraph's hashCode() would only hash its paragraphStyle and the emptyTextStyle used for getStyleAtPosition(0). Will that cause a problem?

Full class code below:

public class EmptyParagraph<S, PS> implements Paragraph<S, PS> {
    public static final IndexRange EMPTY_RANGE = new IndexRange(0, 0);

    private final PS paragraphStyle;
    private final S emptyTextStyle;

    public EmptyParagraph(PS paragraphStyle, S textStyle) {
        this.paragraphStyle = paragraphStyle;
        this.emptyTextStyle = textStyle;
    }

    public List<StyledText<S>> getSegments() {
        return Collections.unmodifiableList(new ArrayList<StyledText<S>>(0));
    }

    public PS getParagraphStyle() {
        return paragraphStyle;
    }

    @Override
    public int length() {
        return 0;
    }

    public String substring(int from, int to) {
        return "";
    }

    public String substring(int from) {
        return "";
    }

    public Paragraph<S, PS> concat(Paragraph<S, PS> p) {
        return p;
    }

    public Paragraph<S, PS> concat(CharSequence str) {
        if(str.length() == 0) {
            return this;
        }

        List<StyledText<S>> segs = new ArrayList<>(1);
        segs.set(0, new StyledText<S>(
                str.toString(),
                // what should be the style here?
                emptyTextStyle));
        return new NormalParagraph<>(paragraphStyle, segs);
    }

    public Paragraph<S, PS> insert(int offset, CharSequence str) {
        List<StyledText<S>> segs = new ArrayList<>(1);
        segs.set(0, new StyledText<S>(
                str.toString(),
                // what should be the style here?
                emptyTextStyle));
        return new NormalParagraph<>(paragraphStyle, segs);
    }

    @Override
    public Paragraph<S, PS> subSequence(int start, int end) {
        return this;
//        return trim(end).subSequence(start);
    }

    public Paragraph<S, PS> trim(int length) {
        return this;

//        if(length >= length()) {
//            return this;
//        } else {
//            TwoDimensional.Position pos = navigator.offsetToPosition(length, Backward);
//            int segIdx = pos.getMajor();
//            List<StyledText<S>> segs = new ArrayList<>(segIdx + 1);
//            segs.addAll(segments.subList(0, segIdx));
//            segs.add(segments.get(segIdx).subSequence(0, pos.getMinor()));
//            return new NormalParagraph<>(paragraphStyle, segs);
//        }
    }

    public Paragraph<S, PS> subSequence(int start) {
        return this;

//        if(start < 0) {
//            throw new IllegalArgumentException("start must not be negative (was: " + start + ")");
//        } else if(start == 0) {
//            return this;
//        } else if(start <= length()) {
//            TwoDimensional.Position pos = navigator.offsetToPosition(start, Forward);
//            int segIdx = pos.getMajor();
//            List<StyledText<S>> segs = new ArrayList<>(segments.size() - segIdx);
//            segs.add(segments.get(segIdx).subSequence(pos.getMinor()));
//            segs.addAll(segments.subList(segIdx + 1, segments.size()));
//            return new NormalParagraph<>(paragraphStyle, segs);
//        } else {
//            throw new IndexOutOfBoundsException(start + " not in [0, " + length() + "]");
//        }
    }

    public Paragraph<S, PS> delete(int start, int end) {
        return this;

//        return trim(start).concat(subSequence(end));
    }

    public Paragraph<S, PS> restyle(S style) {
        return new EmptyParagraph<>(paragraphStyle, style);

//        return new NormalParagraph<>(paragraphStyle, toString(), style);
    }

    public Paragraph<S, PS> restyle(int from, int to, S style) {
        return restyle(style);

//        if(from >= length()) {
//            return this;
//        } else {
//            to = Math.min(to, length());
//            Paragraph<S, PS> left = subSequence(0, from);
//            Paragraph<S, PS> middle = new NormalParagraph<>(paragraphStyle, substring(from, to), style);
//            Paragraph<S, PS> right = subSequence(to);
//            return left.concat(middle).concat(right);
//        }
    }

    public Paragraph<S, PS> restyle(int from, StyleSpans<? extends S> styleSpans) {
        return this;

//        int len = styleSpans.length();
//        if(styleSpans.equals(getStyleSpans(from, from + len))) {
//            return this;
//        }
//
//        Paragraph<S, PS> left = trim(from);
//        Paragraph<S, PS> right = subSequence(from + len);
//
//        String middleString = substring(from, from + len);
//        List<StyledText<S>> middleSegs = new ArrayList<>(styleSpans.getSpanCount());
//        int offset = 0;
//        for(StyleSpan<? extends S> span: styleSpans) {
//            int end = offset + span.getLength();
//            String text = middleString.substring(offset, end);
//            middleSegs.add(new StyledText<>(text, span.getStyle()));
//            offset = end;
//        }
//        Paragraph<S, PS> middle = new NormalParagraph<>(paragraphStyle, middleSegs);
//
//        return left.concat(middle).concat(right);
    }

    public Paragraph<S, PS> setParagraphStyle(PS paragraphStyle) {
        return new EmptyParagraph<>(paragraphStyle, emptyTextStyle);

//        return new NormalParagraph<>(paragraphStyle, segments);
    }

    /**
     * Returns the style of character with the given index.
     * If {@code charIdx < 0}, returns the style at the beginning of this paragraph.
     * If {@code charIdx >= this.length()}, returns the style at the end of this paragraph.
     */
    public S getStyleOfChar(int charIdx) {
        return emptyTextStyle;

//        if(charIdx < 0) {
//            return segments.get(0).getStyle();
//        }
//
//        TwoDimensional.Position pos = navigator.offsetToPosition(charIdx, Forward);
//        return segments.get(pos.getMajor()).getStyle();
    }

    /**
     * Returns the style at the given position. That is the style of the
     * character immediately preceding {@code position}. If {@code position}
     * is 0, then the style of the first character (index 0) in this paragraph
     * is returned. If this paragraph is empty, then some style previously used
     * in this paragraph is returned.
     * If {@code position > this.length()}, then it is equivalent to
     * {@code position == this.length()}.
     *
     * <p>In other words, {@code getStyleAtPosition(p)} is equivalent to
     * {@code getStyleOfChar(p-1)}.
     */
    public S getStyleAtPosition(int position) {
        return emptyTextStyle;

//        if(position < 0) {
//            throw new IllegalArgumentException("Paragraph position cannot be negative (" + position + ")");
//        }
//
//        TwoDimensional.Position pos = navigator.offsetToPosition(position, Backward);
//        return segments.get(pos.getMajor()).getStyle();
    }

    /**
     * Returns the range of homogeneous style that includes the given position.
     * If {@code position} points to a boundary between two styled ranges,
     * then the range preceding {@code position} is returned.
     */
    public IndexRange getStyleRangeAtPosition(int position) {
        return EMPTY_RANGE;

//        TwoDimensional.Position pos = navigator.offsetToPosition(position, Backward);
//        int start = position - pos.getMinor();
//        int end = start + segments.get(pos.getMajor()).length();
//        return new IndexRange(start, end);
    }

    public StyleSpans<S> getStyleSpans() {
        return null;

//        StyleSpansBuilder<S> builder = new StyleSpansBuilder<>(segments.size());
//        for(StyledText<S> seg: segments) {
//            builder.add(seg.getStyle(), seg.length());
//        }
//        return builder.create();
    }

    public StyleSpans<S> getStyleSpans(int from, int to) {
        return null;

//        TwoDimensional.Position start = navigator.offsetToPosition(from, Forward);
//        TwoDimensional.Position end = to == from
//                ? start
//                : start.offsetBy(to - from, Backward);
//        int startSegIdx = start.getMajor();
//        int endSegIdx = end.getMajor();
//
//        int n = endSegIdx - startSegIdx + 1;
//        StyleSpansBuilder<S> builder = new StyleSpansBuilder<>(n);
//
//        if(startSegIdx == endSegIdx) {
//            StyledText<S> seg = segments.get(startSegIdx);
//            builder.add(seg.getStyle(), to - from);
//        } else {
//            StyledText<S> startSeg = segments.get(startSegIdx);
//            builder.add(startSeg.getStyle(), startSeg.length() - start.getMinor());
//
//            for(int i = startSegIdx + 1; i < endSegIdx; ++i) {
//                StyledText<S> seg = segments.get(i);
//                builder.add(seg.getStyle(), seg.length());
//            }
//
//            StyledText<S> endSeg = segments.get(endSegIdx);
//            builder.add(endSeg.getStyle(), end.getMinor());
//        }
//
//        return builder.create();
    }

    /**
     * Returns the string content of this paragraph,
     * excluding the line terminator.
     */
    @Override
    public String toString() {
        return "";

//        if(text == null) {
//            StringBuilder sb = new StringBuilder(length());
//            for(StyledText<S> seg: segments)
//                sb.append(seg);
//            text = sb.toString();
//        }
//        return text;
    }

    @Override
    public char charAt(int index) {
        // what should be returned here?
        return 0;
    }

    @Override
    public boolean equals(Object other) {
        if(other instanceof EmptyParagraph) {
            EmptyParagraph<?, ?> that = (EmptyParagraph<?, ?>) other;
            return Objects.equals(this.paragraphStyle, that.paragraphStyle)
                    && Objects.equals(this.emptyTextStyle, that.emptyTextStyle);
        } else {
            return false;
        }
    }

    @Override
    public int hashCode() {
        // doesn't this lead to two EmptyParagraph objects having the same hash?
        return Objects.hash(paragraphStyle, emptyTextStyle);
    }
}

@JordanMartinez
Copy link
Contributor

@benedictleejh That sounds to me like it's just a manifestation of this issue because, although the area doesn't have any text, the first Paragraph still has a StyledText object used to style any new insertions, hence the deleted style reappearing.

If the EditableStyledDocument code was implemented, the inserted text would be styled using the default style initialStyle in your described case because there wouldn't be any other style to use.

@ghost
Copy link

ghost commented Jan 13, 2016

I have been quietly following the thread. Is this solvable by making the
insertionStyle user-defined. Then, it can be set to last used, or to
default initial style, or to any adhoc style, regardless if paragraph is
empty or not... no? (not a programmer here.. only a spectator...)

On Tue, Jan 12, 2016 at 7:52 PM, JordanMartinez notifications@github.com
wrote:

@benedictleejh https://github.com/benedictleejh That sounds to me like
it's just a manifestation of this issue because, although the area doesn't
have any text, the first Paragraph still has a StyledText object used to
style any new insertions, hence the deleted style reappearing.

If the EditableStyledDocument code was implemented, the inserted text
would be styled using the default style initialStyle in your described
case because there wouldn't be any other style to use.


Reply to this email directly or view it on GitHub
#216 (comment)
.

@JordanMartinez
Copy link
Contributor

Mmm... I don't think so? I think the Exception would still be thrown even if that was allowed.

And isn't your prorposed idea sort of already allowed via
replace(int start, int end, ReadOnlyStyledDocument doc)
where one can specific the doc by using its factory method:
ReadOnlyStyledDocument.fromString(String str, S style, PS paragraphStyle)

@JordanMartinez
Copy link
Contributor

Looking through the problems (and questions) I listed above, I have a few thoughts on them...

  • CharSequence would need to be removed:
    • EmptyParagraph would no longer need to override CharSequence's charAt() method
    • CharSequence-argument-methods (concat(CharSequence str) and insert(CharSequence str)) would need to be replaced with a corresponding method that uses a Paragraph argument. To get around the problem of figuring out what the returned NormalParagraph's style object would be, the usage of these methods might need to be limited to external classes that would know that information (e.g. EditableStyledDocument).
  • Code that fails or throws an exception if it encounters an object whose length is 0 would need to account for the possibility now:
    • EmptyParagraph would no longer need to have substring() or getStyleSpans()
    • getStyleRangeAtPosition() for EmptyParagraph would no longer be a problem
  • To get around the possibly problematic hashCode() issue, maybe it shouldn't be overridden to allow its default hashCode from Object.

@JordanMartinez
Copy link
Contributor

I've had another go at this bug. See this branch for what I have so far.

Edit: I've made a cleaner version of the branch

@JordanMartinez
Copy link
Contributor

@TomasMikula I'm pretty sure I've accounted for the Style problem you mentioned above via my EmptyParagraph implementation:

I think the right solution is to represent an empty Paragraph as having no text segments. Though there are probably places where we use the style of an empty segment, so we will have to find a way to not need that style.

and in another comment...

First, since there currently is the invariant that Paragraph always has at least one (possibly empty) StyledText, there is code that relies on it and does not have to check whether the Paragraph is empty, e.g. at Paragraph.java#L221, Paragraph.java#L246, and other places.

Secondly, the lines referred to above also return the style of the possibly empty text segment, which is used by EditableStyledDocument#getStyleForInsertionAt(), which in turn is used by EditableStyledDocument#replaceText().

However, the bug still occurs. Can you tell if there is something wrong in my implementation or is there another factor to this problem?

@JordanMartinez
Copy link
Contributor

After doing some more debugging, it seems like richTextChanges will emit NormalParagraph objects that should be EmptyParagraph objects.

@JordanMartinez
Copy link
Contributor

Sweet! Fixed it. See this branch. I'll work on a cleaner way to implement the same basic thing.

@TomasMikula
Copy link
Member

I actually meant that the EmptyParagraph would contain a meaningful style object. That style would be used for text inserted into that paragraph. However, I realized that that would be equivalent to the current situation. Thus the refactoring to EmptyParagraph/NonEmptyParagraph seems to be orthogonal to fixing this bug.

I would like to first have a failing test case written for this bug. Which brings me to the fact that the testability could be improved by better separation of model from view (#241).

@JordanMartinez
Copy link
Contributor

Fixed. The bug was caused by Paragraph#equals(). It doesn't first check each paragraph's length to see if they are both 0 (if so, don't bother to check their segments equality) before checking their respective segments equality. See PR #242.

@JordanMartinez
Copy link
Contributor

After adding some println statements to various places:

StyledDocumentBase#equals():

@Override
public final boolean equals(Object other) {
    if(other instanceof StyledDocument) {
        StyledDocument<?, ?> that = (StyledDocument<?, ?>) other;
        System.out.println(">>> Does this document equal other document? ");
        boolean equaled = Objects.equals(this.paragraphs, that.getParagraphs());
        System.out.println(equaled ? "Yes, documents equal" : "No, documents do not equal");
        return equaled;
    } else {
        return false;
    }
}

Paragraph#equals():

@Override
public boolean equals(Object other) {
    if(other instanceof Paragraph) {
        Paragraph<?, ?> that = (Paragraph<?, ?>) other;
        System.out.println("\tTesting it paragraphs are equal");

        System.out.println("\t\tAre paragraph styles equal? ");
        boolean pstyles = Objects.equals(this.paragraphStyle, that.paragraphStyle);
        System.out.println("\t\t\t" + (pstyles ? "Yes, paragraph styles equal" : "No, paragraph styles do not equal"));

        System.out.println("\t\tAre segments equal? ");
        boolean segmentsEqual = Objects.equals(this.segments, that.segments);
        System.out.println("\t\t\t" + (segmentsEqual ? "Yes, segments equal" : "No, segements do not equal"));

        return pstyles && segmentsEqual;
//            return Objects.equals(this.paragraphStyle, that.paragraphStyle)
//                && Objects.equals(this.segments, that.segments);
    } else {
        return false;
    }
}

TextChange#equals():

@Override
public boolean equals(Object other) {
    if(other instanceof TextChange) {
        TextChange<?, ?> that = (TextChange<?, ?>) other;
        System.out.println(
                "Testing if this text change [" + this.toString() +
                "] equals another text change [" + that.toString() + "]"
        );
        System.out.print("\tPositions are equal? ");
        boolean positions = Objects.equals(this.position, that.getPosition());
        System.out.println("\t\t" + (positions ? "Yes, positions equal" : "No, positions do not equal"));

        System.out.println("\tRemoved StyledDocuments are equal? ");
        boolean removed = Objects.equals(this.removed, that.removed);
        System.out.println("\t\t" + (removed ? "Yes, removed documents equal" : "No, removed documents do not equal"));

        System.out.print("\tInserted StyledDocuments are equal? ");
        boolean inserted = Objects.equals(this.inserted, that.getInserted());
        System.out.println("\t\t" + (inserted ? "Yes, inserted documents equal" : "No, inserted documents do not equal"));

        return positions && removed && inserted;
//            return Objects.equals(this.position, that.position)
//                    && Objects.equals(this.removed, that.removed)
//                    && Objects.equals(this.inserted, that.inserted);
    } else {
        return false;
    }
}

then using this test:

public void printTestOut(String message) {
    String lineBreak = "============================================================";
    System.out.println("\n");
    System.out.println("\t\t\t" + message);
    System.out.println(lineBreak);
}

@Test
public void testForBug216() {
    BoldUnderline initialStyle = new BoldUnderline(false);
    StyledTextAreaModel<BoldUnderline, String> model = new StyledTextAreaModel<BoldUnderline, String>(
            initialStyle, "", new EditableStyledDocument<>(initialStyle, ""), true
    );
    // set up text
    printTestOut("Calling replaceText with 'testtest");
    model.replaceText(0, 0, "testtest");

    // style first and second strings
    printTestOut("Setting first test to bold & underline style");
    model.setStyle(0, 4, new BoldUnderline(true));
    printTestOut("Setting second test to bold & underline style");
    model.setStyle(5, 8, new BoldUnderline(true));

    // add a space styled by initialStyle
    model.positionCaret("test".length());
    model.setUseInitialStyleForInsertion(true);
    printTestOut("Inserting a space using initial style for insertion");
    model.replaceText(model.getCaretPosition(), model.getCaretPosition(), " ");
    model.setUseInitialStyleForInsertion(false);

    // add more spaces
    printTestOut("Adding a space, initial style not used");
    model.insertText(model.getCaretPosition(), " ");

    printTestOut("Undo called");
    model.undo();
    printTestOut("Redo called");
    // Redo should not throw IllegalArgumentException
    model.redo();
}

I get the following output:

:richtextfx:compileJava
:richtextfx:processResources UP-TO-DATE
:richtextfx:classes
:richtextfx:compileTestJava
:richtextfx:processTestResources UP-TO-DATE
:richtextfx:testClasses
:richtextfx:test


            Calling replaceText with 'testtest
============================================================
>>> Does this document equal other document? 
    Testing it paragraphs are equal
        Are paragraph styles equal? 
            Yes, paragraph styles equal
        Are segments equal? 
            No, segements do not equal
No, documents do not equal


            Setting first test to bold & underline style
============================================================
>>> Does this document equal other document? 
    Testing it paragraphs are equal
        Are paragraph styles equal? 
            Yes, paragraph styles equal
        Are segments equal? 
            No, segements do not equal
No, documents do not equal


            Setting second test to bold & underline style
============================================================
>>> Does this document equal other document? 
    Testing it paragraphs are equal
        Are paragraph styles equal? 
            Yes, paragraph styles equal
        Are segments equal? 
            No, segements do not equal
No, documents do not equal


            Inserting a space using initial style for insertion
============================================================
>>> Does this document equal other document? 
    Testing it paragraphs are equal
        Are paragraph styles equal? 
            Yes, paragraph styles equal
        Are segments equal? 
            No, segements do not equal
No, documents do not equal


            Adding a space, initial style not used
============================================================
>>> Does this document equal other document? 
    Testing it paragraphs are equal
        Are paragraph styles equal? 
            Yes, paragraph styles equal
        Are segments equal? 
            No, segements do not equal
No, documents do not equal


            Undo called
============================================================
>>> Does this document equal other document? 
    Testing it paragraphs are equal
        Are paragraph styles equal? 
            Yes, paragraph styles equal
        Are segments equal? 
            No, segements do not equal
No, documents do not equal

Testing if this text change [org.fxmisc.richtext.RichTextChange@76c47763] 
equals another text change [org.fxmisc.richtext.RichTextChange@76c47763]
    Positions are equal?        Yes, positions equal
    Removed StyledDocuments are equal? 
>>> Does this document equal other document? 
    Testing it paragraphs are equal
        Are paragraph styles equal? 
            Yes, paragraph styles equal
        Are segments equal? 
            Yes, segments equal
Yes, documents equal
        Yes, removed documents equal
    Inserted StyledDocuments are equal?         Yes, inserted documents equal


            Redo called
============================================================
>>> Does this document equal other document? 
    Testing it paragraphs are equal
        Are paragraph styles equal? 
            Yes, paragraph styles equal
        Are segments equal? 
            No, segements do not equal
No, documents do not equal

Testing if this text change [org.fxmisc.richtext.RichTextChange@76b5ef63] 
equals another text change [org.fxmisc.richtext.RichTextChange@6c9875e7]
    Positions are equal?        Yes, positions equal
    Removed StyledDocuments are equal? 
>>> Does this document equal other document? 
    Testing it paragraphs are equal
        Are paragraph styles equal? 
            Yes, paragraph styles equal
        Are segments equal? 
            No, segements do not equal
No, documents do not equal
        No, removed documents do not equal
    Inserted StyledDocuments are equal?         Yes, inserted documents equal


org.fxmisc.richtext.StyledTextAreaModelTest > testForBug216 FAILED
    java.lang.IllegalArgumentException at StyledTextAreaModelTest.java:62

Unexpected change received.
Expected:
org.fxmisc.richtext.RichTextChange@76b5ef63
Received:
org.fxmisc.richtext.RichTextChange@6c9875e7

TomasMikula added a commit that referenced this issue Jan 25, 2016
@TomasMikula
Copy link
Member

So in 0677d48 I just changed the order of two statements so that when concatenating two empty paragraphs, the style of the first one is used for the result.

@JordanMartinez
Copy link
Contributor

Great! Does this mean 0.7 can be released?

@JordanMartinez
Copy link
Contributor

Oh, just saw #245...

@JordanMartinez
Copy link
Contributor

In light of @benedictleejh's above comment:

I'd like to suggest that the empty paragraph should return the default style for the text area. I've run into an issue where if all the text in the text area was styled, deleting all the text and then trying to insert new text would cause the new text to inherit the deleted text's style.

I haven't filed an issue for that since @TomasMikula's explanation of this bug implies that if this was properly fixed, that would get fixed as a side effect. If you do prefer, I could certainly file an issue for that however.

The solution that Tomas implemented doesn't account for this specific issue. (Just tried it using the RichText demo). So, I'm opening up a new issue for this (see #247).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants