On Mon, 2 Sep 2024 16:24:33 GMT, Michael Strauß <mstra...@openjdk.org> wrote:
> Yes, it's a sizable chunk of the specification: > https://www.w3.org/TR/css-transitions-1/#starting Thanks, somehow I had trouble finding it. > The algorithm as prescribed by the specification is one of several algorithms > that were considered by the W3C working group back in 2013. It is clearly > tuned for two-state application scenarios, and might simply not be the right > solution for your problem. I've thought a lot about the problem, and while it > may be possible to come up with an algorithm that covers more edge cases, > that takes a huge toll on the complexity/maintainability budget. Alright, if CSS specifies it like this, then there's little point discussing improvements here. > I don't think this would work at all, because you're setting the value > programmatically. You'll only get a transition if you let the CSS system set > the value. If you do this, and then query the value of the property under > transition, you will get an intermediate value. This simply wouldn't work > otherwise, considering how JavaFX properties work. We can't have properties > change their value over time, but when you look at them programmatically, > they already have the target value. Yeah, I starting seeing this halfway through my review and playing with the code. It just doesn't apply to programmatically set values at all; my own work on throttled and animated bindings may have contributed to the confusion. > Yes, having an adapter between the styleable property implementation and the > transition timer was an improvement that [you > suggested](https://github.com/openjdk/jfx/pull/870#discussion_r1392551399); > it seems like we've come full circle. Sure, clearing the mediator reference > is not great, but it is a tiny and isolated part (as in, this implementation > detail doesn't leak out of the class in which it is used). I wouldn't call > this a serious problem of the design. It's been a while, and I didn't even remember suggesting that anymore, so imagine my surprise when I see my own suggestion reflected in `TransitionMediator`. My responses are usually triggered because of something in the solution that I find confusing or overly complicated. What I didn't like in the original was the subclassing and passing in that subclass to a static method of the same class. What I'm not liking in the current one is the married nature of the two classes, where a mediator is both tracked and passed to the timer. The conditional cancellation and reference clearing logic are the primary things that trigger me here, but also the new reversing logic where a mediator is passed in, then casted. I'm just making suggestions for you to think about (after spending hours looking at the code and trying to follow its logic). The suggestions usually originate from something in the code that is hard to follow or doing things in a seemingly roundabout way. They're suggestions as I may not be seeing all the edge cases, and you should be able to see much better than I if these suggestions have merit. I didn't quite see the conditional way you are clearing a reference in the previous PR (I may have missed it, or perhaps I didn't review it again afterwards). I still think there's room for improvement, which perhaps will allow you to eliminate some of the duplication and confusing parts: 1. You can have `TransitionTimer.run` return an object for cancellation. This is IMHO nicer than tracking an object you passed to it. What I'm further disliking here is that it returns a timer, which is already started, and which you're not supposed to start/stop, and which has an extra method `cancel` (which differs from `stop`). This would be better separated, either by hiding its `AnimationTimer` nature (by using composition) or by not returning a full timer. The opposite is also possible (ie. do expose its `AnimationTimer` nature, but then don't auto start it; have the caller start it. Use `stop` for cancels, not have two methods for similar purposes). 2. You can move the check if a timer should be started based on the target value to `TransitionTimer.run`; if the end value is the same as the timer is already aware off then have it return the existing timer. Callers of `TransitionTimer.run` could look something like: this.timer = TransitionTimer.run( this, newValue, this::callback, transitionDefinition, Toolkit.getToolkit().getPrimaryTimer().nanos() ); To find the startValue it can call `getValue`. For component transitionables, `equals` implementation should do a deep equals. Motivation: eliminates some of the code duplication in the property implementations and leaves the decision whether a new timer is started (or perhaps even internally re-purposed) up to `TransitionTimer`. 3. You can avoid the `cancel(false)` by calling `super.set` from a method other than `set`. For example: private void callback(double progress, double startValue, double endValue) { super.set(progress < 1 ? startValue + (endValue - startValue) * progress : endValue); if (progress >= 1) { timer = null; } } You can now always clear the timer reference in `set` and `bind`: private void clearTransition() { origin = StyleOrigin.USER; if (timer != null) { timer .cancel(true); timer = null; } } Motivation: timer management is a bit more compact and clear. Timers always get cancelled and cleared in `set` and `bind`, and the callback will know exactly when the timer has served its purpose. No need to do a reference equality check to conditionally clear the reference even for reversals. 4. Reversing should be determinable now within `TransitionTimer.run` instead of needing a custom implementation for each type. I think you may be able to avoid `deepEquals`, or have the wrapper around multiple properties handle this in its own `equals`. Motivation: removes ugly casting in `updateReversingAdjustedStartValue` 5. Not a point of improvement, but something that I think is poorly implemented currently in FX (similar to a post from a few weeks ago on the mailinglist). I see that you are passing in the nano time each time, and it seems we may have no choice to do this because `Toolkit.getToolkit().getPrimaryTimer().nanos()` is not returning a constant value within the scope of a single pulse. This is IMHO a serious oversight; what's the point of adding a random number of nanos depending on the exact time it is called during a pulse. This should have been constant, in which case the nanos could simply be obtained by `TransitionTimer.run` instead of having it passed in each time. You could consider providing an overloaded variant where the nanos are optional to avoid the ugly `Toolkit.getToolkit().getPrimaryTimer().nanos()` incantation except where you have no choice. > As for your ideas about refactoring the code to get rid of > `TransitionMediator`, I'd like to understand a bit more context because this > seems pretty random to me. Sure, I can imagine any number of implementations > that would do the job, but what is your main motivation for rejecting the > current implementation in favor for another approach? The purpose of > `TransitionMediator` and `TransitionTimer` seem sufficiently clear-cut to me, > and the implementation is reasonably clean given the complex problem being > solved. I don't see an _obvious_ shortcoming of the current implementation. They're only suggestions (or ideas as you call them). IMHO you as the author know best if there is merit in these suggestions. Perhaps a suggestion contains something you didn't think of allowing you to simplify the code a bit further. I've tried adding my reasoning now. As for the current implementation, I think it works, but it is a bit hard to see if all edge cases are covered. I did notice one thing while experimenting with the code that you may want to address. `StyleableProperty_transition_Test` doesn't seem to care for reversal logic. If I mess it up (always return `false` for one of the properties), the test still passes. ------------- PR Comment: https://git.openjdk.org/jfx/pull/1522#issuecomment-2325475048
