Add support for FXML (again) by Jugen · Pull Request #654 · FXMisc/RichTextFX

Jugen · 2017-11-22T17:46:20Z

Fixed regression part of #653

JordanMartinez · 2017-11-22T17:52:38Z

Looks like you'll need to revert the default methods back to regular interface method declarations

Could you add a test in this PR, too, that initializes an area with all the FXML configurations? That should insure this doesn't get broken in the future. I'm not sure whether such a test should go in the regular test package or in the integrationTest package.

Jugen · 2017-11-22T19:29:37Z

I've never submitted a test before, so need some guidance:

Does the test need to be registered somewhere or are all class files that have a main method in RichTextFX/richtextfx/src/integrationTest/.../org/fxmisc/richtext automatically executed ?
Do you have to have a method annotated with @Test ?

JordanMartinez · 2017-11-22T20:22:44Z

Mm... yeah I guess this should go in the integrationTest package. I suppose the test class should also go in the api package within integrationTest since this doesn't fall into behavior-related tests, nor is it expansive enough to need its own package.

So, create a new class that extends RichTextFXTestBase (or whatever it's called).
Add a start method that is similar to InlineCssTextAreaAppTest's start method, but without the area or its helper methods.
Add a method that has a name format like "tests_whether_fxml_construction_of_area_works()" that is annotated with @Test. In its body, you could use the FXML loader to create an area, which is then assigned to the stage's root. Then, you could assert that certain properties are true (e.g. it has a context menu, it's text is wrapped, etc.)

In case you're wondering, some test classes are annotated with @RunWith(NestedRunner.class) because it allows one to set up contexts that get shared between multiple tests. Since this is a single test, it's not needed.

JordanMartinez

Your work is almost done. Good job so far.

Were you planning to change the Consumer objects to EventHandler objects, too?

When the PR is complete, I'll ask you to reduce the commit history to the smallest amount of commits that is sensible and to reword some of their messages to better explain what you did in each one. Don't worry about this step until everything else has been taken care of, so that you don't waste your time on it.

JordanMartinez · 2017-11-23T15:48:46Z

+        // FxmlTest.fxml is located in resources folder:
+        // src/integrationTest/resources/org/fxmisc/richtext/api/
+        try	{ obj = FXMLLoader.load( getClass().getResource( "FxmlTest.fxml" ) ); }
+        catch ( Exception EX ) { EX.printStackTrace(); }


There shouldn't be a catch here. Rather, the test method should propagate the thrown exception:

@Test public void testSomething() throws IOException { // no longer need the try or catch statements // try { FXMLLoader.load(); // } catch (IOException EX) {}

Also, it should throw the exception that FXMLLoader.load will throw, not the base Exception class upon which all other exceptions are based.

Will do ....

JordanMartinez · 2017-11-23T15:50:11Z

+<?import javafx.scene.control.ContextMenu?>
+<?import javafx.scene.control.MenuItem?>
+
+<VBox xmlns:fx="http://javafx.com/fxml/1">


To prevent noise, it should return a single area, not a VBox of two areas.

JordanMartinez · 2017-11-23T15:50:36Z

+
+<VBox xmlns:fx="http://javafx.com/fxml/1">
+    <StyleClassedTextArea editable="true" wrapText="true" autoScrollOnDragDesired="true" />
+    <StyleClassedTextArea contextMenuXOffset="12.0" contextMenuYOffset="34.0" >


These two sets of properties should be combined into the same area

I can do that, I did it this way because I thought its easier to read than having one long line of properties :-)

Does it need to be a one-liner? Could you do something like

<StyleClassedTextArea editable="true" wrapText=true" ... <contextMenu>...</contextMenu> </StyleClassedTextArea>

JordanMartinez · 2017-11-23T15:54:55Z

+        Object obj = null;
+        // FxmlTest.fxml is located in resources folder:
+        // src/integrationTest/resources/org/fxmisc/richtext/api/
+        try	{ obj = FXMLLoader.load( getClass().getResource( "FxmlTest.fxml" ) ); }


