Skip to content

更新 JFXProgressBar 和 JFXSpinner#5565

Merged
Glavo merged 21 commits intoHMCL-dev:mainfrom
Glavo:tree-visible
Feb 18, 2026
Merged

更新 JFXProgressBar 和 JFXSpinner#5565
Glavo merged 21 commits intoHMCL-dev:mainfrom
Glavo:tree-visible

Conversation

@Glavo
Copy link
Member

@Glavo Glavo commented Feb 18, 2026

No description provided.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

该 PR 旨在更新 HMCL 内置的 JFoenix JFXSpinnerJFXProgressBar 的实现与样式,增加对“节点树是否正在显示(tree showing)”的感知,从而更合理地启停动画,并补充 determinate 模式下的百分比显示样式。

Changes:

  • 更新 root.css 中 Spinner 相关选择器与 determinate 百分比文本样式
  • 新增/更新 JFXSpinnerJFXProgressBar 及其 Skin,实现动画与布局逻辑
  • JFXNodeUtils 中引入 treeVisible/treeShowing 的工具能力,并新增 TreeShowingProperty 用于可观察的 showing 状态

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 8 comments.

Show a summary per file
File Description
HMCL/src/main/resources/assets/css/root.css 调整 spinner arc 选择器为后代选择器,新增 determinate 百分比文本颜色
HMCL/src/main/java/com/jfoenix/utils/TreeShowingProperty.java 新增可观察的 tree showing 属性(监听 scene/window/可见性链路)
HMCL/src/main/java/com/jfoenix/utils/JFXNodeUtils.java 新增 treeVisibleProperty/treeShowing 相关工具方法(包含反射/MethodHandle 路径)
HMCL/src/main/java/com/jfoenix/skins/JFXSpinnerSkin.java 新增 Spinner 皮肤:绘制 arc/track、百分比文本、并根据 showing 状态启停动画
HMCL/src/main/java/com/jfoenix/skins/JFXProgressBarSkin.java 新增 ProgressBar 皮肤:secondary progress 与 indeterminate 动画处理
HMCL/src/main/java/com/jfoenix/controls/JFXSpinner.java 新增 Spinner 控件定义与可样式化属性(radius/startingAngle)
HMCL/src/main/java/com/jfoenix/controls/JFXProgressBar.java 新增 ProgressBar 控件定义与 secondaryProgress 属性

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines 163 to 172
private void updateAnimation() {
ProgressIndicator control = getSkinnable();
final boolean isTreeVisible = control.isVisible() &&
control.getParent() != null &&
control.getScene() != null;
if (indeterminateTransition != null) {
pauseTimeline(!isTreeVisible);
} else if (isTreeVisible) {
createIndeterminateTimeline();
}
Copy link

Copilot AI Feb 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updateAnimation() ignores treeShowingExpression and only checks isVisible, getParent() != null, and getScene() != null. This means the indeterminate timeline can keep running when the window is hidden (or other ancestors make the node non-showing), even though you already compute TreeShowingProperty. Use treeShowingExpression.get() (or JFXNodeUtils.isTreeShowing(control)) as the gating condition for pausing/starting the timeline.

Copilot uses AI. Check for mistakes.
fillRect = new Rectangle();
fillRect.setFill(Color.TRANSPARENT);
text = new Text();
text.setStyle("-fx-font-size:null");
Copy link

Copilot AI Feb 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

text.setStyle("-fx-font-size:null") sets an invalid CSS value (null) and can cause JavaFX CSS warnings and/or prevent font sizing from working as expected. Prefer removing this line and letting CSS control the font via the .percentage style class, or explicitly setting the style to an empty string / null if the intent is to clear inline styles.

Suggested change
text.setStyle("-fx-font-size:null");

Copilot uses AI. Check for mistakes.
@Glavo Glavo requested a review from Copilot February 18, 2026 20:59
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 6 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +127 to +130
if (getSkinnable().isIndeterminate()) {
createIndeterminateTimeline();
if (JFXNodeUtils.isTreeShowing(getSkinnable())) {
indeterminateTransition.play();
Copy link

Copilot AI Feb 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The layoutChildren method calls createIndeterminateTimeline() on every layout pass when the progress bar is indeterminate (line 128). This could be inefficient as layoutChildren can be called frequently during resizing or other layout changes. The createIndeterminateTimeline() method does check if a timeline exists and clears it (lines 184-185), but this still involves stopping, clearing, and recreating the timeline repeatedly. Consider adding a guard to only recreate the timeline when the width actually changes, or caching the timeline and only recreating it when necessary.

Copilot uses AI. Check for mistakes.
final double progress = control.getProgress();
int intProgress = (int) Math.round(progress * 100.0);
Font font = text.getFont();
text.setFont(Font.font(font.getFamily(), radius / 1.7));
Copy link

Copilot AI Feb 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The magic number 1.7 is used to calculate the font size based on the radius (line 238: radius / 1.7). This ratio determines how the percentage text scales relative to the spinner size. Consider extracting this as a named constant (e.g., PERCENTAGE_TEXT_SCALE_FACTOR) to improve code readability and make it easier to adjust this ratio if needed in the future.

Suggested change
text.setFont(Font.font(font.getFamily(), radius / 1.7));
final double PERCENTAGE_TEXT_SCALE_FACTOR = 1.7;
text.setFont(Font.font(font.getFamily(), radius / PERCENTAGE_TEXT_SCALE_FACTOR));

Copilot uses AI. Check for mistakes.
indeterminateTransition.setCycleCount(Timeline.INDEFINITE);
}

private void clearAnimation() {
Copy link

Copilot AI Feb 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The clearAnimation() method calls indeterminateTransition.stop() without a null check. While the current code paths appear to check for null before calling this method (line 220 in dispose), this creates a fragile design. If clearAnimation() is ever called from a new code path without a null check, it will throw a NullPointerException. Consider adding a null check at the beginning of clearAnimation() to make it safer and more defensive.

Suggested change
private void clearAnimation() {
private void clearAnimation() {
if (indeterminateTransition == null) {
return;
}

Copilot uses AI. Check for mistakes.

this.treeShowingExpression = new TreeShowingProperty(bar);

bar.widthProperty().addListener(observable -> {
Copy link

Copilot AI Feb 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The width property listener (lines 55-58) is added directly without using registerChangeListener. This means it won't be automatically cleaned up when the skin is disposed. While JavaFX will eventually clean up these listeners when the control is garbage collected, this is inconsistent with the pattern used for other listeners in this class and could potentially cause memory leaks if the skin is replaced before the control is disposed. Consider using registerChangeListener for consistency and automatic cleanup, or manually remove this listener in the dispose() method.

Suggested change
bar.widthProperty().addListener(observable -> {
registerChangeListener(bar.widthProperty(), obs -> {

Copilot uses AI. Check for mistakes.

clip = new Region();
clip.setBackground(new Background(new BackgroundFill(Color.BLACK, CornerRadii.EMPTY, Insets.EMPTY)));
bar.backgroundProperty().addListener(observable -> JFXNodeUtils.updateBackground(bar.getBackground(), clip));
Copy link

Copilot AI Feb 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The bar.backgroundProperty() listener (line 90) is added directly without using registerChangeListener. This means it won't be automatically cleaned up when the skin is disposed. While this listener is on an internal component (bar) that gets disposed with the skin, using registerChangeListener would be more consistent with the pattern used elsewhere in this class and would ensure proper cleanup.

Suggested change
bar.backgroundProperty().addListener(observable -> JFXNodeUtils.updateBackground(bar.getBackground(), clip));
registerChangeListener(bar.backgroundProperty(), observable -> JFXNodeUtils.updateBackground(bar.getBackground(), clip));

Copilot uses AI. Check for mistakes.
}

private void initialize() {
setPrefWidth(200);
Copy link

Copilot AI Feb 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default preferred width for JFXProgressBar has been changed from 500 (previously set in CSS) to 200 (now set in code at line 55). This is a significant reduction that could affect existing layouts where the progress bar width was not explicitly set. Verify that this change is intentional and that all usages of JFXProgressBar in the codebase either explicitly set the width or can accommodate the new default width of 200.

Suggested change
setPrefWidth(200);
setPrefWidth(500);

Copilot uses AI. Check for mistakes.
@Glavo Glavo merged commit 010abf0 into HMCL-dev:main Feb 18, 2026
2 checks passed
@Glavo Glavo deleted the tree-visible branch February 18, 2026 21:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

Comments