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

Dotted underline #317

Merged
merged 5 commits into from
May 24, 2016
Merged

Dotted underline #317

merged 5 commits into from
May 24, 2016

Conversation

afester
Copy link
Collaborator

@afester afester commented May 20, 2016

Proposal to implement solution for issue #316: "Dotted underline".
This patch adds a path to render underlines for text segments.
The segments can be styled through the following CSS properties:

-fx-underline-stroke           (Color of the underline)
-fx-underline-stroke-dash-size (size of the dashes/spaces for a dashed line - this should really be an array some time)
-fx-underline-stroke-width     (The width of the underline)

The patch contains an enhancement of the JavaKeywords example where the word "simplements" is underlined:

image

@TomasMikula
Copy link
Member

Very cool! Can I propose to change the the names of CSS properties to omit the word "stroke", which kind of refers to the implementation detail that the visible underline is actually a stroke of a Path?

I suggest

-fx-underline-color
-fx-underline-width
-fx-underline-dash-size

Marking just the word simplements as error is somewhat arbitrary in the JavaKeywords demo. I am kind of already spreading bad ideas like using regular expressions to parse Java code. Don't want to get even messier :D. Maybe we can demonstrate the functionality better in a separate, say spell-checker, demo. That doesn't need to be part of this PR though.

I wasn't quite happy with the implementation of the background, because it creates the background Path for each Text node, even if that node doesn't specify any background. We might want to optimize this in the future.

For good looking results, it might also be necessary to join the underlines of neighboring segment, if they specify the same underline style, into one. This would be less of a problem with solid lines, but adjacent dashed lines will look ugly.

@afester
Copy link
Collaborator Author

afester commented May 24, 2016

Hi Tomas,
I have now implemented the following properties:

-fx-underline-color
-fx-underline-width
-fx-underline-dash-array
-fx-underline-cap

With -fx-underline-dash-array it is now possible to specify a real array of dash lengths so that it supports also dot/dash style lines.
I also added a simple spell checking example and reverted the JavaKeywords sample. I think it makes sense to have a (at least simple) example available.
I have not yet approached the possibly unnecessary creation of Paths for each text segment.
I have also not yet worked on joining adjacent lines if they have the same style - this might require some more investigation. Its probably not only about the styles of the lines which need to be the same, but also the vertical position, which might depend on the style of the text segment itself.
Anyway, please let me know if you think that either of these topics still need to be addressed for the pull request ....

@TomasMikula
Copy link
Member

Looks good 👍 Thanks for the contribution!

@TomasMikula TomasMikula merged commit 7c32514 into FXMisc:master May 24, 2016
@TomasMikula TomasMikula mentioned this pull request May 26, 2016
@JordanMartinez
Copy link
Contributor

@afester Am I correct in understanding that the underline CSS properties you added take affect regardless of Text#isUnderline()? I've written that in the CSS guide for this project, but I wanted to confirm that.

@afester
Copy link
Collaborator Author

afester commented May 31, 2016

@JordanMartinez yes, as of now that is correct. Probably we should change that though - I will Check when I am back from vacation...

@TomasMikula
Copy link
Member

TomasMikula commented May 31, 2016

What @afester implemented is much richer than the pre-existing boolean property, which supports only a solid line of the same color as the text. I'm fine with keeping them as two independent properties, although I see that the naming will be confusing.

@JordanMartinez
Copy link
Contributor

@afester ok. Enjoy your vacation 🍹

@TomasMikula what should the naming scheme be then?

@afester
Copy link
Collaborator Author

afester commented Jun 8, 2016

@JordanMartinez Ideally the -fx-underline property should control whether the stylable underline which I implemented is visible or not. However, the property is implemented as final in the Text class and it directly controls the creation of the underline node (implemented in the Text's peer class NGText) - which, as @TomasMikula wrote, is always solid and in the same color as the text. So it seems that there is no possibility to redirect and/or reuse that property to control the stylable underline, instead of enabling/disabling the solid underline provided by Text. I am not completely satisfied with the confusing naming scheme either, and I am definitely open for any suggestions on better alternatives ...

@afester afester deleted the dottedUnderline branch June 8, 2016 14:27
@JordanMartinez
Copy link
Contributor

I have an idea for the name scheme. Since the underline CSS is what is typically found in the Text class and we have implemented an alternative form for the underline, why don't we add the alt word in front?

In order words, the CSS you've implemented would be renamed to:

-fx-alt-underline-color
-fx-alt-underline-width
-fx-alt-underline-dash-array
-fx-alt-underline-cap

@afester
Copy link
Collaborator Author

afester commented Jun 8, 2016

Yes, that makes sense. I would then also add -fx-alt-underline as the fx-underline equivalent which then serves as the toggle switch to enable/disable the underline. The default rendering would be a solid line in the same color as the text, to be compatible with the normal -fx-underline behaviour. @TomasMikula what do you think?

@TomasMikula
Copy link
Member

A non-zero width can serve as the toggle. To choose from different styles of the line, we could have

-fx-alt-underline-style: solid | dashed

with solid being the default. We could add e.g. dotted later.

@afester
Copy link
Collaborator Author

afester commented Jun 9, 2016

Ok, that is fine. We should not need -fx-alt-underline-style though - that is configurable in a much more flexible way through the -fx-alt-underline-dash-array property.

@afester
Copy link
Collaborator Author

afester commented Jun 9, 2016

One (final) thought: Should we not use a custom prefix for RichTextFX specific CSS property names, like -rtfx instead of -fx? It seems that there is not a definitive recommendation for that in the JavaFX specs, but it would at least lower the risk for future name clashes, and it makes much more obvious which properties are RichTextFX specific. See http://stackoverflow.com/questions/36725475/naming-of-css-property-names-for-javafx.

The property names could then be like

-rtfx-underline-color
-rtfx-underline-width
-rtfx-underline-dash-array
-rtfx-underline-cap

@TomasMikula
Copy link
Member

-rtfx sounds good to me 👍

Well, dotted is not quite the same as dashed, is it? And what if someone wants to contribute a squiggly line?

@JordanMartinez
Copy link
Contributor

I'll file a new issue to update the RTFX-specific CSS to use the -rtfx- prefix, that way we can track it.

@afester
Copy link
Collaborator Author

afester commented Jun 10, 2016

I have updated the property names and the behaviour wrt. the non-zero width in #324.

Well, dotted is not quite the same as dashed, is it? And what if someone wants to contribute a squiggly line?

Yes, but you can define simple dotted lines already now, like

-rtfx-underline-dash-array: 2 2;
-rtfx-underline-width: 2;

Squiggly/waved/triangled underlines (often seen in IDEs like Eclipse), or even a completely custom rendering is if course another thing, which could be added later, now that we have the required path segments/coordinates which make up the underline ...

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.

3 participants