Skip to content

重构 JavaManagementPage 以使用 ListCell 展示元素#5223

Merged
Glavo merged 2 commits into
HMCL-dev:mainfrom
Glavo:JavaManagementPage
Jan 13, 2026
Merged

重构 JavaManagementPage 以使用 ListCell 展示元素#5223
Glavo merged 2 commits into
HMCL-dev:mainfrom
Glavo:JavaManagementPage

Conversation

@Glavo
Copy link
Copy Markdown
Member

@Glavo Glavo commented Jan 13, 2026

No description provided.

Copy link
Copy Markdown
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

This PR refactors the JavaManagementPage to use JavaFX's standard ListCell pattern instead of a custom Control-based approach. The refactoring simplifies the architecture by eliminating the JavaItem wrapper class and directly working with JavaRuntime objects.

Changes:

  • Replaced ListPageBase<JavaItem> with ListPageBase<JavaRuntime> to eliminate unnecessary wrapper class
  • Converted JavaItem (Control-based) and JavaRuntimeItemSkin (SkinBase-based) into a single JavaItemCell (ListCell-based) implementation
  • Moved toolbar initialization logic from the inner class to JavaPageSkin for better organization

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

removeIconPane.getChildren().setAll(removeIcon.createIcon(24));
removeTooltip.setText(item.isManaged() ? i18n("java.uninstall") : i18n("java.disable"));
}

Copy link

Copilot AI Jan 13, 2026

Choose a reason for hiding this comment

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

The remove button should be disabled when the JavaRuntime is the current Java runtime that HMCL is running on. The original code had this check:
if (JavaRuntime.CURRENT_JAVA != null && java.getBinary().equals(JavaRuntime.CURRENT_JAVA.getBinary())) removeButton.setDisable(true);

This logic has been removed in the refactoring, which means users could potentially attempt to uninstall the Java runtime that HMCL is currently using, which would lead to errors or undefined behavior.

Suggested change
boolean isCurrentJava = JavaRuntime.CURRENT_JAVA != null
&& item.getBinary().equals(JavaRuntime.CURRENT_JAVA.getBinary());
removeButton.setDisable(isCurrentJava);

Copilot uses AI. Check for mistakes.
Comment on lines +293 to +296
if (removeIcon != newRemoveIcon) {
removeIcon = newRemoveIcon;
removeIconPane.getChildren().setAll(removeIcon.createIcon(24));
removeTooltip.setText(item.isManaged() ? i18n("java.uninstall") : i18n("java.disable"));
Copy link

Copilot AI Jan 13, 2026

Choose a reason for hiding this comment

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

Icon creation occurs on every call to updateItem when the icon type changes. However, the icon pane is being updated by replacing all children with removeIconPane.getChildren().setAll(removeIcon.createIcon(24)). This could be optimized by caching the two possible icons (DELETE_FOREVER and DELETE) and reusing them rather than creating new icon nodes each time. Icon creation can be expensive, and updateItem is called frequently during list scrolling.

Copilot uses AI. Check for mistakes.
Comment on lines +331 to 343
() -> {
String path = java.getBinary().toString();
ConfigHolder.globalConfig().getUserJava().remove(path);
ConfigHolder.globalConfig().getDisabledJava().add(path);
try {
JavaManager.removeJava(java);
} catch (InterruptedException ignored) {
}
},
null
);
}
}
Copy link

Copilot AI Jan 13, 2026

Choose a reason for hiding this comment

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

The confirmation dialogs for managed and unmanaged Java should use consistent patterns. In the managed Java path, the confirmation callback directly executes Controllers.taskDialog(...). However, in the unmanaged Java path, the confirmation callback is wrapped in a lambda. For better consistency and maintainability, both paths should follow the same pattern.

Suggested change
() -> {
String path = java.getBinary().toString();
ConfigHolder.globalConfig().getUserJava().remove(path);
ConfigHolder.globalConfig().getDisabledJava().add(path);
try {
JavaManager.removeJava(java);
} catch (InterruptedException ignored) {
}
},
null
);
}
}
() -> disableJavaRuntime(java),
null
);
}
}
private void disableJavaRuntime(JavaRuntime java) {
String path = java.getBinary().toString();
ConfigHolder.globalConfig().getUserJava().remove(path);
ConfigHolder.globalConfig().getDisabledJava().add(path);
try {
JavaManager.removeJava(java);
} catch (InterruptedException ignored) {
}
}

Copilot uses AI. Check for mistakes.
@Glavo Glavo merged commit ccc64db into HMCL-dev:main Jan 13, 2026
2 checks passed
@Glavo Glavo deleted the JavaManagementPage branch January 13, 2026 13:35
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.

2 participants