Implemented background colors for text #166

MewesK · 2015-08-16T09:24:29Z

Just as you suggested the implementation is based on the same principles as the selection fill.
On top of that I had to extend Text to support CSS based colors.

I should mention that I have only implemented -fx-background-color and not anything else from -fx-background. JavaFX under the hood is so damn weird that it would be rather ugly to support the actual Background property. Using just the color property makes the code simple and shouldn't have a huge impact like having an actual Image in every Text instance.

One thing I would like to improve is the background node instantiation. Right now every Text instance has a matching background Path. Maybe it is possible to create only Path instances when there actually is a background color set.

Well I'm actually quite happy with the result. It'S not perfect yet but it works great. The missing background color feature was a real show stopper for me. I'm glad that I was able to fix that. (taught me a lot about JavaFX)

TomasMikula · 2015-08-16T17:28:43Z

richtextfx/src/main/java/org/fxmisc/richtext/skin/TextExt.java

+
+public class TextExt extends Text {
+
+    private ObjectProperty<Paint[]> backgroundColor = null;


If only a single color is supported, the type should not lie about it. Let's just change it to ObjectProperty<Paint>.

That the way the JavaFX converter works. I was using Color instead of Paint[] before, but the CSS converter injected a Paint array anyway which led to casting errors along the way.

It's the same way in the internal implementation in the Background class. All I can think of is making the property private and expose a Paint property which hides the implementation details.

StyleConverter.getPaintConverter() should do the job.

I tried the ColorConverter before with the mentioned behavior. But I will give this one a try too, thanks.

Nope sorry, this one has the same behavior. Under the hood it still injects a Paint array which leads to casting errors later on:

Exception in thread "JavaFX Application Thread" java.lang.ClassCastException: [Ljavafx.scene.paint.Paint; cannot be cast to javafx.scene.paint.Paint

Maybe I'm just too stupid to make it work so don't hold back if you want to try your luck.

I reported a bug: https://bugs.openjdk.java.net/browse/JDK-8133685

Ranming it might cause confusion for the user who expects -fx-background-color.
So we can either just keep it like it is and tolerate the unnecessary Paint array. Or we rename it.
-fx-background-fill would be a good choice since it follows -fx-highlight-fill. It's your call.

There is another problem and that is that com.sun.* is not going to be visible from JDK 9 on. So unless you can implement it without com.sun.*, I would say go with -fx-background-fill. (Sure, for now you could probably get away with just giving null as the converter, since it is overridden anyway, but that would stop working when/if they fix the bug).

I just changed the code based on your last comment. I will test the latest version tonight a bit more.

My next pull request will be about line styles (think red background for lines with break point or or yellow background for current selected line). Any suggestions to make it more likely to get merged upstream?

A nifty customisation would be controlling line style and background offset
from/around text. This will enable highlighting text in different ways. Not
sure how hard it is to implement though.

In my mind, one of the things to consider is how the background will work
when making text selection. For example, if selection overlaps an already
coloured text background?

I look forward to plying with your addition :)

On Aug 17, 2015 3:58 AM, "Mewes Kochheim" [email protected] wrote:

In richtextfx/src/main/java/org/fxmisc/richtext/skin/TextExt.java
#166 (comment):

+import javafx.beans.property.ObjectProperty;
+import javafx.css.CssMetaData;
+import javafx.css.Styleable;
+import javafx.css.StyleableObjectProperty;
+import javafx.css.StyleableProperty;
+import javafx.scene.paint.Color;
+import javafx.scene.paint.Paint;
+import javafx.scene.text.Text;
+
+import java.lang.reflect.Field;
+import java.util.ArrayList;
+import java.util.List;
+
+public class TextExt extends Text {
+

private ObjectProperty<Paint[]> backgroundColor = null;

I just changed the code based on your last comment. I will test the latest
version tonight a bit more.

My next pull request will be about line styles (think red background for
lines with break point or or yellow background for current selected line).
Any suggestions to make it more likely to get merged upstream?

—
Reply to this email directly or view it on GitHub
https://github.com/TomasMikula/RichTextFX/pull/166/files#r37176819.

TomasMikula · 2015-08-16T17:29:39Z

Great job! See my comments above.

Implemented background colors for text

TomasMikula · 2015-08-17T17:13:11Z

Looks good, thanks for the contribution!

@melkhaldi so you would want the equivalent of the -fx-background-insets property. It is not super difficult to implement, but is not completely trivial either. You would have to manipulate a closed Path to expand by the given insets.

@MewesK Regarding styling the whole lines, let's move the discussion to #6.

TomasMikula · 2015-08-17T19:55:40Z

I have modified the RichText demo to support background color.

ghost · 2015-08-17T20:07:16Z

Hi Tomas, just tried the JAR to test, it does not seem to have means to apply color to background. Did you add a dedicated button/GUI control for it?

TomasMikula · 2015-08-17T20:12:16Z

Hi Maher, I haven't uploaded new jars. You would have to build from source at this point. It is just 3 commands, if you have Gradle installed:

git clone https://github.com/TomasMikula/RichTextFX.git
cd RichTextFX
gradle RichText

TomasMikula · 2015-08-17T20:12:46Z

I will make a new release later.

ghost · 2015-08-17T20:13:30Z

oh alright, i thought it was uploaded already. Don't have gradle set up yet
:) (did a reformat)

On Mon, Aug 17, 2015 at 1:12 PM, Tomas Mikula [email protected]
wrote:

Hi Maher, I haven't uploaded new jars. You would have to build from source
at this point. It is just 3 commands, if you have Gradle installed:

git clone https://github.com/TomasMikula/RichTextFX.git
cd RichTextFX
gradle RichText

—
Reply to this email directly or view it on GitHub
#166 (comment)
.

JordanMartinez · 2015-08-17T20:50:54Z

I followed Tomas' instructions to set it up from the source. It works good! Great job guys!

Here's one idea for a slight enhancement (though it isn't a high priority at all). When the background-fill is blue, it's hard to distinguish it from a selected text (see the highlighted, selected example below). This could be improved further if the selection color (which defaults to blue) uses a different color when selecting a text that has a similarly-colored background-fill.

