Issue #594: Properly calculate the background and underline shapes fo… #595

afester · 2017-09-28T10:50:24Z

…r non-consecutive ranges

Added an integration test which does some simple styling tests
The underline test fails without the code modifications in ParagraphText.java
Fixed the calculation of shapes in ParagraphText.java: shapes must only be combined when the ranges are consecutive

JordanMartinez

Thanks for the PR.

There's a few things that can be cleaned up before merging this

JordanMartinez · 2017-09-28T19:30:51Z

richtextfx/src/integrationTest/java/org/fxmisc/richtext/style/StylingTests.java

+import javafx.scene.text.Text;
+import javafx.scene.text.TextFlow;
+
+@RunWith(NestedRunner.class)


NestedRunner is only necessary if you want to use a class to set up common things among multiple tests. Since you're not doing that here, you can remove it.

True. It was essentially a preparation for later enhancements, but it is indeed better to add it once it is really required.

I like NestedRunner because it reduces code duplication when setting up similar tests. However, it doesn't allow one to run tests within the nested classes in my IDE. I'm not sure how to fix that, but even if we did, I would think that such a thing should not need to be explained for the tests to work on someone else's computer.

JordanMartinez · 2017-09-28T19:32:43Z

richtextfx/src/integrationTest/java/org/fxmisc/richtext/style/StylingTests.java

+        Region paragraphBox = getParagraphBox(0);
+
+        // get the ParagraphText (protected subclass of TextFlow) 
+        TextFlow tf = (TextFlow) paragraphBox.getChildrenUnmodifiable().get(0);


Could this be changed to check for the instance first, only cast if it's valid and throw an exception if it's not? Otherwise, I am not sure it will be the first one if a selection is made.

True. get(0) is not always correct - I need to iterate over the children and look up the TextFlow (for which there should be only one, though),

JordanMartinez · 2017-09-28T19:33:37Z

richtextfx/src/integrationTest/java/org/fxmisc/richtext/style/StylingTests.java

+
+        // expected: one text node which contains the complete text
+        List<Text> textNodes = getTextNodes();
+        assertEquals(1, textNodes.size());


Perhaps this should be its own test?

JordanMartinez · 2017-09-28T19:34:20Z

richtextfx/src/integrationTest/java/org/fxmisc/richtext/style/StylingTests.java

+        List<Text> textNodes = getTextNodes();
+        assertEquals(1, textNodes.size());
+
+        // Set word "World" to bold


The comment forces me to look up what String object is equated with "World".

JordanMartinez · 2017-09-28T19:35:33Z

richtextfx/src/integrationTest/java/org/fxmisc/richtext/style/StylingTests.java

+    private final static String styledWord1 = "World";
+    private final static String moreText = " and also the ";
+    private final static String styledWord2 = "Sun";
+    private final static String remainingLine = " and Moon";


Relating to line 89's comment, since the string itself does not contain the style, it should equal to what the text it is assigned is. Otherwise, styledWord1 is unstyled in one moment and styled in a later moment.

JordanMartinez · 2017-09-28T19:36:07Z

richtextfx/src/integrationTest/java/org/fxmisc/richtext/style/StylingTests.java

