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

Limit TextChange's type's possibilities by only using it for merging #487

Merged
merged 1 commit into from
Apr 12, 2017
Merged

Limit TextChange's type's possibilities by only using it for merging #487

merged 1 commit into from
Apr 12, 2017

Conversation

JordanMartinez
Copy link
Contributor

Fixes regressions introduced by #458 by implementing the changes proposed in #486.

/** Indicates that the paragraph style of an empty {@link Paragraph} ({@code p.length() == 0}) was changed */
RESTYLED_PARAGRAPH
/** Indicates that the change is a style change, a replacement or something other than the other two types */
NEITHER,
Copy link

Choose a reason for hiding this comment

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

I really don't like that name. When someone would be reading code, "neither of WHAT?" will be constant question.
Because "style change, a replacement or something other" implies, in code below, that no merge will be done, I propose to rename this to MergeType.NONE

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 like NONE better, but it still leaves the similar question of "if it isn't none, then what is it?" 😄

I think we'll just take the minor performance hit and make the code more readable by writing it like so:

if (this.getType == INSERTION || this.getType() == DELETION)
    && this.getType() == that.getType() // and so forth

@JordanMartinez JordanMartinez merged commit eb6d4bf into FXMisc:master Apr 12, 2017
@JordanMartinez JordanMartinez deleted the fixChangeType branch April 12, 2017 15:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants