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

Make RichTextFX work on Java 9 #270

Closed
johnmiddlekauff opened this issue Mar 19, 2016 · 42 comments · Fixed by #614
Closed

Make RichTextFX work on Java 9 #270

johnmiddlekauff opened this issue Mar 19, 2016 · 42 comments · Fixed by #614
Assignees
Milestone

Comments

@johnmiddlekauff
Copy link

Given that RichTextFX uses private APIs in JavaFX 8, will the current functionality provided by RichTextFX be possible under JavaFX 9?

@alt-grr
Copy link

alt-grr commented Mar 19, 2016

http://blog.codefx.org/java/dev/javafx-project-jigsaw-jep-253/

tl; dr; Should be possible, but not without refactoring and releasing new major version of RichTextFX.

@JordanMartinez
Copy link
Contributor

Should be possible, but not without refactoring and releasing new major version of RichTextFX.

Aye, and a new release for all of its dependencies: Flowless, ReactFX, UndoFX, WellBehavedFX.

@afester
Copy link
Collaborator

afester commented Feb 22, 2017

I have started a Java9 branch at https://github.com/afester/RichTextFX/tree/Java9. This branch contains the required changes to make the code compilable with Java 9-ea+155.

Due to the enhanced jigsaw authorization mechanisms at runtime, the following command line parameters are currently required to launch the demo applications (I only tried the JavaKeywords demo so far):

--add-exports javafx.graphics/com.sun.javafx.text=ALL-UNNAMED 
--add-exports javafx.graphics/com.sun.javafx.scene.text=ALL-UNNAMED 
--add-exports javafx.graphics/com.sun.javafx.geom=ALL-UNNAMED 
--add-opens javafx.graphics/javafx.scene.text=ALL-UNNAMED 
--add-opens javafx.graphics/com.sun.javafx.text=ALL-UNNAMED

Without these, the application will throw java.lang.IllegalAccessError and/or java.lang.reflect.InaccessibleObjectException exceptions when accessing protected methods through reflection.

The demo can then be launched, but there is still a rendering issue - the rich text is only partially rendered in the view. When clicking into a particular line, that line is then properly rendered. This might also be an issue of the FlowLess or other component - I have NOT recompiled any other dependency with Java 9 so far.

@JordanMartinez
Copy link
Contributor

I'll probably be looking into this more as we get closer to Java 9's release.

@JordanMartinez
Copy link
Contributor

Here's something to think about: how should this project be modulated? How many modules should there be and what components should be within each one? Which ones will be required and which are optional?

@JordanMartinez JordanMartinez changed the title Will RichTextFX be possible on JavaFX 9? Make RichTextFX work on Java 9 Oct 3, 2017
@JordanMartinez
Copy link
Contributor

I'll be looking at this with the help of #593 once #596 gets merged.

@JordanMartinez JordanMartinez added this to the 0.7-M8 milestone Oct 4, 2017
@PavelTurk
Copy link
Contributor

Could anyone say if richtextfx can be used on Java 9? I cloned master branch but can't build it as I get package com.sun.javafx.css.converters does not exist and other errors. Do I do something wrong?

@JordanMartinez
Copy link
Contributor

RichTextFX does not work on Java 9 yet. We're 1 issue away from releasing the 0.7-M7 release, and I'll be focusing on this issue once that gets done. Feel free to look at #593 as a temporary workaround.

@PavelTurk
Copy link
Contributor

@JordanMartinez Thank you very much for the information. Can you say when approximately RichTextFX will be ready for Java 9?

@JordanMartinez
Copy link
Contributor

@PashaTurok I'd like to have it done within two weeks at the very latest, but it depends. I hope to follow the same approach I used in TestFX (see #593 where I mention it there), so that we can share the code base as much as possible between the two different versions of Java. I'm not sure how easy/hard that will be.

@JordanMartinez
Copy link
Contributor

I've been thinking more about this. When I initially submitted my PR to TestFX to make it support Java 9, we didn't know that the Java release cycle would increased in speed to only 6 months ish. Now that we do know that, I wonder if Java 8 people will be forced to upgrade sooner (in a few months) rather than later (in a few years), making the dual support for both versions somewhat pointless after a while.