This should return an area (you'll need to cast the returned object into what it actually is) so we can assert that its properties are what they were specified to be in the FXML document.

JordanMartinez · 2017-11-23T15:57:36Z

+        // src/integrationTest/resources/org/fxmisc/richtext/api/
+        try	{ obj = FXMLLoader.load( getClass().getResource( "FxmlTest.fxml" ) ); }
+        catch ( Exception EX ) { EX.printStackTrace(); }
+        org.junit.Assert.assertNotNull( obj );


We should assert that the properties values are what we specified them to be.

If FXMLLoader cannot set a property it will throw an exception, so if no exception is thrown then it's guaranteed to have been set :-)

Good to know. Mind writing that in as a comment for those of us who don't use it that often?

Jugen · 2017-11-23T16:40:36Z

Were you planning to change the Consumer objects to EventHandler objects, too?

Yes, I've done that in a separate branch. I was going to do a PR once this one is done ?

JordanMartinez · 2017-11-23T16:52:57Z

Yes, I've done that in a separate branch. I was going to do a PR once this one is done ?

Ok. Yeah, I guess that falls into a slightly different scope that this one despite still being FXML related. We should open a separate issue for that, too, so that it shows up in the generated changelog.

JordanMartinez · 2017-11-23T16:54:25Z

Is there a reason for wrapping the area in a VBox?

Jugen · 2017-11-23T18:42:34Z

It could have been any Pane subclass (think Swing layout manager). Originally I had it as a BorderPane, until I wanted to add multiple text areas and made it a VBox for simplicity. Technically it's possible for it to be a Control, although it doesn't usually make sense.

Bear in mind that the FXML file is still going to change in order to accommodate testing the EventHandlers, so if you don't mind I'd prefer to keep it as is for now. We can look at changing it then, if you still think it would be better.

JordanMartinez · 2017-11-24T04:09:35Z

Looks good! Thank you for your work.

JordanMartinez · 2017-11-24T15:55:27Z

I just realized that I forgot to ask you to squash your commits into as few as possible before merging... My bad.

Fix for #653 (phase one)

24547f5

Fixed regression part of #653

Added public method markers

c4cb961

Fxml Test attempt

e90a053

Jugen added 7 commits November 23, 2017 08:55

Delete FxmlTest.java

a3aa203

Delete FxmlTest.fxml

a642497

Fxml Tester 2nd attempt

61bd111

Fixed fxml location

6efc01a

Update FxmlTester.java

693743e

Moved to resources

6318435

Moved FXML file to resources

ca126db

JordanMartinez suggested changes Nov 23, 2017

View reviewed changes

Jugen added 2 commits November 23, 2017 18:30

Removed catch clause and added comment

d8d8a5c

Consolidated properties into one TextArea

812e43e

JordanMartinez approved these changes Nov 24, 2017

View reviewed changes

JordanMartinez changed the title ~~Fix for #653 (phase one)~~ Add support for FXML (again) Nov 24, 2017

JordanMartinez merged commit 08ff468 into FXMisc:master Nov 24, 2017

JordanMartinez mentioned this pull request Nov 24, 2017

Regression of properties that could be set in FXML, fail in 0.8.1 #653

Closed

Jugen deleted the patch-1 branch November 24, 2017 12:15

Conversation

Jugen commented Nov 22, 2017

Uh oh!

JordanMartinez commented Nov 22, 2017

Uh oh!

Jugen commented Nov 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JordanMartinez commented Nov 22, 2017

Uh oh!

JordanMartinez left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jugen commented Nov 23, 2017

Uh oh!

JordanMartinez commented Nov 23, 2017

Uh oh!

JordanMartinez commented Nov 23, 2017

Uh oh!

Jugen commented Nov 23, 2017

Uh oh!

JordanMartinez commented Nov 24, 2017

Uh oh!

JordanMartinez commented Nov 24, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Jugen commented Nov 22, 2017 •

edited

Loading

JordanMartinez left a comment •

edited

Loading