Skip to content

[Enhancement] 账户列表优化#6093

Open
ToobLac wants to merge 7 commits into
HMCL-dev:mainfrom
ToobLac:account-order
Open

[Enhancement] 账户列表优化#6093
ToobLac wants to merge 7 commits into
HMCL-dev:mainfrom
ToobLac:account-order

Conversation

@ToobLac
Copy link
Copy Markdown
Contributor

@ToobLac ToobLac commented May 16, 2026

  • 搜索
  • 拖拽改变顺序(搜索时禁用)

Closes #5999 Resolves #5615 Resolves #3046

CalbootOnceMore and others added 2 commits May 1, 2026 21:57
@Glavo
Copy link
Copy Markdown
Member

Glavo commented May 29, 2026

/gemini review

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

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.

Comment thread HMCL/src/main/java/org/jackhuang/hmcl/ui/account/AccountListItemSkin.java Outdated
Copy link
Copy Markdown
Member

@Glavo Glavo left a comment

Choose a reason for hiding this comment

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

本 PR 可以把全局账户拖到便携账户中间,但实际这两类账户被存储在不同的文件里,这种次序不会被持久化,下次启动 HMCL 后次序会丢失。

@ToobLac
Copy link
Copy Markdown
Contributor Author

ToobLac commented May 31, 2026

本 PR 可以把全局账户拖到便携账户中间,但实际这两类账户被存储在不同的文件里,这种次序不会被持久化,下次启动 HMCL 后次序会丢失。

Solved

@Glavo
Copy link
Copy Markdown
Member

Glavo commented Jun 1, 2026

/gemini review

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

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.

Comment thread HMCL/src/main/java/org/jackhuang/hmcl/setting/Accounts.java Outdated
Comment on lines +120 to +123
return account.getCharacter().toLowerCase(Locale.ROOT).contains(text)
|| account.getUsername().toLowerCase(Locale.ROOT).contains(text)
|| account.getUUID().toString().contains(text)
|| type.contains(text);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

在搜索过滤时,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);

Comment on lines +279 to +283
list.setOnDragOver((event) -> {
if (event.getGestureSource() != list && event.getDragboard().hasString()) {
event.acceptTransferModes(TransferMode.MOVE);
}
event.consume();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

在搜索状态下,虽然拖拽的起点被禁用了,但 setOnDragOversetOnDragDropped 仍然可能接收到拖拽事件。如果此时发生拖拽释放,计算出的目标索引会基于过滤后的列表,从而导致原始列表的顺序错乱。建议在 setOnDragOver 中增加 !skinnable.isSearching().get() 校验,完全禁用搜索时的拖拽接收。

Suggested change
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();
});

Comment on lines +296 to +302
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;
}
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

在拖拽释放事件中,多次调用 Accounts.getAccounts() 会带来不必要的开销。此外,使用 accountId.equals(...) 相比 getIdentifier().equals(accountId) 更加安全,能有效避免潜在的 NullPointerException。建议将账户列表存入局部变量,并优化比对逻辑。

Suggested change
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;
}
}

Comment on lines +335 to 345
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;
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

在计算目标索引时,混合使用 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;
        }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

2 participants