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

8342565: [TestBug] StubTextLayout #1667

Open
wants to merge 21 commits into
base: master
Choose a base branch
from

Conversation

andy-goryachev-oracle
Copy link
Contributor

@andy-goryachev-oracle andy-goryachev-oracle commented Dec 20, 2024

Changed the StubTextLayout to use product PrismTextLayout with much simplified glyph layout (via stubbed fonts). The new layout assumes all the glyphs are squares of font size, while the bold type face produces wider glyphs (by 1 pixel). The default font size has changed from 10 to 12 to make it closer to win/linux.

This brings the test environment closer to the product configuration and expands the capabilities of our headless testing pipeline, which will be useful for upcoming behavior tests.

Existing tests have been adjusted/reworked mainly due to default font size change.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed (2 reviews required, with at least 1 Reviewer, 1 Author)

Issue

  • JDK-8342565: [TestBug] StubTextLayout (Enhancement - P4)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/1667/head:pull/1667
$ git checkout pull/1667

Update a local copy of the PR:
$ git checkout pull/1667
$ git pull https://git.openjdk.org/jfx.git pull/1667/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1667

View PR using the GUI difftool:
$ git pr show -t 1667

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1667.diff

Using Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented Dec 20, 2024

👋 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.

@openjdk
Copy link

openjdk bot commented Dec 20, 2024

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

@andy-goryachev-oracle andy-goryachev-oracle marked this pull request as draft January 8, 2025 19:02
@andy-goryachev-oracle andy-goryachev-oracle changed the title StubTextLayout 8342565: [TestBug] StubTextLayout Jan 24, 2025
@andy-goryachev-oracle andy-goryachev-oracle marked this pull request as ready for review January 27, 2025 17:50
@openjdk openjdk bot added the rfr Ready for review label Jan 27, 2025
@mlbridge
Copy link

mlbridge bot commented Jan 27, 2025

Webrevs

@bridgekeeper
Copy link

bridgekeeper bot commented Jan 27, 2025

@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 add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

@kevinrushforth
Copy link
Member

This pull request has been inactive for more than 4 weeks ...

This seems like a Skara bug to me. This PR was just taken out of Draft and has several recent commits pushed to it.

@andy-goryachev-oracle
Copy link
Contributor Author

This pull request has been inactive for more than 4 weeks

I doubt that.

@kevinrushforth
Copy link
Member

Since this touches product code (in a minor way), I'd like two reviewers.

/reviewers 2

@kevinrushforth kevinrushforth self-requested a review January 27, 2025 22:22
@openjdk
Copy link

openjdk bot commented Jan 27, 2025

@kevinrushforth
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).


private static double toViewPortLength(double prefHeight) {
// it would be better to calculate this from listView but there is no API for this
return prefHeight - 2;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think after this node is inside the scene tree and has a skin, you can just calculate that with:
prefHeight(-1) - snappedTopInset() - snappedBottomInset().
Something like that I already used.

Also, Viewport is one word, so should not be camel case.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

using prefHeight(-1) - snappedTopInset() - snappedBottomInset() breaks the original test, I'd rather avoid that (all I did was minor refactoring to move the logic and the comment into one place).

thanks for -> toViewportLength()

@Maran23
Copy link
Member

Maran23 commented Feb 4, 2025

I think this is a good change, completely utilizing that the text layout code is abstracted away for good.
Changes look good to me so far.
Some tests are hard to understand the changes, but that is a preexisting problem (especially if the name of the test is just rt_xxxx).
Not sure about the code style of using S, M or L as variable names (minor), so I will see what other reviewers say about this.

@andy-goryachev-oracle
Copy link
Contributor Author

S, M or L as variable names (minor)

the meaning should be obvious from the context (small, medium, large) and I don't want code like this to take two pages:

testScrollTo(360, 1, new Integer[] { S, S, S, S, S, S, S, S, S, S, S, S, S, S, S, S, S, S, S, S, S, S, S, S, S, S, S, S, S, S, S, S, S, S, S, S });

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rfr Ready for review
Development

Successfully merging this pull request may close these issues.

3 participants