I also noticed that a white vertical line appears between two texts with different background-fills, but that line is only visible if the text is rather large. Again, not really that big of an issue.

(Edit: The black vertical line separated "text|text" is due to screenshotting the window when the caret was visible. This isn't some bug.)

MewesK · 2015-08-18T14:46:23Z

Hi Jordan,

the white line is caused by the internal TextLayout class. We could of course just add 1px to the width of each BG shape to work around this "bug" but it may look "wrong" for small text.

The second issue with two similar fill colors for BG and selection could be dealt with by using a slightly transparent color for the selection. (that's the way Mac OS is doing it at least) I'll play around with this idea a little when I#M at home.

JordanMartinez · 2015-08-18T15:11:34Z

@MewesK I think the white line isn't a big enough issue to do that. However, if the situation iss at least known here, then a person who truly needs that to be gone could talk to you about how to adjust it or try doing it themselves...

I'm not very familiar with Mac, so I'm not sure how that would work. I'm sure you'll come up with something :-)

ghost · 2015-08-18T15:21:14Z

I believe this might be something that can be controlled via an inset
property. For example, if char at end is so and so, set inset to +x.
On Aug 18, 2015 8:11 AM, "JordanMartinez" [email protected] wrote:

@MewesK https://github.com/MewesK I think the white line isn't a big
enough issue to do that. However, if the situation iss at least known here,
then a person who truly needs that to be gone could talk to you about how
to adjust it or try doing it themselves...

I'm not very familiar with Mac, so I'm not sure how that would work. I'm
sure you'll come up with something :-)

—
Reply to this email directly or view it on GitHub
#166 (comment)
.

TomasMikula · 2015-08-18T16:22:10Z

Regarding the white gap, we could also try to construct a minimal example, where TextLayout#getRange(0, 1) ends at position x and TextLayout#getRange(1, 2) starts at x+ɛ and file it to the JavaFX team. Though I'm not very optimistic that they will resolve it, since TextLayout is private API.

MewesK added 2 commits August 16, 2015 11:13

Implemented background color for text

17dc982

Performance improvements

8fb1c4e

TomasMikula reviewed Aug 16, 2015
View reviewed changes

MewesK and others added 2 commits August 16, 2015 22:06

Improvements based on TomasMikulas comments

76fb7f7

Latest changes based on FXMisc#166

ab7af2d

TomasMikula added a commit that referenced this pull request Aug 17, 2015

Merge pull request #166 from MewesK/master

84d9588

Implemented background colors for text

TomasMikula merged commit 84d9588 into FXMisc:master Aug 17, 2015

TomasMikula mentioned this pull request Aug 17, 2015

Support background color for a range of text #22

Closed

TomasMikula mentioned this pull request Jul 18, 2016

Matching Bracket/Brace/Paren Highlight #337

Closed

JordanMartinez mentioned this pull request May 4, 2017

Add support for a border around some text #346

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implemented background colors for text #166

Implemented background colors for text #166

MewesK commented Aug 16, 2015

TomasMikula Aug 16, 2015

MewesK Aug 16, 2015

TomasMikula Aug 16, 2015

MewesK Aug 16, 2015

MewesK Aug 16, 2015

TomasMikula Aug 16, 2015

MewesK Aug 16, 2015

TomasMikula Aug 16, 2015

MewesK Aug 17, 2015

ghost Aug 17, 2015

TomasMikula commented Aug 16, 2015

TomasMikula commented Aug 17, 2015

TomasMikula commented Aug 17, 2015

ghost commented Aug 17, 2015

TomasMikula commented Aug 17, 2015

TomasMikula commented Aug 17, 2015

ghost commented Aug 17, 2015

JordanMartinez commented Aug 17, 2015

MewesK commented Aug 18, 2015

JordanMartinez commented Aug 18, 2015

ghost commented Aug 18, 2015

TomasMikula commented Aug 18, 2015


		public class TextExt extends Text {

		private ObjectProperty<Paint[]> backgroundColor = null;

Implemented background colors for text #166

Implemented background colors for text #166

Conversation

MewesK commented Aug 16, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomasMikula commented Aug 16, 2015

TomasMikula commented Aug 17, 2015

TomasMikula commented Aug 17, 2015

ghost commented Aug 17, 2015

TomasMikula commented Aug 17, 2015

TomasMikula commented Aug 17, 2015

ghost commented Aug 17, 2015

JordanMartinez commented Aug 17, 2015

MewesK commented Aug 18, 2015

JordanMartinez commented Aug 18, 2015

ghost commented Aug 18, 2015

TomasMikula commented Aug 18, 2015