8320359: ImageView: add styleable fitWidth, fitHeight, preserveRatio, smooth properties #1293

andy-goryachev-oracle · 2023-11-21T00:01:14Z

Adding missing styleable properties to ImageView:

-fx-preserve-ratio
-fx-smooth
-fx-fit-width
-fx-fit-height

Updated CSS Reference.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue
Change requires CSR request JDK-8321126 to be approved

Issues

JDK-8320359: ImageView: add styleable fitWidth, fitHeight, preserveRatio, smooth properties (Enhancement - P4)
JDK-8321126: ImageView: add styleable fitWidth, fitHeight, preserveRatio, smooth properties (CSR)

Reviewers

Michael Strauß (@mstr2 - Committer)
John Hendrikx (@hjohn - Committer)
Kevin Rushforth (@kevinrushforth - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1293

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

Using diff file

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

Webrev

Link to Webrev Comment

andy-goryachev-oracle · 2023-11-21T00:01:25Z

/csr

bridgekeeper · 2023-11-21T00:02:37Z

👋 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 · 2023-11-21T00:03:26Z

@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-8320359 with the correct fix version. This pull request cannot be integrated until the CSR request is approved.

mstr2 · 2023-11-22T02:23:08Z

modules/javafx.graphics/src/main/java/javafx/css/CssMetaData.java

+     *
+     * @since 22
+     */
+    public static List<CssMetaData<? extends Styleable, ?>> initStyleables(


I think this enhancement shouldn't be a part of this PR.

created JDK-8320796

mlbridge · 2023-11-30T16:23:52Z

Webrevs

mstr2 · 2023-11-30T16:50:53Z

modules/javafx.graphics/src/main/java/com/sun/javafx/css/CssUtil.java

+/**
+ * Utility methods for dealing with CSS.
+ */
+public class CssUtil {


I think it's best for utility classes to make it final and the constructor private.

this class is not accessible to the outside world, but sure, will do.

hjohn

LGTM

hjohn · 2023-11-30T23:02:10Z

modules/javafx.graphics/src/main/java/com/sun/javafx/css/CssUtil.java

+    }
+
+    /** immutable list with random access backed by an array */
+    private static class ImmutableArrayList<T> extends AbstractList<T> implements RandomAccess {


I'd like to point that a skeletal implementation of AbstractList will perform worse than ArrayList for any method that needs to walk the list, as the AbstractList will use iterators for methods like indexOf, hashCode and equals. Now that will probably be irrelevant, but as this seems to be a micro optimization, you should be aware of all the trade offs.

Good point, thanks!
This also applies to UnmodifiableArrayList.

For completeness sake, I wanted to mention a few issues (not in scope for this PR) that came out of the code review:

could use UnmodifiableArrayList but it stores extra int size. perhaps a factory method can be added to it for when size != elements.length.

could improve UnmodifiableArrayList with fast(er) indexOf, hashCode, equals per @hjohn 's comment earlier

Control.getCssMetaData() can be improved to skip merging of two lists if skinBase is null

the same immutable list should be used in Control.getCssMetaData() instead of ArrayList()

I wouldn't do any of the above unless there is a very good reason (and I'm not seeing one). Just use standard List.of as the last step (or use the Collections.unmodifableList wrapper); you'll get the most optimized, automatically maintained, bugfree, immutable List implementation Java has to offer. It means another copy will be made; that's fine, this only happens once -- it's not in a hot path.

If you feel like optimizing something, don't bother with Control.getCssMetaData either; instead, deduplicate the property lists so there is only one list per unique Skin + Control combo. That saves a complete list per control instance.

Good point, though I would still not use List.of() because of the unnecessary copy.
I agree on Skin + Control copy. Just not sure how... a static hash table perhaps?

I will approve this as is, but I agree with John that it would be better to use one of the existing implementations of List: new UnmodifiableArrayList, List.of, or Collections.unmodifiableList. The cost of the extra copy for List.of is in the noise compared to the benefit of not having to maintain yet another special case List class. Similarly, the extra word of data storage in UnmodifiableArrayList is in the noise. This is a one-per-class not one-per instance list.

kevinrushforth

Looks good. I left one HTML formatting comment on the newly added cssref.html docs (I also left an editorial comment that you should feel free to ignore).

I'll reapprove if and when you make the suggested doc formatting change.

kevinrushforth · 2023-12-01T17:34:05Z

modules/javafx.graphics/src/main/docs/javafx/scene/doc-files/cssref.html

-        <th class="propertyname" scope="row">-fx-image</th>
+          <th class="propertyname" scope="row">-fx-fit-height</th>
+          <td class="value"><a href="#typenumber" class="typelink">&lt;number&gt;</a></td>
+          <td>0</td>


Since you don't specify a style class for the newly added properties, they will all be left justified. The existing null value for -fx-image specifies class="default" which centers it. I recommend either adding the class="default" to all of the newly added properties, or removing it from the default value of the existing fx-image property.

I'll reapprove if you want to make the change.

thank you! I am going to add class="default" as it allows for styling of the default column.

Two notes:

There are a few other places where class="default" is omitted (HBox for instance). We may want to fix it in a separate PR.

I don't think the center alignment is the right choice here, I would rather see this column left aligned. Perhaps we should fix the .default class as part of the enhancement.

kevinrushforth · 2023-12-01T17:35:27Z

modules/javafx.graphics/src/main/java/com/sun/javafx/css/CssUtil.java

+    }
+
+    /** immutable list with random access backed by an array */
+    private static class ImmutableArrayList<T> extends AbstractList<T> implements RandomAccess {


I will approve this as is, but I agree with John that it would be better to use one of the existing implementations of List: new UnmodifiableArrayList, List.of, or Collections.unmodifiableList. The cost of the extra copy for List.of is in the noise compared to the benefit of not having to maintain yet another special case List class. Similarly, the extra word of data storage in UnmodifiableArrayList is in the noise. This is a one-per-class not one-per instance list.

kevinrushforth

Looks good.

Once you update the CSR to match the HTML changes, I'll Review that and you can Finalize it.

andy-goryachev-oracle · 2023-12-01T19:13:24Z

Once you update the CSR

updated already

openjdk · 2023-12-04T21:46:05Z

@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:

8320359: ImageView: add styleable fitWidth, fitHeight, preserveRatio, smooth properties

Reviewed-by: mstrauss, jhendrikx, kcr

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 3 new commits pushed to the master branch:

092d5d2: 8314597: Deprecate for removal protected access methods in converters
036d81b: 8269921: TextFlow: listeners on bounds can throw NPE while computing text bounds
0d33417: 8320267: WebView crashes on macOS 11 with WebKit 616.1

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 · 2023-12-04T21:48:19Z

/integrate

openjdk · 2023-12-04T21:48:48Z

Going to push as commit aedc887.
Since your change was applied there have been 3 commits pushed to the master branch:

092d5d2: 8314597: Deprecate for removal protected access methods in converters
036d81b: 8269921: TextFlow: listeners on bounds can throw NPE while computing text bounds
0d33417: 8320267: WebView crashes on macOS 11 with WebKit 616.1

Your commit was automatically rebased without conflicts.

openjdk · 2023-12-04T21:48:59Z

@andy-goryachev-oracle Pushed as commit aedc887.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

added styleable properties

91025ab

openjdk bot added the csr Need approved CSR to integrate pull request label Nov 21, 2023

andy-goryachev-oracle added 4 commits November 20, 2023 16:05

whitespace

b9db283

node

434568d

name

7ffe869

styleable

9b3d492

mstr2 reviewed Nov 22, 2023

View reviewed changes

andy-goryachev-oracle added 6 commits November 27, 2023 08:08

no nulls

3119050

Merge remote-tracking branch 'origin/master' into 8320359.image.view.css

d43ca42

Merge remote-tracking branch 'origin/master' into 8320359.image.view.css

983bb25

css util

7e01f0f

cleanup

eb4485b

cleanup

49e5bc6

andy-goryachev-oracle marked this pull request as ready for review November 30, 2023 16:18

openjdk bot added the rfr Ready for review label Nov 30, 2023

mstr2 approved these changes Nov 30, 2023

View reviewed changes

review comments

a6b772c

mstr2 approved these changes Nov 30, 2023

View reviewed changes

save 8 bytes

f735b8b

hjohn approved these changes Nov 30, 2023

View reviewed changes

hjohn reviewed Nov 30, 2023

View reviewed changes

kevinrushforth self-requested a review December 1, 2023 13:55

kevinrushforth approved these changes Dec 1, 2023

View reviewed changes

andy-goryachev-oracle added 2 commits December 1, 2023 10:36

class = default

f720ddb

list of

69761b4

andy-goryachev-oracle requested a review from hjohn December 1, 2023 18:56

andy-goryachev-oracle requested review from kevinrushforth and mstr2 December 1, 2023 18:56

kevinrushforth approved these changes Dec 1, 2023

View reviewed changes

mstr2 approved these changes Dec 1, 2023

View reviewed changes

hjohn approved these changes Dec 2, 2023

View reviewed changes

openjdk bot added ready Ready to be integrated and removed csr Need approved CSR to integrate pull request labels Dec 4, 2023

openjdk bot added the integrated Pull request has been integrated label Dec 4, 2023

openjdk bot closed this Dec 4, 2023

openjdk bot removed ready Ready to be integrated rfr Ready for review labels Dec 4, 2023

andy-goryachev-oracle deleted the 8320359.image.view.css branch December 4, 2023 21:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8320359: ImageView: add styleable fitWidth, fitHeight, preserveRatio, smooth properties #1293

8320359: ImageView: add styleable fitWidth, fitHeight, preserveRatio, smooth properties #1293

andy-goryachev-oracle commented Nov 21, 2023 •

edited by openjdk bot

andy-goryachev-oracle commented Nov 21, 2023

bridgekeeper bot commented Nov 21, 2023

openjdk bot commented Nov 21, 2023

mstr2 Nov 22, 2023

andy-goryachev-oracle Nov 27, 2023

mlbridge bot commented Nov 30, 2023 •

edited

mstr2 Nov 30, 2023

andy-goryachev-oracle Nov 30, 2023

hjohn left a comment

hjohn Nov 30, 2023

andy-goryachev-oracle Nov 30, 2023

hjohn Dec 1, 2023

andy-goryachev-oracle Dec 1, 2023

kevinrushforth Dec 1, 2023

kevinrushforth left a comment

kevinrushforth Dec 1, 2023

andy-goryachev-oracle Dec 1, 2023

kevinrushforth Dec 1, 2023

kevinrushforth left a comment

andy-goryachev-oracle commented Dec 1, 2023

openjdk bot commented Dec 4, 2023

andy-goryachev-oracle commented Dec 4, 2023

openjdk bot commented Dec 4, 2023

openjdk bot commented Dec 4, 2023

8320359: ImageView: add styleable fitWidth, fitHeight, preserveRatio, smooth properties #1293

8320359: ImageView: add styleable fitWidth, fitHeight, preserveRatio, smooth properties #1293

Conversation

andy-goryachev-oracle commented Nov 21, 2023 • edited by openjdk bot

Progress

Issues

Reviewers

Reviewing

Webrev

andy-goryachev-oracle commented Nov 21, 2023

bridgekeeper bot commented Nov 21, 2023

openjdk bot commented Nov 21, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mlbridge bot commented Nov 30, 2023 • edited

Webrevs

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hjohn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinrushforth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinrushforth left a comment

Choose a reason for hiding this comment

andy-goryachev-oracle commented Dec 1, 2023

openjdk bot commented Dec 4, 2023

andy-goryachev-oracle commented Dec 4, 2023

openjdk bot commented Dec 4, 2023

openjdk bot commented Dec 4, 2023

andy-goryachev-oracle commented Nov 21, 2023 •

edited by openjdk bot

mlbridge bot commented Nov 30, 2023 •

edited