Custom EditableStyledDocument

[JordanMartinez]

I used EditableStyledDocument's methods as the base for the new interface.

[JordanMartinez]

I didn't include replaceText(int, int, String) because that seems to be handled by StyledTextAreaModel now and you seem to only have it in EditableStyledDocumentImpl as a convenience method to use when testing that class

[JordanMartinez]

I'd also like to rename plainTextChanges to plainChanges in StyledTextAreaModel and StyledTextArea. I know it introduces a breaking change, but it seems to be a more consistent naming scheme (e.g. plainChanges and richChanges).

[TomasMikula]

If there is a rich-text change, then insertedLength or removedLength is non-zero already. You probably want to first map and then filter out plain-text changes where removed.equals(inserted).

[JordanMartinez]

Wouldn't it be more efficient to filter it first? Otherwise, we're creating unneeded PlainTextChangees:

default EventStream<PlainTextChange> plainChanges() {
    return richChanges()
            // exclude style changes
            .filter(c -> !c.inserted.getText().equals(c.removed.getText()))
            .map(c -> new PlainTextChange(c.position, c.removed.getText(), c.inserted.getText()));
}

[TomasMikula]

Yes, slightly. On the other hand there is a slight repetition of code in calling getText().

And if you want to avoid an intermediate EventStream, use filterMap.

[JordanMartinez]

Yes, slightly. On the other hand there is a slight repetition of code in calling getText().

Yeah... I noticed that, too. One way to get around it 😜

    default EventStream<PlainTextChange> plainChanges() {
        return richChanges()
                // exclude style changes
                .map(c -> {
                    String inserted = c.inserted.getText();
                    String removed = c.removed.getText();
                    if (!inserted.equals(removed)) {
                        return new PlainTextChange(c.position, removed, inserted);
                    } else {
                        return null;
                    }
                })
                .filter(c -> c != null);
    }

[TomasMikula]

I don't care so much, do what you think is right ;)

[TomasMikula]

I wouldn't care too much about optimizing one object allocation, since there are already other much higher costs paid anyway.

[JordanMartinez]

With the most recent approach used, does that lead to higher costs due to the EventStreams?

[TomasMikula]

An extra EventStream adds a few stack frames to the call stack when propagating the event. But even by the time an edit reaches the plainChanges() stream, so many objects have been allocated and dismissed that one more or less doesn't make any difference. Therefore, I would go for code clarity.

[JordanMartinez]

So you do care 😉

Ok. I didn't factor that into consideration. Code clarity it is.

[TomasMikula]

I guess I do care, when I see how much noise 4e6ce6b adds in the name of saving one object allocation per change. Also, code comments can go out of sync with code very easily, so self-documenting code is better :)

[JordanMartinez]

On another note, does StyledTextArea#getModel() need to be protected? I feel like it should be package-private now (it was used to get the underlying document, but we have a better API for that now)

[TomasMikula]

Agreed.

[TomasMikula]

I don't think it is very efficient :) Could be improved.

[JordanMartinez]

Lol... that was my assumption since you wrote it 😄

[JordanMartinez]

Whoops... that commit message should be "package-private" not "private"...

[JordanMartinez]

If there's nothing else that needs to be fixed, I'd like to open a new PR that fixes the "package-private not private" erroneous commit message.

[TomasMikula]

You can also

git rebase -i HEAD~14

to reorder and squash some of the last 14 commits and then

git push -f

to rewrite the history of this PR.

[JordanMartinez]

Sorry that it took so long to update the PR. I almost finished updating the history, but then I had to leave for something and wasn't able to complete the update.

[TomasMikula]

No worries, my reaction periods tend to be much longer ;)

[TomasMikula]

Out of context of our previous discussion, this comment does not make much sense, does it? ;)

[JordanMartinez]

I suppose not... should it be removed entirely?

[JordanMartinez]

Is there anything else to do before merging this PR? I still haven't heard back from you about the "out of context" mapping comment.

[TomasMikula]

Yeah, I think that comment is better removed entirely.

[JordanMartinez]

The commit message gives more context to the comment removal. So, that should suffice for now.

[TomasMikula]

👍

Going to merge now. Though EditableStyledDocument as it stands now is a hell of an interface asking someone to implement. It should be gradually simplified.

[JordanMartinez]

Though EditableStyledDocument as it stands now is a hell of an interface asking someone to implement. It should be gradually simplified.

That is true.... On one hand, StyledTextAreaModel does seem to make use of all of them at some point or another (whether directly or through some other call within a method call). Still, I didn't check to insure that it was as simple as StyledTextAreaModel would currently allow.
On another hand, making StyledDocumentBase public (or perhaps an EditableStyledDocumentBase) could help with that.

[JordanMartinez]

When you have time, could you update the snapshot release?

[TomasMikula]

Updated.

[JordanMartinez]

Thanks! 👍