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

Java 9 compatibility (using reflection) #614

Merged
merged 18 commits into from
Oct 20, 2017

Conversation

JFormDesigner
Copy link
Contributor

@JFormDesigner JFormDesigner commented Oct 14, 2017

This PR makes RichTextFX compatible to Java 9 (fixes #270) and keeps Java 8 compatibility.
The result is a single JAR that works for Java 8 and 9.

It is based on PR #593, which uses proxies/reflection in TextFlowExt to access private Java 9 classes/methods and runs without any special Java 9 runtime switches.

This PR extends #593 by also using proxies/reflection in TextFlowExt when running in Java 8.
Additionally there is a class JavaFXCompatibility that handles two API differences between Java 8 and 9.

I've added a method cache to class GenericIceBreaker, which dramatically reduces the number of method lookups (Class..getDeclaredMethod(...) invocations).

Experimented also with a proxy object cache in GenericIceBreaker, but this reduced the number of proxy objects only by about 50% and proxy creation is very light weight. So there was no benefit at all.

Travis now also builds on oraclejdk9 using source/targetCompatibility 9 to detect Java 9 compatibility issues in source code early.

I've tested the PR with Markdown Writer FX running on Java 8 and 9 without any problems so far.

So I think the PR is ready for testing by others and/or merging.

@Jugen
Copy link
Collaborator

Jugen commented Oct 14, 2017

In JavaFXCompatibility you can remove the "new_EnumConverter" method and replace the two references to it in TextExt, see BORDER_TYPE and UNDERLINE_CAP, with the Java 8 compatible:

(StyleConverter<?, StrokeLineCap>) StyleConverter.getEnumConverter( StrokeLineCap.class )

Unfortunately there doesn't seem to be an equivalent for the SequenceConverter ?

@JordanMartinez
Copy link
Contributor

Looks like the builds passed, but the demo package failed to compile due to not using the compatibility class. See AppVeyor's build

@JFormDesigner
Copy link
Contributor Author

@Jugen thanks for the hint. Fixed. Have also not found something for SequenceConverter.

@JordanMartinez fixed the demo. Made class JavaFXCompatibility public, but actually I think this is not a good idea. IMO this should be an internal class and not show up in the Javadoc API.

Will check whether the problematic code (see below) in class TextHyperlinkArea and in class StyledTextArea is necessary at all:

// XXX: binding selectionFill to textFill,
// see the note at highlightTextFill
t.impl_selectionFillProperty().bind(t.fillProperty());

I've removed this code some time ago in PR #303. I'll try to remember why....

@JFormDesigner
Copy link
Contributor Author

Upps, build now fails when building Javadoc for class JavaFXCompatibility. I really should do a local build before committing....

@JordanMartinez
Copy link
Contributor

JordanMartinez commented Oct 14, 2017

t.impl_selectionFillProperty().bind(t.fillProperty());

That's needed so either the selection shape or the text's background fill CSS actually works correctly. I forget which.
Edit: For reference, see this code and this code

I agree that the class needs to be kept internal and not public. We should add a public API to do the same thing that TextHyperlinkArea#createStyledTextNode does on a TextExt node (so that developers are still free to use a custom-built Text object) but without exposing the internal class.

Perhaps such a thing could appear in the util package as a util class?

@JordanMartinez
Copy link
Contributor

Also, there is no 0.4.8-alpha version of testfx-junit. It stops at the 7th alpha release since only the core was changed to some degree.

@JFormDesigner
Copy link
Contributor Author

Also, there is no 0.4.8-alpha version of testfx-junit.

Hmm, maybe they created it recently, but testfx-junit-4.0.8-alpha.jar exists.

The problem in the oraclejdk9 build is that testfx-internal-java8-4.0.8-alpha.jar is used instead of the Java 9 version. Any idea how to fix this?

BTW any idea how to change the sourceCompatibility and targetCompatibility values in the gradle build to 9 when building with oraclejdk9? The idea was to compile/build against Java 9 API to detect compatibility issues...

@JordanMartinez
Copy link
Contributor

Hmm, maybe they created it recently, but testfx-junit-4.0.8-alpha.jar exists.

Really? I thought it existed at one point, but in TestFX's Gitter feed, someone said it didn't exist, so that's why I made that comment. In other words, if it does now, you're right.

The problem in the oraclejdk9 build is that testfx-internal-java8-4.0.8-alpha.jar is used instead of the Java 9 version. Any idea how to fix this?

Is that an issue with TestFX itself?

BTW any idea how to change the sourceCompatibility and targetCompatibility values in the gradle build to 9 when building with oraclejdk9? The idea was to compile/build against Java 9 API to detect compatibility issues...

I believe it's just

sourceCompatibility = 9 // or "9"
targetCompatibility = 9 // or "9"

…to java9-reflection

# Conflicts:
#	richtextfx-demos/src/main/java/org/fxmisc/richtext/demo/hyperlink/TextHyperlinkArea.java
#	richtextfx/src/main/java/org/fxmisc/richtext/StyledTextArea.java
…raclejdk9 to detect Java 9 compatibility issues in source code
@JFormDesigner
Copy link
Contributor Author

Is that an issue with TestFX itself?

Not sure. To fix it, I've added testfx-internal-java8-4.0.8-alpha.jar to build.gradlein commit 80ad8b6 if Gradle runs in a Java 9 VM.

Anyway, TestFX could avoid this by dropping testfx-internal-java8-4.0.8-alpha.jar and testfx-internal-java9-4.0.8-alpha.jar and change testfx-core-4.0.8-alpha.jar to a Multi-Release JAR (http://openjdk.java.net/jeps/238) that contains Java 8 and 9 code in a single JAR.

@JFormDesigner
Copy link
Contributor Author

updated initial comment with some useful information...

@Jugen
Copy link
Collaborator

Jugen commented Oct 17, 2017

I have tested this in my application as well (with Java 8 and 9) and it seems to be ok :-) Thank you !

The JRE just issues a warning when launching from a command prompt that illegal reflective access is being used and will be denied in a future release.

@JordanMartinez
Copy link
Contributor

The failed Mac build is another instance of #608. Otherwise, everything else builds fine.

@JordanMartinez
Copy link
Contributor

@afester I feel ready to merge this PR, but since you've opened #618, what are your thoughts?

@JFormDesigner
Copy link
Contributor Author

Have one final thing to discuss: what should we do with the package org.fxmisc.richtext.j9adapters?

It contains public classes and interfaces that are only used in class TextFlowExt.
Currently, I've excluded j9adapters from javadoc. So it does not show up in RichTextFX API docs, but is still visible in the IDE (e.g. auto-completion) and could be used by others.

Should we:

A) move all classes/interfaces from package j9adapters into class TextFlowExt as private nested classes (as @afester has partly done in #618)?

B) create a package org.fxmisc.richtext.internal (for RichTextFX internal public classes) and move it there (e.g. org.fxmisc.richtext.internal.j9adapters). Eclipse uses those internal packages heavily. The package could be used in the future also for other RichTextFX internal classes.

C) leave it where it is

D) other

@JordanMartinez
Copy link
Contributor

I think A makes sense. It's better to not leave it as it is (C) since it adds "noise" to the classes one could use and opens the possibility for misuse/abuse by developers. What would be D? 😄

The package could be used in the future also for other RichTextFX internal classes.

That could be done in the future, but only if we need to. Otherwise, it seems unnecessary in our current development of this project.

@JordanMartinez
Copy link
Contributor

The precise build is having issues even after restarting it. We'll ignore them for now since the build is deprecated anyways.

@JordanMartinez
Copy link
Contributor

@afester Since I want to respect @JFormDesigner's work and release a new version soon, could you please respond? I plan on merging this tomorrow and making a release soon thereafter.

@JordanMartinez JordanMartinez merged commit 3e039a3 into FXMisc:master Oct 20, 2017
@JordanMartinez
Copy link
Contributor

Thanks for your work!

@afester
Copy link
Collaborator

afester commented Oct 23, 2017

@JordanMartinez Sorry for the late answer - in any case, that definitely makes sense :-)

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.

Make RichTextFX work on Java 9
6 participants