Re: RFR: 8323511 Scrollbar Click jumps inconsistent amount of pixels [v2]
On Sun, 17 Mar 2024 18:07:35 GMT, Johan Vos wrote: >> Florian Kirmaier has updated the pull request incrementally with one >> additional commit since the last revision: >> >> JDK-8323511 >> reverted accidental indentation chang > > modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/VirtualScrollBar.java > line 142: > >> 140: int index = firstVisibleCell.getIndex(); >> 141: if (newValue < oldValue) { >> 142: index = Math.max(0, index - 1); > > Why are the boundary checks (Math.max/min) removed here? The ``Math.max(0, index - 1)`` was introduced to fix an IndesOutOfBoundsException ([JDK-8311983](https://bugs.openjdk.org/browse/JDK-8311983)) for mouse clicks above the thumb, but it also introduced this bug (different scroll amount for clicks above and below thumb). This change fixes the handling of -1 cell indexes in a different way, but for that we have to pass it to ``VirtualFlow.scrollTo(int index)`` and then to ``VirtualFlow.tryScrollOneCell(int targetIndex, boolean downOrRight)``. Therefore it is necessary to remove this ``Math.max(..)`` call. - PR Review Comment: https://git.openjdk.org/jfx/pull/1326#discussion_r1528208036
Re: RFR: 8327471: RTLTextFlowCharacterIndexTest fails on Linux [v2]
On Thu, 14 Mar 2024 10:37:26 GMT, Karthik P K wrote: >> Because of the difference in the size of characters in default fonts in >> different OS, 2 tests were failing in `RTLTextFlowCharacterIndexTest`. >> >> Increased the scene width to accommodate all the characters as required for >> the test to validate HitInfo values in all the platforms. > > Karthik P K has updated the pull request incrementally with one additional > commit since the last revision: > > Review comments looks good - Marked as reviewed by angorya (Reviewer). PR Review: https://git.openjdk.org/jfx/pull/1399#pullrequestreview-1943358032
Integrated: 8327471: RTLTextFlowCharacterIndexTest fails on Linux
On Wed, 13 Mar 2024 05:17:40 GMT, Karthik P K wrote: > Because of the difference in the size of characters in default fonts in > different OS, 2 tests were failing in `RTLTextFlowCharacterIndexTest`. > > Increased the scene width to accommodate all the characters as required for > the test to validate HitInfo values in all the platforms. This pull request has now been integrated. Changeset: 4e2216bf Author:Karthik P K URL: https://git.openjdk.org/jfx/commit/4e2216bfef7f7c542957af01de01662e7060a06b Stats: 20 lines in 1 file changed: 1 ins; 0 del; 19 mod 8327471: RTLTextFlowCharacterIndexTest fails on Linux Reviewed-by: angorya - PR: https://git.openjdk.org/jfx/pull/1399
Integrated: 8324326: Update ICU4C to 74.2
On Tue, 12 Mar 2024 14:45:06 GMT, Hima Bindu Meda wrote: > Updated icu to v74.2. Sanity testing looks fine. Verified build on all > platforms This pull request has now been integrated. Changeset: ad3d44e2 Author:Hima Bindu Meda URL: https://git.openjdk.org/jfx/commit/ad3d44e27f8ffb90aad81497f0bba2b00f7a49aa Stats: 10765 lines in 123 files changed: 3601 ins; 1545 del; 5619 mod 8324326: Update ICU4C to 74.2 Reviewed-by: kcr, sykora - PR: https://git.openjdk.org/jfx/pull/1398
Re: RFR: 8311895: CSS Transitions [v10]
> Implementation of [CSS > Transitions](https://gist.github.com/mstr2/c72f8c9faa87de14926978f517a6018a). > > ### Example > > .button { > -fx-background-color: dodgerblue; > } > > .button:hover { > -fx-background-color: red; > -fx-scale-x: 1.1; > -fx-scale-y: 1.1; > > transition: -fx-background-color 0.5s ease, > -fx-scale-x 0.5s ease, > -fx-scale-y 0.5s ease; > } > > src="https://user-images.githubusercontent.com/43553916/184781143-0520fbfe-54bf-4b8d-93ac-834708e46500.gif"; > width="200"/> > > ### Limitations > This implementation supports both shorthand and longhand notations for the > `transition` property. However, due to limitations of JavaFX CSS, mixing both > notations doesn't work: > > .button { > transition: -fx-background-color 1s; > transition-easing-function: linear; > } > > This issue should be addressed in a follow-up enhancement. Michael Strauß has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 46 commits: - Merge branch 'master' into feature/css-transitions - Merge branch 'master' into feature/css-transitions - Add TransitionMediator - Test whether two Interpolatable instances are compatible - Merge branch 'master' into feature/css-transitions - Added documentation - Review changes - Review changes - Make interpolator fields final - Review changes - ... and 36 more: https://git.openjdk.org/jfx/compare/ad3d44e2...b76568c3 - Changes: https://git.openjdk.org/jfx/pull/870/files Webrev: https://webrevs.openjdk.org/?repo=jfx&pr=870&range=09 Stats: 4434 lines in 42 files changed: 4395 ins; 1 del; 38 mod Patch: https://git.openjdk.org/jfx/pull/870.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/870/head:pull/870 PR: https://git.openjdk.org/jfx/pull/870
Re: RFR: 8311895: CSS Transitions [v9]
On Tue, 14 Nov 2023 09:41:08 GMT, John Hendrikx wrote: >> Michael Strauß has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Test whether two Interpolatable instances are compatible > > modules/javafx.graphics/src/main/java/com/sun/javafx/css/TransitionTimer.java > line 179: > >> 177: if (newTimer.delay < 0) { >> 178: double adjustedDelay = nanosToMillis(newTimer.delay) * >> newTimer.reversingShorteningFactor; >> 179: newTimer.startTime = now + millisToNanos(adjustedDelay); > > I don't see the value of converting these back and forth to milliseconds. > Why not just: > > Suggestion: > > newTimer.startTime = now + newTimer.delay * > newTimer.reversingShorteningFactor; > > I checked the calculations, and it makes no difference (the millis aren't > rounded or anything), so you're just moving the decimal point before/after > doing a multiplication that doesn't care where the decimal point is. > > If there is a reason that I missed, then I think this really needs a comment. I've removed the conversions. > modules/javafx.graphics/src/main/java/com/sun/javafx/css/TransitionTimer.java > line 196: > >> 194: double frac = (double)(nanos - (millis * 1_000_000L)) / >> 1_000_000D; >> 195: return (double)millis + frac; >> 196: } > > This function seems to want to preserve extra decimals in some way. If the > original long has a magnitude that is so large that some least significant > digits get lost in the double conversion, then trying to add them later > (`millis + frac`) will not restore them. > > In other words, you can just do: > > Suggestion: > > private static double nanosToMillis(long nanos) { > return nanos / 1_000_000.0; > } > > > Or avoiding the (generally slower) division completely: > > Suggestion: > > private static double nanosToMillis(long nanos) { > return nanos * 0.000_001; > } > > > All the extra operations are only likely to introduce small errors. Changed as suggested. > modules/javafx.graphics/src/main/java/com/sun/javafx/css/TransitionTimer.java > line 207: > >> 205: long wholeMillis = (long)millis; >> 206: double frac = millis - (double)wholeMillis; >> 207: return wholeMillis * 1_000_000L + (long)(frac * 1_000_000D); > > I'd recommend keeping this simple (it seems to want to recover extra > significant digits, but that's not possible): > > Suggestion: > > return ((long)(millis * 1_000_000.0); Changed as suggested. > modules/javafx.graphics/src/main/java/com/sun/javafx/css/TransitionTimer.java > line 234: > >> 232: >> 233: Node node = (Node)timer.getProperty().getBean(); >> 234: node.fireEvent(new TransitionEvent(eventType, >> timer.getProperty(), elapsedTime)); > > minor: > Suggestion: > > long elapsedTime; > > // Elapsed time specification: > https://www.w3.org/TR/css-transitions-1/#event-transitionevent > if (eventType == TransitionEvent.RUN || eventType == > TransitionEvent.START) { > elapsedTime = Math.min(Math.max(-timer.delay, 0), > timer.duration); > } else if (eventType == TransitionEvent.CANCEL) { > elapsedTime = Math.max(0, timer.currentTime - > timer.startTime); > } else if (eventType == TransitionEvent.END) { > elapsedTime = timer.duration; > } else { > throw new IllegalArgumentException("eventType"); > } > > Node node = (Node)timer.getProperty().getBean(); > node.fireEvent(new TransitionEvent(eventType, > timer.getProperty(), Duration.millis(nanosToMillis(elapsedTime; Changed as suggested. > modules/javafx.graphics/src/main/java/javafx/css/StyleableDoubleProperty.java > line 111: > >> 109: private TransitionTimer timer = null; >> 110: >> 111: private static class TransitionTimerImpl extends >> TransitionTimer { > > I think you can simplify this. TransitionTimer does not need any of its > generic parameters if you provide an abstract method to get the property > (just the general interface), which is sufficient for the `getBean` call you > do on it, and for passing it along as the source of events. > > The constructor, `onUpdate` and `onStop` don't need the property parameter at > all if you make the `TransistionTimerImpl` non-static. > > I also have the feeling that instead of subclassing `TransitionTimer`, > implementing an interface, which acts as the adapter you need between the > general timer infrastructure and a specific property, would be more clean. > My main reason for thinking so is the akward way the timer is put to use: > > TransitionTimer.run(new TransitionTimerImpl(this, v), transition) > > ...calling a static method on `TransitionTimer` while providing a subclass > instance of it. > > It could then look
Re: RFR: 8311895: CSS Transitions [v11]
> Implementation of [CSS > Transitions](https://gist.github.com/mstr2/c72f8c9faa87de14926978f517a6018a). > > ### Example > > .button { > -fx-background-color: dodgerblue; > } > > .button:hover { > -fx-background-color: red; > -fx-scale-x: 1.1; > -fx-scale-y: 1.1; > > transition: -fx-background-color 0.5s ease, > -fx-scale-x 0.5s ease, > -fx-scale-y 0.5s ease; > } > > src="https://user-images.githubusercontent.com/43553916/184781143-0520fbfe-54bf-4b8d-93ac-834708e46500.gif"; > width="200"/> > > ### Limitations > This implementation supports both shorthand and longhand notations for the > `transition` property. However, due to limitations of JavaFX CSS, mixing both > notations doesn't work: > > .button { > transition: -fx-background-color 1s; > transition-easing-function: linear; > } > > This issue should be addressed in a follow-up enhancement. Michael Strauß has updated the pull request incrementally with one additional commit since the last revision: Changes per review - Changes: - all: https://git.openjdk.org/jfx/pull/870/files - new: https://git.openjdk.org/jfx/pull/870/files/b76568c3..22aa4d64 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx&pr=870&range=10 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=870&range=09-10 Stats: 17 lines in 1 file changed: 4 ins; 4 del; 9 mod Patch: https://git.openjdk.org/jfx/pull/870.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/870/head:pull/870 PR: https://git.openjdk.org/jfx/pull/870
Re: RFR: 8311895: CSS Transitions [v2]
On Mon, 31 Jul 2023 13:44:28 GMT, John Hendrikx wrote: >> Michael Strauß has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Make TransitionEvent final > > Some early feedback, it's a lot of code :) I've implemented @hjohn's suggestion of an interface between `TransitionTimer` and `StyleableProperty` with the new `TransitionMediator` class. This cleans up the code and separates the interacting components. This PR is now again ready for review. - PR Comment: https://git.openjdk.org/jfx/pull/870#issuecomment-2004638297