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.

Sign up for GitHub

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

Custom EditableStyledDocument #277

Merged
merged 12 commits into from
Apr 1, 2016
Merged

Custom EditableStyledDocument #277

Changes from 1 commit
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
1eb8e5a
Rename EditableStyledDocument to EditableStyledDocumentImpl in prepar…
JordanMartinez Mar 31, 2016
a8f0f44
Added interface for custom EditableStyledDocument
JordanMartinez Mar 31, 2016
f4e111f
Make EditableStyledDocumentImpl implement new interface
JordanMartinez Mar 31, 2016
5583d52
Update constructors to use new interface
JordanMartinez Mar 31, 2016
bbf8efc
Reduce access privilege to package-private (protected not needed)
JordanMartinez Mar 31, 2016
277b548
Increase accessibility for clone-related fields to public
JordanMartinez Mar 31, 2016
fc48bae
Add API for getting the area's EditableStyledDocument for cloning
JordanMartinez Mar 31, 2016
4ec6411
Reduce getModel() accessiblity to private as it's no longer needed to…
JordanMartinez Mar 31, 2016
2f55e40
Update factory class to use new API for getting the document of an area
JordanMartinez Mar 31, 2016
9aa1a8b
Provide default implementation for plainChanges
JordanMartinez Mar 31, 2016
fb39977
Ported implementation's javadoc to EditableStyledDocument and updated…
JordanMartinez Mar 31, 2016
89af8ca
Remove comment about why map is used as it is no longer in its contex…
JordanMartinez Apr 1, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion richtextfx/src/main/java/org/fxmisc/richtext/EditableStyledDocument.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,13 @@ public interface EditableStyledDocument<PS, S> extends StyledDocument<PS, S> {

ReadOnlyStyledDocument<PS, S> snapshot();

EventStream<PlainTextChange> plainChanges();
default EventStream<PlainTextChange> plainChanges() {
return richChanges()
// map is used to prevent code repetition: StyledDocument#getText()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

.map(c -> new PlainTextChange(c.position, c.removed.getText(), c.inserted.getText()))
// filter out rich changes where the style was changed but text wasn't added/removed
.filter(pc -> !pc.removed.equals(pc.inserted));
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()));
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
    }

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So you do care 😉

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)


EventStream<RichTextChange<PS, S>> richChanges();

Expand Down