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

Added some documentation to some small classes #482

Merged
merged 7 commits into from
Apr 10, 2017

Conversation

neilccbrown
Copy link

I tried to document everything where I was confident about how it worked, so a few methods in these classes are still uncommented. But thought it might prove useful in improving the Javadoc a bit, even if some of the content is already on the wiki.

@JordanMartinez
Copy link
Contributor

Thank you for the PR!
I'll take a closer look at this later on in the week as I don't have time right now.

Copy link
Contributor

@JordanMartinez JordanMartinez left a comment

Choose a reason for hiding this comment

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

I had a few moments of free time 😄

Overall the documentation is clear.

Additional explanation or forewarning could be added in a few places.
I also think that classes should be referred to using {@link GenericStyledArea} rather than just plain text GenericStyledArea as it will force us to update those values should we later rename those classes.

* @param <PS> The type of the paragraph style.
* @param <SEG> The type of the content segments in the paragraph (e.g. {@code StyledText<S>})
* @param <S> The type of the style of individual segments.
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks good. Perhaps there should also be a note that explains that constructing a Paragraph must always have at least one SEG object, even if it is an empty one, to insure the code works properly.

* A class which adds some more styleable properties to JavaFX's Text class.
*
* They can be styled using the properties (and accessors/mutators) or via CSS.
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps there should be a note here that explains that the added-on CSS properties all start with the "-rtfx" prefix.

Copy link
Contributor

Choose a reason for hiding this comment

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

Moreover, I believe the added-on underline CSS properties will take effect even if Text#isUnderline is false. This should probably be stated at the class level and for each underline-related property

/**
* Creates a new StyledText with the same content but the given style.
*
* <p>This class is unmodified.</p>
Copy link
Contributor

Choose a reason for hiding this comment

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

Does "This class is unmodified" need to be there since you already specify that the method creates a new object and the class-level javadoc says the object is immutable?

@neilccbrown
Copy link
Author

I believe I've addressed all your comments in the newest commits. Thanks for taking the time to consider them.

@JordanMartinez JordanMartinez merged commit 1e96f94 into FXMisc:master Apr 10, 2017
@JordanMartinez
Copy link
Contributor

Thanks!

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