重构 JavaManagementPage 以使用 ListCell 展示元素#5223
Conversation
There was a problem hiding this comment.
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>withListPageBase<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")); | ||
| } | ||
|
|
There was a problem hiding this comment.
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.
| boolean isCurrentJava = JavaRuntime.CURRENT_JAVA != null | |
| && item.getBinary().equals(JavaRuntime.CURRENT_JAVA.getBinary()); | |
| removeButton.setDisable(isCurrentJava); |
| if (removeIcon != newRemoveIcon) { | ||
| removeIcon = newRemoveIcon; | ||
| removeIconPane.getChildren().setAll(removeIcon.createIcon(24)); | ||
| removeTooltip.setText(item.isManaged() ? i18n("java.uninstall") : i18n("java.disable")); |
There was a problem hiding this comment.
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.
| () -> { | ||
| String path = java.getBinary().toString(); | ||
| ConfigHolder.globalConfig().getUserJava().remove(path); | ||
| ConfigHolder.globalConfig().getDisabledJava().add(path); | ||
| try { | ||
| JavaManager.removeJava(java); | ||
| } catch (InterruptedException ignored) { | ||
| } | ||
| }, | ||
| null | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
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.
| () -> { | |
| 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) { | |
| } | |
| } |
No description provided.