My idea is not to ignore Java 8 completely. Rather, I think we should duplicate our current project and then modify the duplicate to work on Java 9. I know it will take longer to submit PRs if we want to change a single thing in the project, but it does allow immediate trouble-free access to Java 9 support. My only question is how to distinguish the release versions from a Java 8 and 9 build. Thoughts?

@PavelTurk
Copy link
Contributor

@JordanMartinez Could you explain what you mean that the Java release cycle would increased in speed to only 6 months ish.? I've read several times but still can't understand the idea.

@Jugen
Copy link
Collaborator

Jugen commented Oct 11, 2017

@PashaTurok See this blog post "Moving Java Forward Faster" by Mark Reinhold the Chief Architect of the Java Platform Group at Oracle where he specifically talks about this.

@afester
Copy link
Collaborator

afester commented Oct 11, 2017

@JordanMartinez I would not duplicate the project. I would propose

  • either an approach where we continue to provide new features and bug fixes on the master branch which will then be supported on Java 9 only. The (current) Java 8 codebase would essentially be frozen on a Java 8 branch - that would even make merging bugfixes and/or single enhancements over to Java 8 much simpler.

  • Or use the adapter approach you described above - which would mean that we have the code for both Java 8 and Java 9 on the master branch and provide some mechanism to selectively compile one of them (one of the use cases where I really miss conditional compilation like #ifdef in Java, btw ...)

I wonder if Java 8 people will be forced to upgrade sooner

Probably not. Not sure who uses RichTextFX besides the projects listed on the wiki, but often enough people still use older versions for a rather long time, probably even through special maintenance agreements. Then, there are also long term support releases. See also
http://www.oracle.com/technetwork/java/eol-135779.html

@JordanMartinez
Copy link
Contributor

Probably not. Not sure who uses RichTextFX besides the projects listed on the wiki, but often enough people still use older versions for a rather long time, probably even through special maintenance agreements.

Good point. In that case, we'll stick with the adapter approach. I think I was focusing too much on how to support Java 9 quickly (i.e. duplicate code, merge #593, we're done) rather than thoughtfully.

When I started to work through how the codebase would need to be split up to support 8 and 9 at once, it required three different modules due to TextFlowExt: richtextfx-internal-java8, richtextfx-internal-java9, and richtextfx-shared (its TwoDimensional dependency). My first impression was that this would take longer than I had hoped, but I haven't continued working on it since then.

@Jugen
Copy link
Collaborator

Jugen commented Oct 11, 2017

I basically second @afester 's proposal as well, with the following notes for the second option:

I think its possible to compile both the Java 8 and 9 code together (modules aren't needed at first), and to then package them together in a single multi-release jar. This can be done by putting the Java 9 code into its own folder say "java9" (inside "richtextfx/src" or "main" ?) It'll have its own appropriate build.gradle file with for example sourceCompatibility = '1.9' (or 9?) and no jar entry.

The 'Multi-Release': true attribute needs to be added to the richtextfx jar { manifest section, as well as any needed 'Add-Opens' attribute entries such as 'java.base/java.lang'. Then somehow the Java 9 classes need to be included in the jar file under META-INF/versions/9

Not sure if this will help but here is a maven example.

@JFormDesigner
Copy link
Contributor

JFormDesigner commented Oct 11, 2017

Why no use reflection to fix the JavaFX incompatibilities in RichTextFX?

This does not require any change to the structure of the code base or to the build system and results in a single JAR that works with Java 8 and 9+.

Personally, I use some kind of "compatibility" classes (e.g. JavaFXCompatibility or IntelliJCompatibility). This makes it easy to later find places where compatibility issues exist and remove them if the old version should be no longer supported. E.g.

class JavaFXCompatibility
{
    /**
     * Java 8:  javafx.scene.layout.GridPane.impl_getCellBounds( int columnIndex, int rowIndex )
     * Java 9+: javafx.scene.layout.GridPane.getCellBounds( int columnIndex, int rowIndex )
     */
    static Bounds GridPane_getCellBounds( GridPane gridPane, int columnIndex, int rowIndex ) {
        try {
            if( mGridPane_getCellBounds == null ) {
                mGridPane_getCellBounds = GridPane.class.getMethod(
                    JavaVersion.IS_JAVA_9_OR_LATER ? "getCellBounds" : "impl_getCellBounds",
                        int.class, int.class );
            }
            return (Bounds) mGridPane_getCellBounds.invoke( gridPane, columnIndex, rowIndex );
        } catch( NoSuchMethodException | SecurityException | IllegalAccessException | IllegalArgumentException | InvocationTargetException ex ) {
            throw new Error(ex);
        }
    }

    private static Method mGridPane_getCellBounds;
}

I compiled RichTextFX core (without demos and tests) with Java 9 and found out that there are 3 kinds of incompatibilities:

  1. two changed package names in class TextExt: private JavaFX 8 classes are public in JavaFX 9
  2. one renamed method in class StyledTextArea: removed "impl_"
  3. access private JavaFX classes/methods in class TextFlowExt

1st and 2nd could be solved with reflection (similar to above example).

For 3rd, the solution in #593 for JavaFX 9 could be adapted so that it also works for JavaFX 8. #593 uses some new JavaFX 9 methods in TextFlowExt.getCaretShape(), TextFlowExt.getRangeShape() and TextFlowExt.hit(), which could be solved with reflection. And the classes in package org.fxmisc.richtext.j9adapters should also work with Java 8 IMHO.

@JordanMartinez
Copy link
Contributor

@Jugen Thanks for your input. I've been waiting for The Java Module System to release a chapter on multi-release JARs. I'll look at your example when I have time (hopefully later this week).

@JFormDesigner Good points, too. What are the down sides to using reflection as opposed to accessing things directly? We currently use reflection to get the TextLayout, but is there any benefit to accessing such things directly (e.g. getCaretShape()) rather than by reflection in Java 9? Unfortunately, reflection is still a topic I need to invest time into learning well.

Otherwise, it seems that this questions has become: "Do we use a 3-module project structure to build a Java 8 and 9 version of the code? Or just reflection + #593? What are the pros/cons of each and which is the best direction for this project?"

@Jugen
Copy link
Collaborator

Jugen commented Oct 13, 2017

BTW (for those that may not know :) the module system and multi-release jar files are two separate things that are mostly independent of one another. Multi-Release JAR Files are really very simple requiring only two steps:

  1. Adding Multi-Release: true to the manifest file, and
  2. Adding the classes for Java 9 and up in its appropriate folder under META-INF/versions
    (The jar tool, see under examples, can do this for you)

Multi-release jar files are backwards compatible so any previous JRE will have no trouble with them :-)

@Jugen
Copy link
Collaborator

Jugen commented Oct 13, 2017

Historically reflection is regarded as a last resort if there is no other way for two reasons:

  1. Reflection is (was?) slow. This can be mitigated by doing the lookup once and storing the reflection result in a static variable (if possible) just like in @JFormDesigner example above :-)
  2. There is no compile time safety. This need not be a problem though when a project has thorough test cases, just like RichTextFX :-)

There are I think two advantages for going the reflection route:

  1. There would only need to be one code base with no duplication of classes. So no ones going to accidentally update the wrong file because they didn't know, or forgot, that there is a Java 9 version in some other folder somewhere.
  2. Multi-release jar files won't be necessary.

All that being said though if it cannot ALL be done with reflection then imho it shouldn't be done at all, and the three classes mentioned by @JFormDesigner should be duplicated and every method in the Java 8 class documented to YELL that there's a Java 9 copy and its location :-)

@JordanMartinez
Copy link
Contributor

BTW (for those that may not know :) the module system and multi-release jar files are two separate things that are mostly independent of one another.

😭 I didn't know! lol!

There are I think two advantages for going the reflection route...

If we won't be releasing a multi-release jar, then how should a Java 8 release differ from a Java 9 release? In other words, how will the release versions be different? Will it be "Major.Minor.Patch-Java8" and its 9 counterpart?

