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
