重构 WorldListPage 以使用 ListCell 展示元素 #5224
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors WorldListPage to use JavaFX's standard ListCell pattern for displaying world items, eliminating the need for custom WorldListItem and WorldListItemSkin classes.
Changes:
- Replaced custom
WorldListItemControl class with direct use ofWorlddomain objects in the list - Migrated UI rendering logic from
WorldListItemSkininto a newWorldListCellinner class that properly implements theListCellpattern - Moved world operation methods (export, delete, copy, reveal, launch, etc.) from
WorldListItemtoWorldListPageas they now operate directly onWorldobjects
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| WorldListPage.java | Changed generic type from WorldListItem to World, added new WorldListCell inner class implementing ListCell<World>, moved world operation methods from WorldListItem to this class, and added createListCell() override in WorldListPageSkin |
| WorldListItem.java | Deleted - functionality moved to WorldListPage and WorldListCell |
| WorldListItemSkin.java | Deleted - UI rendering logic moved to WorldListCell.updateItem() method |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| public void delete(World world) { | ||
| WorldManageUIUtils.delete(world, () -> this.getItems().remove(world)); |
There was a problem hiding this comment.
The delete method removes the world from the displayed items list but does not update the worlds field. This creates an inconsistency: if the user toggles the "show all" filter or triggers a refresh, the deleted world may reappear briefly until the refresh completes. Consider also removing the world from the worlds list or calling refresh() after deletion instead of just removing from the items.
| WorldManageUIUtils.delete(world, () -> this.getItems().remove(world)); | |
| WorldManageUIUtils.delete(world, this::refresh); |
| Task.runAsync(() -> world.install(savesDir, name)) | ||
| .whenComplete(Schedulers.javafx(), () -> { | ||
| itemsProperty().add(new WorldListItem(this, new World(savesDir.resolve(name)), backupsDir, profile, id)); | ||
| itemsProperty().add(new World(savesDir.resolve(name))); |
There was a problem hiding this comment.
When a world is successfully installed, it's added directly to the items list but not to the worlds field. This creates inconsistency similar to the delete method: toggling filters or other operations that rely on the worlds field won't include the newly added world until a full refresh occurs. Consider calling refresh() after successful installation or updating both the worlds field and the items list.
| itemsProperty().add(new World(savesDir.resolve(name))); | |
| itemsProperty().add(new World(savesDir.resolve(name))); | |
| // Ensure backing 'worlds' list and filters are updated as well | |
| refresh(); |
| Task.supplyAsync(Schedulers.io(), () -> { | ||
| // Ensure the game version number is parsed | ||
| profile.getRepository().getGameVersion(id); |
There was a problem hiding this comment.
The game version is fetched but the result is discarded. The comment says "Ensure the game version number is parsed" but it's unclear why this is necessary. If the intent is to cache the game version for later use, consider storing the result. If the parsing has side effects, consider adding a comment explaining why this call is needed.
No description provided.