jfx
jfx copied to clipboard
openjdk
8341438: TextFlow: incorrect caretShape(), hitTest(), rangeShape() with non-empty padding/border
Summary
This change adds new methods to the TextFlow which work correctly in the presence of non-empty insets (borders/padding). For backward compatibility, the old buggy methods are getting deprecated (not for removal). Also, adds new methods in Text which provide missing functionality.
Problem
A number of methods in TextFlow fail to return correct values in the presence of non-empty insets (i.e. when either padding or border are set):
- caretShape
- hitTest
- rangeShape
Additionally, the current API fail to provide strike-through shape, and account for line spacing in the range shape, a problem shared between the TextFlow and the Text classes.
Solution
The solution is two-fold:
- deprecate the buggy methods (not for removal), adding explanations in their javadoc comments
- add new methods that behave correctly in the presence of non-empty insets and/or implementing the missing functionality.
The proposed solution retains the buggy methods for the purposes of backward compatibility in applications which employ the workarounds, while providing new APIs with additional parameters similar to those offered by the new TextLayout API https://bugs.openjdk.org/browse/JDK-8341670
Testing
Additional visualization of the data returned by the new APIs is available in the Monkey Tester using the following branch (in the Text and TextFlow pages):
https://github.com/andy-goryachev-oracle/MonkeyTest/tree/text.insets.corrected
Progress
- [x] Change must not contain extraneous whitespace
- [ ] Change requires CSR request JDK-8358000 to be approved
- [x] Commit message must refer to an issue
- [ ] Change must be properly reviewed (2 reviews required, with at least 1 Reviewer, 1 Author)
Issues
- JDK-8341438: TextFlow: incorrect caretShape(), hitTest(), rangeShape() with non-empty padding/border (Bug - P4)
- JDK-8357594: Text/TextFlow: strike through shape (Enhancement - P4)
- JDK-8358000: Enhanced Text/TextFlow APIs (CSR)
- JDK-8358000: Enhanced Text/TextFlow APIs (CSR)
Reviewing
Using git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/1817/head:pull/1817
$ git checkout pull/1817
Update a local copy of the PR:
$ git checkout pull/1817
$ git pull https://git.openjdk.org/jfx.git pull/1817/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 1817
View PR using the GUI difftool:
$ git pr show -t 1817
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1817.diff
Using Webrev
:wave: Welcome back angorya! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.
![bridgekeeper[bot] avatar](https://avatars.githubusercontent.com/u/41768318?v=4 "Published by a pub.dev verified publisher"){kind=small}
@andy-goryachev-oracle This change now passes all automated pre-integration checks.
ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.
After integration, the commit message for the final commit will be:
8357594: Additional geometry-based Text/TextFlow APIs
Reviewed-by: kcr, mstrauss
You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.
At the time when this comment was updated there had been 2 new commits pushed to the master branch:
- 203c049a671ca00e6012dfedd6aa9848e2584b85: 8357714: AudioClip.play crash on macOS when loading resource from jar
- e0f8e720752aecffb9090d1a3b82317b518f94a6: 8345348: CSS media feature queries
Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.
➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.
⚠️ @andy-goryachev-oracle This pull request contains merges that bring in commits not present in the target repository. Since this is not a "merge style" pull request, these changes will be squashed when this pull request in integrated. If this is your intention, then please ignore this message. If you want to preserve the commit structure, you must change the title of this pull request to Merge <project>:<branch> where <project> is the name of another project in the OpenJDK organization (for example Merge jdk:master).
@andy-goryachev-oracle Command syntax:
/issue [add|remove] <id>[,<id>,...]/issue [add] <id>: <description>
Some examples:
/issue add JDK-1234567,4567890/issue remove JDK-4567890/issue 1234567: Use this exact title
If issues are specified only by their ID, the title will be automatically retrieved from JBS. The project prefix (JDK- in the above examples) is optional. Separate multiple issue IDs using either spaces or commas.
@andy-goryachev-oracle has indicated that a compatibility and specification (CSR) request is needed for this pull request.
@andy-goryachev-oracle please create a CSR request for issue JDK-8341438 with the correct fix version. This pull request cannot be integrated until the CSR request is approved.
@andy-goryachev-oracle The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 1 Reviewer, 1 Author).
@andy-goryachev-oracle
Adding additional issue to issue list: 8357594: Text/TextFlow: strike through shape.
Webrevs
- 05: Full (a283479f)
- 04: Full - Incremental (a2099f02)
- 03: Full - Incremental (d466ebc9)
- 02: Full (16ed27eb)
- 01: Full (236c6af1)
- 00: Full (c1d7029c)
@andy-goryachev-oracle This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply issue a /touch or /keepalive command to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!
@andy-goryachev-oracle The pull request is being re-evaluated and the inactivity timeout has been reset.
@Ziad-Mid and @arapte could you take a look at this PR please?
I tried it, the tests pass successfully however when trying in MonkeyTester I am not able to show characters and caret path when I check the box it throws an error :
Exception in thread "JavaFX Application Thread" java.lang.NullPointerException: Cannot invoke "com.oracle.tools.fx.monkey.util.TextShapeLogic.ordinal()" because "logic" is null
(https://github.com/andy-goryachev-oracle/MonkeyTest/tree/text.insets.corrected)
when trying in MonkeyTester
Please update (git pull) the monkey tester and retry (or just select a value in the "API:" combo box.
I'm a bit skeptical with this approach. You describe the existing methods as "buggy". If that's the case, being good stewards of the API should point us towards fixing the bugs instead of adding new methods that seem confusingly similar.
The JBS ticket includes the following information:
This bug might present a compatibility risk for existing applications which tried to compensate for it by getting the borders and padding from the corresponding Nodes and adding it to each path element (or setting translateX/Y).
Is this just a guess, or do you have any data to corroborate the problem? Messing up the API and baking "buggy" implementations into JavaFX forever should require quite a bit of justification.
Have alternatives been discussed? For example, we could have a system property to fall back to the old behavior.
Have alternatives been discussed?
Yes, internally. This PR has been out for quite a while, too.
The priority here is not to break the existing applications that might have coded a workaround. We offer a new set of (better named) APIs withe the right behavior, while deprecating (not for removal) the existing ones.
Adding a system property seems a worse choice since it will break the apps and force the change.
Have alternatives been discussed?
Yes, internally. This PR has been out for quite a while, too.
Okay... the OpenJFX project states in CONTRIBUTING.md, section "New features / API additions": Discuss the proposed feature on the openjfx-dev mailing list.
I'm not sure how "internal" discussions qualify. Oracle is not the only stakeholder in this project.
The priority here is not to break the existing applications that might have coded a workaround.
My question was whether this is an educated guess, or whether this problem is corroborated by data.
Point taken. Posted to the mailing list.
My question was whether this is an educated guess
It's an educated guess.
Question: should this method be deprecated to be consistent with TextFlow?
Namely, deprecate the following four methods, adding getXxxxx methods as replacements
I am open to suggestions, but currently leaning toward leaving Text as is. As you correctly mentioned, it is not functionally required, and would only add more noise.
I feel making Text such a heavy object (with properties and all) might have been a design mistake: it would have been much better to use TextFlow everywhere, including Labels, TextFields, and TextAreas, and use Text as immutable objects similar to GlyphList to store runs of characters with the same font and attributes. Doing so might have allowed easy introduction of rich text into these controls too, but oh well.
@andy-goryachev-oracle
Removing additional issue from issue list: 8357594.
Made https://bugs.openjdk.org/browse/JDK-8357594 to be the main issue, closed https://bugs.openjdk.org/browse/JDK-8341438 as a duplicate of JDK-8357594 .
Going to push as commit 04c5e40cc116cb42150572959b53d1e465700e0e.
Since your change was applied there have been 2 commits pushed to the master branch:
- 203c049a671ca00e6012dfedd6aa9848e2584b85: 8357714: AudioClip.play crash on macOS when loading resource from jar
- e0f8e720752aecffb9090d1a3b82317b518f94a6: 8345348: CSS media feature queries
Your commit was automatically rebased without conflicts.
@andy-goryachev-oracle Pushed as commit 04c5e40cc116cb42150572959b53d1e465700e0e.
:bulb: You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.