Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the JFXSlider component, which is a Material Design implementation of a JavaFX slider control. The changes introduce a new implementation with enhanced visual styling and animation features.
Changes:
- Replaced CSS styling for JFXSlider with new Material Design compliant styles including colored track, animated thumb, and value indicator
- Added new JFXSlider control class with support for custom value factories and indicator positioning
- Added new JFXSliderSkin class extending SliderSkin with Material Design animations and enhanced functionality
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| HMCL/src/main/resources/assets/css/root.css | Replaced old svg-slider styles with new jfx-slider Material Design styles including track, thumb, colored-track, slider-value, and animated-thumb components |
| HMCL/src/main/java/com/jfoenix/controls/JFXSlider.java | New slider control class with value factory support and indicator position property |
| HMCL/src/main/java/com/jfoenix/skins/JFXSliderSkin.java | New skin implementation with Material Design animations and custom rendering logic |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| package com.jfoenix.controls; | ||
|
|
||
| import com.jfoenix.converters.IndicatorPositionConverter; |
There was a problem hiding this comment.
The IndicatorPositionConverter class is imported but does not exist in the codebase. This will cause a compilation error. You need to create the com.jfoenix.converters.IndicatorPositionConverter class that implements StyleConverter<String, IndicatorPosition> to convert the CSS property "-jfx-indicator-position" values ("left" or "right") to the IndicatorPosition enum.
| /** | ||
| * @return the current slider value factory | ||
| */ | ||
| public final Callback<JFXSlider, StringBinding> getValueFactory() { | ||
| return valueFactory == null ? null : valueFactory.get(); | ||
| } | ||
|
|
||
| /** | ||
| * sets custom string binding for the slider text value | ||
| * | ||
| * @param valueFactory a callback to create the string value binding | ||
| */ |
There was a problem hiding this comment.
The documentation style is inconsistent within this file. Lines 72-89 use /// style comments while lines 99-110 use /** */ style Javadoc comments. For consistency with the rest of the JFoenix controls in this codebase (like JFXCheckBox), consider using the /// style consistently throughout the file.
| /** | |
| * @return the current slider value factory | |
| */ | |
| public final Callback<JFXSlider, StringBinding> getValueFactory() { | |
| return valueFactory == null ? null : valueFactory.get(); | |
| } | |
| /** | |
| * sets custom string binding for the slider text value | |
| * | |
| * @param valueFactory a callback to create the string value binding | |
| */ | |
| /// @return the current slider value factory | |
| public final Callback<JFXSlider, StringBinding> getValueFactory() { | |
| return valueFactory == null ? null : valueFactory.get(); | |
| } | |
| /// Sets a custom string binding for the slider text value. | |
| /// | |
| /// @param valueFactory a callback to create the string value binding |
| timeline.setRate(1); | ||
| timeline.play(); | ||
| }); | ||
| track.addEventHandler(MouseEvent.MOUSE_RELEASED, (event) -> { | ||
| timeline.setRate(-1); | ||
| timeline.play(); | ||
| }); | ||
| thumb.addEventHandler(MouseEvent.MOUSE_PRESSED, (event) -> { | ||
| timeline.setRate(1); | ||
| timeline.play(); | ||
| }); | ||
| thumb.addEventHandler(MouseEvent.MOUSE_RELEASED, (event) -> { | ||
| timeline.setRate(-1); | ||
| timeline.play(); |
There was a problem hiding this comment.
The timeline is used in these event handlers but it's not initialized until the first layoutChildren call (line 153). If a user interacts with the slider before the first layout pass, this will cause a NullPointerException. Consider checking if timeline is null before using it, or initializing the timeline earlier in the constructor.
| timeline.setRate(1); | |
| timeline.play(); | |
| }); | |
| track.addEventHandler(MouseEvent.MOUSE_RELEASED, (event) -> { | |
| timeline.setRate(-1); | |
| timeline.play(); | |
| }); | |
| thumb.addEventHandler(MouseEvent.MOUSE_PRESSED, (event) -> { | |
| timeline.setRate(1); | |
| timeline.play(); | |
| }); | |
| thumb.addEventHandler(MouseEvent.MOUSE_RELEASED, (event) -> { | |
| timeline.setRate(-1); | |
| timeline.play(); | |
| if (timeline != null) { | |
| timeline.setRate(1); | |
| timeline.play(); | |
| } | |
| }); | |
| track.addEventHandler(MouseEvent.MOUSE_RELEASED, (event) -> { | |
| if (timeline != null) { | |
| timeline.setRate(-1); | |
| timeline.play(); | |
| } | |
| }); | |
| thumb.addEventHandler(MouseEvent.MOUSE_PRESSED, (event) -> { | |
| if (timeline != null) { | |
| timeline.setRate(1); | |
| timeline.play(); | |
| } | |
| }); | |
| thumb.addEventHandler(MouseEvent.MOUSE_RELEASED, (event) -> { | |
| if (timeline != null) { | |
| timeline.setRate(-1); | |
| timeline.play(); | |
| } |
| -fx-background-radius: 50% 50% 50% 0%; | ||
| -fx-background-insets: 0; | ||
| } | ||
|
|
There was a problem hiding this comment.
The previous CSS included a disabled state styling (.jfx-slider:disabled with opacity 0.4) which has been removed. Consider adding a disabled state style to provide visual feedback when the slider is disabled, consistent with other UI controls in the application.
| .jfx-slider:disabled { | |
| -fx-opacity: 0.4; | |
| } |
| if (((JFXSlider) getSkinnable()).getValueFactory() != null) { | ||
| sliderValue.textProperty() | ||
| .bind(((JFXSlider) getSkinnable()).getValueFactory().call((JFXSlider) getSkinnable())); | ||
| } else { | ||
| sliderValue.textProperty().bind(Bindings.createStringBinding(() -> { | ||
| if (getSkinnable().getLabelFormatter() != null) { | ||
| return getSkinnable().getLabelFormatter().toString(getSkinnable().getValue()); | ||
| } else { | ||
| return String.valueOf(Math.round(getSkinnable().getValue())); | ||
| } | ||
| }, getSkinnable().valueProperty())); |
There was a problem hiding this comment.
There are multiple casts to (JFXSlider) throughout the method. Consider storing a reference to the JFXSlider in a local variable or class field (e.g., 'JFXSlider jfxSlider = (JFXSlider) getSkinnable();') to improve code readability and reduce duplication.
| if (((JFXSlider) getSkinnable()).getValueFactory() != null) { | |
| sliderValue.textProperty() | |
| .bind(((JFXSlider) getSkinnable()).getValueFactory().call((JFXSlider) getSkinnable())); | |
| } else { | |
| sliderValue.textProperty().bind(Bindings.createStringBinding(() -> { | |
| if (getSkinnable().getLabelFormatter() != null) { | |
| return getSkinnable().getLabelFormatter().toString(getSkinnable().getValue()); | |
| } else { | |
| return String.valueOf(Math.round(getSkinnable().getValue())); | |
| } | |
| }, getSkinnable().valueProperty())); | |
| JFXSlider jfxSlider = (JFXSlider) getSkinnable(); | |
| if (jfxSlider.getValueFactory() != null) { | |
| sliderValue.textProperty() | |
| .bind(jfxSlider.getValueFactory().call(jfxSlider)); | |
| } else { | |
| sliderValue.textProperty().bind(Bindings.createStringBinding(() -> { | |
| if (jfxSlider.getLabelFormatter() != null) { | |
| return jfxSlider.getLabelFormatter().toString(jfxSlider.getValue()); | |
| } else { | |
| return String.valueOf(Math.round(jfxSlider.getValue())); | |
| } | |
| }, jfxSlider.valueProperty())); |
No description provided.