All that being said though if it cannot ALL be done with reflection then imho it shouldn't be done at all...

In that case, would someone be willing to submit a PR using the reflection approach? I still plan to release the 0.7-M7 release without it (I'm still learning how to automate this build/release stuff, so I'd rather screw up on that one and learn from it so there are no issues when making a release with Java 9 support)

@JFormDesigner
Copy link
Contributor

...would someone be willing to submit a PR using the reflection approach?

Since I suggested reflection, I think I have to do it..

Have already rebased Mario's commit from #593 to current master and will continue there...
Let's see how this works. If it is not optimal, we can still use other solution...

Here is the branch:
https://github.com/JFormDesigner/RichTextFX/tree/java9-reflection

@Jugen
Copy link
Collaborator

Jugen commented Oct 14, 2017

@JordanMartinez

If we won't be releasing a multi-release jar, then how should a Java 8 release differ from a Java 9 release?

If the reflection route works then there'll only be one release, compiled with Java 8 but it'll also work in Java 9 :-)

@JordanMartinez
Copy link
Contributor

If the reflection route works then there'll only be one release, compiled with Java 8 but it'll also work in Java 9 :-)

Really? How then do you deal with the module-info.java files? Java 8 won't know what to do with them, so we'll need to ignore those files in the compilation. However, without them, won't we need to use runtime flags to add the appropriate access rights in Java 9? If we use a multi-release jar, won't this get around that?

@Jugen
Copy link
Collaborator

Jugen commented Oct 15, 2017

Yip, Java 9 does not enforce the use of modules yet, it's optional :-), so the module-info.java files aren't needed.

Any access right runtime flags will only be needed if API is being used that is not visible by default, in which case the 'flags' can be included in the main executable jar file without Java 8 complaining. There are two (flags) attributes:

  1. Add-Exports: <module>/<package>( <module>/<package>)* Enables access to the public types of a specified package.
  2. Add-Opens: <module>/<package>( <module>/<package>)* Enables access to all non-public elements

The value of each attribute is a space-separated list of slash-separated module-name/package-name pairs.

@JordanMartinez
Copy link
Contributor

JordanMartinez commented Oct 16, 2017

Java 9 does not enforce the use of modules yet, it's optional

Good to know. And it's also important to highlight the word yet.

Any access right runtime flags will only be needed if API is being used that is not visible by default, in which case the 'flags' can be included in the main executable jar file without Java 8 complaining.

So, if I build a Java 9 library and it uses a reflection-based compiled RTFX as its dependency, which won't have its own module-info.java file that can be found as a module, won't I need to include the flags for the library's compilation in gradle's compileJava step to add those kinds of module dependencies? If so, it's a minor convenience. And, arguing for the reflection side, perhaps the library will have to do that with other things as well, so "who cares?"
Also, if the flags still need to occur in the main executable jar, that's still a minor inconvenience.

I appreciate the PR you've submitted and the work you've done. I think the reflection approach is the easiest and fastest way to add support for Java 9 in a short time. However, I don't think it's the best long-term approach. Eventually, it seems that we will need to either migrate it to a more modular approach like what I did in TestFX (if we want to continue supporting Java 8) or continue with @afester's first approach (freezing the Java 8 branch and including development on Java 9 only).

Regardless, I don't want you to feel like you went through all that work for nothing. I realize that the migration to Java 9 will be itself a process, not an immediate step. So, part of me does want to "please you" by saying, "let's just use the reflection approach because it adds support for Java 9 so quickly (all we need to do is merge your PR and make a new release)." However, acknowledging that temptation within myself, I also realize that this approach, if taken, probably won't be the one we end with. I acknowledge that that is ok, since the migration will be a process. Still, if that's the case, I must ask, "Should it still be the route we take?"

While we could use @afester's first approach, the flaw there (as already stated) is the fact that some people who won't or can't upgrade may still need Java 8 support and additional bug fixes / enhancements. If we use the approach I did in TestFX, it requires messing with the build file for a bit and modulating the code across multiple (dare I say "boilerplate?") directories.