+        // setup
+        interact(() -> {
+            area.replaceText(firstWord + styledWord1 + moreText + styledWord2 + remainingLine);
+            //               "Hello World and also the Sun and Moon"


the comment isn't necessary if we rename the strings to the text they represent

JordanMartinez · 2017-09-28T19:41:23Z

richtextfx/src/integrationTest/java/org/fxmisc/richtext/style/StylingTests.java

+            final int start2 = end1 + moreText.length();
+            final int end2 = start2 + styledWord2.length();
+            area.setStyle(start2, end2,
+                          "-rtfx-underline-color: red; -rtfx-underline-dash-array: 2 2; -rtfx-underline-width: 1; -rtfx-underline-cap: butt;");


Could this be declared once in a variable and then just pass that variable in to both area.setStyle() calls?

afester · 2017-10-02T11:25:42Z

Won't this return the background shapes and border shapes as well if we add more tests that test for those things, too.

Yes, that is true. In general those methods rely on a particular order how the nodes are inserted into the text flow. It would be better if the node objects themselves know "what" they are. The straight forward solution would be to subclass Path as CaretPath, SelectionPath, BackgroundPath and UnderlinePath - and then use the sub classes instead of plain Path. This would make debugging and inspection of the scene graph (and writing scene related tests) MUCH simpler, since it is immediately visible what kind of path that particular node represents. Alternatively, we could set the id with a specific prefix for each path, but that would require unique id generation (like "bg1,", "bg2" etc.) and it would refer to the instance, not type. Finally, we could also subclass Path once and user a getter to retrieve the type attribute, something like

Path underlinePath = new TypedPath(PATHTYPE.UNDERLINE);
...
if (underlinePath.getType() == PATHTYPE.UNDERLINE) { ...}

JordanMartinez · 2017-10-02T16:29:47Z

Very good points. I definitely like the subclass or path type approach better than setting the ID. However, which would be easier to use when writing tests and would scale well if we need to add more components to it?

JordanMartinez · 2017-10-02T18:10:24Z

However, which would be easier to use when writing tests and would scale well if we need to add more components to it?

Actually, since we may never add more rtfx-specific components (YAGNI principle), which approach (and its API) will make reading through the tests easy?

JordanMartinez · 2017-10-03T17:06:54Z

Looks like #596 is going to work soon. Once it gets merged, can you rebase this (with the changes mentioned) on top of that PR's work?

JordanMartinez · 2017-10-03T19:46:21Z

Alright #596 has been merged. Looking forward to your update.

afester · 2017-10-04T12:08:42Z

Updated PR primarily for review of the path subclass approach.
The one failed Travis test seems to be
org.fxmisc.richtext.api.CaretTests > caret_bounds_are_absent_after_moving_caret_without_following_it FAILED
java.util.concurrent.TimeoutException

JordanMartinez · 2017-10-04T15:21:03Z

I still see your original commits at the start of this PR and a whole lot of "commit noise" from my other PR. Did you rebase your commits or is this a GitHub issue?

Yeah... that test apparently failed in my PR, too. However, Travis had some backlog and it didn't report the issue correctly or something, so I thought it passed. They emailed me the failure later.

The issue is that the first test fails, irregardless of which one is run first in that class. I'm not sure where it's timing out thought. Might need to add TestLoggingEvent.STANDARD_ERROR to the build.gradle's integrationTest.testLogging closure.

I'll look over the code later on this week (got other things to do today unfortunately...). However, could you move the test itself to integrationTest?

afester · 2017-10-04T19:14:11Z

I still see your original commits at the start of this PR and a whole lot of "commit noise" from my other PR. Did you rebase your commits or is this a GitHub issue?

Yes, I rebased but forgot that the rebased local branch needs to be force-pushed 🥇
I just re-did the rebase, looks much better now...

However, could you move the test itself to integrationTest?
The tests are in the src/integrationTest directory - I suppose that this is the right location?

JordanMartinez

Please amend your commit to fix the issue and force push it.

I'll merge it after that. The TimeoutException thrown in the Mac build isn't related to this at all, so we'll ignore it. That'll need to be addressed in another PR later.

JordanMartinez · 2017-10-05T14:43:10Z

richtextfx/src/main/java/org/fxmisc/richtext/CaretPath.java

+
+/**
+ * A path which describes a selection shape in the Scene graph.
+ * 


"selection" is written instead of what you actually meant: "caret."

Otherwise, everything looks good!

JordanMartinez · 2017-10-06T05:20:16Z

Ok. The Mac build issue has been resolved now.

JordanMartinez · 2017-10-09T15:15:03Z

@afester Can you fix the javadoc issue and rebase on top of master? Then the mac build should pass.

…pes for non-consecutive ranges

JordanMartinez · 2017-10-11T01:44:39Z

Thanks for your work!

JordanMartinez suggested changes Sep 28, 2017

View reviewed changes

afester force-pushed the Issue594 branch from 9321584 to 44729ca Compare October 4, 2017 19:08

JordanMartinez suggested changes Oct 5, 2017

View reviewed changes

JordanMartinez mentioned this pull request Oct 6, 2017

First test run on Mac fails with TimeoutException #608

Closed

Issue FXMisc#594: Properly calculate the background and underline sha…

dd1588c

…pes for non-consecutive ranges

afester force-pushed the Issue594 branch from 44729ca to dd1588c Compare October 9, 2017 19:48

JordanMartinez merged commit eb0e2f1 into FXMisc:master Oct 11, 2017

JordanMartinez mentioned this pull request Oct 11, 2017

Background and underline shapes not properly calculated #594

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue #594: Properly calculate the background and underline shapes fo… #595

Issue #594: Properly calculate the background and underline shapes fo… #595

afester commented Sep 28, 2017

JordanMartinez left a comment

JordanMartinez Sep 28, 2017

afester Oct 2, 2017

JordanMartinez Oct 2, 2017

JordanMartinez Sep 28, 2017

afester Oct 2, 2017

JordanMartinez Sep 28, 2017

JordanMartinez Sep 28, 2017

JordanMartinez Sep 28, 2017

JordanMartinez Sep 28, 2017

JordanMartinez Sep 28, 2017

afester commented Oct 2, 2017

JordanMartinez commented Oct 2, 2017

JordanMartinez commented Oct 2, 2017

JordanMartinez commented Oct 3, 2017

JordanMartinez commented Oct 3, 2017

afester commented Oct 4, 2017

JordanMartinez commented Oct 4, 2017

afester commented Oct 4, 2017

JordanMartinez left a comment

JordanMartinez Oct 5, 2017

JordanMartinez commented Oct 6, 2017

JordanMartinez commented Oct 9, 2017

JordanMartinez commented Oct 11, 2017

Issue #594: Properly calculate the background and underline shapes fo… #595

Issue #594: Properly calculate the background and underline shapes fo… #595

Conversation

afester commented Sep 28, 2017

JordanMartinez left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afester commented Oct 2, 2017

JordanMartinez commented Oct 2, 2017

JordanMartinez commented Oct 2, 2017

JordanMartinez commented Oct 3, 2017

JordanMartinez commented Oct 3, 2017

afester commented Oct 4, 2017

JordanMartinez commented Oct 4, 2017

afester commented Oct 4, 2017

JordanMartinez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JordanMartinez commented Oct 6, 2017

JordanMartinez commented Oct 9, 2017

JordanMartinez commented Oct 11, 2017