[Enhancement] 账户列表优化#6093
Conversation
Co-authored-by: A01-opika <opika2022@outlook.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces search functionality and drag-and-drop reordering for the account list page. The review feedback highlights several critical improvements: wrapping the reordering list operations in a try-finally block to guarantee that the selection check flag is safely reset, preventing a potential NullPointerException by checking for null search queries before converting to lowercase, and avoiding an IndexOutOfBoundsException by validating the index when toggling an account's portable status.
Glavo
left a comment
There was a problem hiding this comment.
本 PR 可以把全局账户拖到便携账户中间,但实际这两类账户被存储在不同的文件里,这种次序不会被持久化,下次启动 HMCL 后次序会丢失。
Solved |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements account searching and drag-and-drop reordering in the account list page. The review feedback highlights several critical and high-severity issues, including a potential IndexOutOfBoundsException when loading accounts with out-of-bounds indices, loss of selection state when moving accounts, potential NullPointerExceptions during search filtering, list corruption if drag-and-drop is performed while searching, and potential layout calculation inaccuracies during drag-and-drop target indexing.
| return account.getCharacter().toLowerCase(Locale.ROOT).contains(text) | ||
| || account.getUsername().toLowerCase(Locale.ROOT).contains(text) | ||
| || account.getUUID().toString().contains(text) | ||
| || type.contains(text); |
There was a problem hiding this comment.
在搜索过滤时,account.getCharacter()、account.getUsername() 和 account.getUUID() 可能会返回 null,直接调用它们的方法会引发 NullPointerException。建议在调用方法前进行非空校验,以提高代码的防御性。
String character = account.getCharacter();
String username = account.getUsername();
java.util.UUID uuid = account.getUUID();
return (character != null && character.toLowerCase(Locale.ROOT).contains(text))
|| (username != null && username.toLowerCase(Locale.ROOT).contains(text))
|| (uuid != null && uuid.toString().contains(text))
|| type.contains(text);| list.setOnDragOver((event) -> { | ||
| if (event.getGestureSource() != list && event.getDragboard().hasString()) { | ||
| event.acceptTransferModes(TransferMode.MOVE); | ||
| } | ||
| event.consume(); |
There was a problem hiding this comment.
在搜索状态下,虽然拖拽的起点被禁用了,但 setOnDragOver 和 setOnDragDropped 仍然可能接收到拖拽事件。如果此时发生拖拽释放,计算出的目标索引会基于过滤后的列表,从而导致原始列表的顺序错乱。建议在 setOnDragOver 中增加 !skinnable.isSearching().get() 校验,完全禁用搜索时的拖拽接收。
| list.setOnDragOver((event) -> { | |
| if (event.getGestureSource() != list && event.getDragboard().hasString()) { | |
| event.acceptTransferModes(TransferMode.MOVE); | |
| } | |
| event.consume(); | |
| list.setOnDragOver((event) -> { | |
| if (!skinnable.isSearching().get() && event.getGestureSource() != list && event.getDragboard().hasString()) { | |
| event.acceptTransferModes(TransferMode.MOVE); | |
| } | |
| event.consume(); | |
| }); |
| for (int i = 0; i < Accounts.getAccounts().size(); i++) { | ||
| if (Accounts.getAccounts().get(i).getIdentifier().equals(accountId)) { | ||
| draggedAccount = Accounts.getAccounts().get(i); | ||
| sourceIndex = i; | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
在拖拽释放事件中,多次调用 Accounts.getAccounts() 会带来不必要的开销。此外,使用 accountId.equals(...) 相比 getIdentifier().equals(accountId) 更加安全,能有效避免潜在的 NullPointerException。建议将账户列表存入局部变量,并优化比对逻辑。
| for (int i = 0; i < Accounts.getAccounts().size(); i++) { | |
| if (Accounts.getAccounts().get(i).getIdentifier().equals(accountId)) { | |
| draggedAccount = Accounts.getAccounts().get(i); | |
| sourceIndex = i; | |
| break; | |
| } | |
| } | |
| javafx.collections.ObservableList<Account> allAccounts = Accounts.getAccounts(); | |
| for (int i = 0; i < allAccounts.size(); i++) { | |
| Account acc = allAccounts.get(i); | |
| if (acc != null && accountId.equals(acc.getIdentifier())) { | |
| draggedAccount = acc; | |
| sourceIndex = i; | |
| break; | |
| } | |
| } |
| private int getTargetIndex(VBox list, double y) { | ||
| int index = 0; | ||
| for (int i = 0; i < list.getChildren().size(); i++) { | ||
| javafx.scene.Node child = list.getChildren().get(i); | ||
| if (child.getLayoutY() + child.getBoundsInParent().getHeight() / 2 > y) { | ||
| return i; | ||
| } | ||
| index = i + 1; | ||
| } | ||
| return index; | ||
| } |
There was a problem hiding this comment.
在计算目标索引时,混合使用 child.getLayoutY() 和 child.getBoundsInParent().getHeight() 可能会在存在缩放或平移变换时导致计算不准确。建议统一使用 child.getBoundsInParent() 来获取子组件在父容器中的 Y 坐标和高度,以保证计算的准确性和一致性。
private int getTargetIndex(VBox list, double y) {
int index = 0;
for (int i = 0; i < list.getChildren().size(); i++) {
javafx.scene.Node child = list.getChildren().get(i);
javafx.geometry.Bounds bounds = child.getBoundsInParent();
if (bounds.getMinY() + bounds.getHeight() / 2 > y) {
return i;
}
index = i + 1;
}
return index;
}
Closes #5999 Resolves #5615 Resolves #3046