To summarize the pros and cons, let's use this table. If you find there are things missing, things that need to be corrected, or it doesn't fairly represent each route, please make note of it, so I can fix it. Considering the below, which route does the community want to go?

Reflection Freeze 8, Continue 9 "Boilerplate" directories/modules
Continuous Java 8 support Frozen Java 8 support (or developer forks) Continuous Java 8 support
Java 9 support Java 9 support Java 9 support
--- module-info.java included module-info.java included
may require executable jar flags --- ---
--- --- requires "boilerplate" directories / modules
--- --- requires modifying build.gradle's compileJava step to account for which JDK is compiling it
--- --- requires java8/java9 to be included in version string OR multi-release jar
--- --- requires modifying how releases are built and released via Gradle

Personally, I'm favoring a "Use the reflection approach now just to add Java 9 support and migrate to the 'boilerplate' approach later on."

@Jugen
Copy link
Collaborator

Jugen commented Oct 17, 2017

So, if I build a Java 9 library and it uses a reflection-based compiled RTFX as its dependency, .... won't I need to include the flags for the library's compilation in gradle's compile Java step to add those kinds of module dependencies?

No, you only need the flags at runtime and it seems only when accessing the API directly but not when it's via reflection. (So the table above can be updated accordingly ;-)

Ok, so there's good news and then there's bad news .....

The good news first is that as you've probably seen @JFormDesigner's reflection PR is working :-). The JRE just writes a warning message to the console (but otherwise there's no Exceptions or misbehavior):

WARNING: An illegal reflective access operation has occurred
WARNING: Illegal reflective access by org.fxmisc.richtext.TextFlowExt to method javafx.scene.text.TextFlow.getTextLayout()
WARNING: All illegal access operations will be denied in a future release

More good news is that a future release is probably a couple (?) of years away.

Now the BAD news is that TextFlowExt depends heavily on the offending TextLayout which it has ALWAYS obtained through reflection because TextFlow.getTextLayout() is private in both JDK 8 and 9. So unfortunately regardless of which of the three options above is chosen this problem won't go away and I'm afraid to say that there doesn't seem to be an elegant solution.

BTW the reflection that is being done via JavaFXCompatibility are none issues, because they are accessing public API, so Java 9 doesn't get indigestion from it.

@JordanMartinez
Copy link
Contributor

No, you only need the flags at runtime and it seems only when accessing the API directly but not when it's via reflection. (So the table above can be updated accordingly ;-)

What do I update? When I looked at the table, I realize that I forgot to add the compileJava flags part, so perhaps it doesn't need to be changed after all?

More good news is that a future release is probably a couple (?) of years away.

I'm assuming this is in regard to Java make a new release, correct? Yes, we have yet to see whether they hold true to their 6-month cycle update or whether it's just words again.

Now the BAD news is that TextFlowExt depends heavily on the offending TextLayout which it has ALWAYS obtained through reflection because TextFlow.getTextLayout() is private in both JDK 8 and 9. So unfortunately regardless of which of the three options above is chosen this problem won't go away and I'm afraid to say that there doesn't seem to be an elegant solution.

Thanks for the clarification. Since I see that the table comment above got 3 likes and no one else has said anything in regards to not using the "reflection + multi-release jar" approach, am I correct in understanding that this is the direction we wish to go, at least for now?

@Jugen
Copy link
Collaborator

Jugen commented Oct 17, 2017

What do I update?

I think just change to may require executable jar flags in the first column of the table.

Yip, it seems like just using reflection for now and then maybe later on if needed use multi-release jars.

@JordanMartinez
Copy link
Contributor

I think just change to may require executable jar flags in the first column of the table.

Done.

@JordanMartinez
Copy link
Contributor

As noted in TestFX/TestFX#433's comment, a person is discouraging the usage of multi-release jars. I've opened an issue in that repo: melix/mrjar-gradle#1

@JordanMartinez
Copy link
Contributor

Just FYI. Someone extended the experimental Java 9 plugin for Gradle via Chainsaw

@goxr3plus
Copy link
Collaborator

goxr3plus commented Mar 11, 2018

@JordanMartinez Hello dear friends , i am in love with this RichTextFX and i am using it on my projects , now i have passed most of them to Java 9 and the only problem i have is with RichTextFX .

Please how i can help on fixing that ? Is there any solution .

I have read most of the posts you have in this page . JFoenix and ControlsFX have elegantly released versions for both Java 9 and Java 8.

Waiting for response :) Java 10 is about to be released .

