[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.
| boolean selected = skinnable.selectedAccountProperty().get() == draggedAccount; | ||
| if (draggedAccount != null && sourceIndex != targetIndex) { | ||
| // Remove from old position | ||
| Accounts.skipSelectionCheckFlag = true; | ||
| Accounts.getAccounts().remove(sourceIndex); | ||
| // Insert at new position | ||
| int newIndex = targetIndex > sourceIndex ? targetIndex - 1 : targetIndex; | ||
| if (newIndex < 0) newIndex = 0; | ||
| if (newIndex > Accounts.getAccounts().size()) newIndex = Accounts.getAccounts().size(); | ||
| Accounts.getAccounts().add(newIndex, draggedAccount); | ||
| if (selected) skinnable.selectedAccountProperty().set(draggedAccount); | ||
| Accounts.skipSelectionCheckFlag = false; | ||
| success = true; | ||
| } |
There was a problem hiding this comment.
If an exception occurs during list modification, Accounts.skipSelectionCheckFlag will remain true permanently, which disables selection validation globally. Wrapping the list operations in a try-finally block ensures the flag is always reset. Additionally, if newIndex == sourceIndex, we can completely avoid removing and re-adding the item, preventing unnecessary list change events and potential UI flickers.
if (draggedAccount != null) {
int newIndex = targetIndex > sourceIndex ? targetIndex - 1 : targetIndex;
if (newIndex < 0) {
newIndex = 0;
}
int maxIndex = Accounts.getAccounts().size() - 1;
if (newIndex > maxIndex) {
newIndex = maxIndex;
}
if (sourceIndex != newIndex) {
boolean selected = skinnable.selectedAccountProperty().get() == draggedAccount;
Accounts.skipSelectionCheckFlag = true;
try {
Accounts.getAccounts().remove(sourceIndex);
Accounts.getAccounts().add(newIndex, draggedAccount);
if (selected) {
skinnable.selectedAccountProperty().set(draggedAccount);
}
} finally {
Accounts.skipSelectionCheckFlag = false;
}
success = true;
}
}| InvalidationListener listener = (observable) -> { | ||
| String text = searchingText.get().toLowerCase(Locale.ROOT); | ||
| if (StringUtils.isBlank(text)) { | ||
| displayedItems.setAll(items); | ||
| return; | ||
| } | ||
| displayedItems.setAll( | ||
| items.stream().filter(item -> { | ||
| Account account = item.getAccount(); | ||
| String type = ""; | ||
| if (account instanceof MicrosoftAccount) type = "microsoft"; | ||
| else if (account instanceof OfflineAccount) type = "offline"; | ||
| else if (account instanceof AuthlibInjectorAccount) type = ((AuthlibInjectorAccount) account).getServer().getUrl().toLowerCase(Locale.ROOT); | ||
| return account.getCharacter().toLowerCase(Locale.ROOT).contains(text) | ||
| || account.getUsername().toLowerCase(Locale.ROOT).contains(text) | ||
| || account.getUUID().toString().contains(text) | ||
| || type.contains(text); | ||
| }).toList() | ||
| ); | ||
| }; |
There was a problem hiding this comment.
If searchingText.get() is null, calling toLowerCase(Locale.ROOT) on it will throw a NullPointerException. It is safer to check for blank/null values first before performing string operations.
InvalidationListener listener = (observable) -> {
String query = searchingText.get();
if (StringUtils.isBlank(query)) {
displayedItems.setAll(items);
return;
}
String text = query.toLowerCase(Locale.ROOT);
displayedItems.setAll(
items.stream().filter(item -> {
Account account = item.getAccount();
String type = "";
if (account instanceof MicrosoftAccount) type = "microsoft";
else if (account instanceof OfflineAccount) type = "offline";
else if (account instanceof AuthlibInjectorAccount) type = ((AuthlibInjectorAccount) account).getServer().getUrl().toLowerCase(Locale.ROOT);
return account.getCharacter().toLowerCase(Locale.ROOT).contains(text)
|| account.getUsername().toLowerCase(Locale.ROOT).contains(text)
|| account.getUUID().toString().contains(text)
|| type.contains(text);
}).toList()
);
};| int index = Accounts.getAccounts().indexOf(account); | ||
| Accounts.getAccounts().removeAll(account); | ||
| account.setPortable(!account.isPortable()); | ||
| Accounts.getAccounts().add(index, account); |
There was a problem hiding this comment.
Using removeAll(account) is inefficient because it scans the entire list. Since we already have the index, we can use remove(index) which is faster. Additionally, if index is -1 (e.g., if the account was removed concurrently), calling add(-1, account) will throw an IndexOutOfBoundsException. We should guard against this by checking if index >= 0.
int index = Accounts.getAccounts().indexOf(account);
if (index >= 0) {
Accounts.getAccounts().remove(index);
account.setPortable(!account.isPortable());
Accounts.getAccounts().add(index, account);
}
Glavo
left a comment
There was a problem hiding this comment.
本 PR 可以把全局账户拖到便携账户中间,但实际这两类账户被存储在不同的文件里,这种次序不会被持久化,下次启动 HMCL 后次序会丢失。
Closes #5999 Resolves #5615 Resolves #3046