@Jugen
Copy link
Collaborator

Jugen commented Mar 12, 2018

There shouldn't be any problem using RTFX in / with Java 9. It just complains about the illegal reflection but otherwise should work.

If you are having a problem please open an issue detailing the problem you are having, and hopefully somebody will be able to help / guide you.

We don't currently have a solution for the use of TextFlow.getTextLayout() by TextFlowExt which is the major problem. So if you want too, you can have a look at that and see if you can come up with something.

@JordanMartinez
Copy link
Contributor

@goxr3plus What specifically is the problem you are experiencing? AFAIK, RichTextFX will stop working in JDK 10 because the reflection approach we're using will no longer be allowed. (I could be wrong about that.) I haven't submitted a fix to OpenJFX to expose what we need as public (or whatever other API changes that are needed) because they're still figuring out a better way to streamline changes on the openjfx-dev mailing list (see the threads by month: Feb, March).

@goxr3plus
Copy link
Collaborator

goxr3plus commented Mar 13, 2018

@Jugen @JordanMartinez Yes you are right :) . I was doing something wrong in other classes i just figured it out now . The whole project runs well except these warning as you mentioned .

I hope we can move RichTextFX successfully to Java 10 and beyond :)

WARNING: An illegal reflective access operation has occurred
WARNING: Illegal reflective access by org.fxmisc.richtext.TextFlowExt (file:/C:/Users/GOXR3PLUSSTUDIO/.m2/repository/org/fxmisc/richtext/richtextfx/0.8.2/richtextfx-0.8.2.jar) to method javafx.scene.text.TextFlow.getTextLayout()
WARNING: Please consider reporting this to the maintainers of org.fxmisc.richtext.TextFlowExt
WARNING: Use --illegal-access=warn to enable warnings of further illegal reflective access operations
WARNING: All illegal access operations will be denied in a future release

@Jugen
Copy link
Collaborator

Jugen commented Mar 13, 2018

Since we might need this sooner rather than later with Java 10 almost upon us (and 11 is scheduled for later this year !?) I've almost finished a Java 9 and up only version of TextFlowExt that doesn't use reflection. (Of course we might not even need this, but this is just in case and I had done this end of last year already ;-)

Just have to do the getUnderlineShape method and then it'll need extensive testing by everybody that's prepared to use alpha/beta code.

@JordanMartinez
Copy link
Contributor

@Jugen I'd like to respond to your comment, but it's getting outside of the scope of this issue. Can you repost what you've written here in a new issue with something like "Make RichTextFX work on Java 10" or something? RichTextFX does work on Java 9, so this issue is closed.

@Jugen
Copy link
Collaborator

Jugen commented Mar 22, 2018

Since RichTextFX seems to work on Java 10, my version isn't needed yet, so I'm shelving it for now and will check again for Java 11.

@JordanMartinez
Copy link
Contributor

Since RichTextFX seems to work on Java 10, my version isn't needed yet, so I'm shelving it for now and will check again for Java 11.

Oh, it does work on 10? That's good to hear. Perhaps by Java 11 the OpenJFX process will be figured out and the accessibility of TextLayout-related things will be resolved.

@saifalmutory
Copy link

Now can I use RichTextFX on Java 9 without any problems? With ignore the warning?
I searched a lot about the rich text area, RichTextFX is the only option for me.

@JordanMartinez
Copy link
Contributor

@saifalmutory Yes, it does work on Java 9. You'll need to pass some runtime args though. If you examine the build.gradle file and the .travis.yml's java 9 build, you can see how that works.

@Jugen Jugen self-assigned this Mar 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants