Skip to content

优化工具栏“全选”功能 #5867

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
CiiLu wants to merge 1 commit into
base: main
Choose a base branch
from
+27 −5
Open

优化工具栏“全选”功能 #5867

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 11 additions & 2 deletions HMCL/src/main/java/org/jackhuang/hmcl/ui/versions/DataPackListPageSkin.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
import javafx.beans.property.BooleanProperty;
import javafx.beans.property.ObjectProperty;
import javafx.beans.property.SimpleBooleanProperty;
import javafx.collections.ListChangeListener;
import javafx.collections.transformation.FilteredList;
import javafx.geometry.Insets;
import javafx.geometry.Pos;
Expand Down Expand Up @@ -123,12 +124,20 @@ final class DataPackListPageSkin extends SkinBase<DataPackListPage> {
enableButton.disableProperty().bind(getSkinnable().readOnly);
disableButton.disableProperty().bind(getSkinnable().readOnly);

// reason for not using selectAll() is that selectAll() first clears all selected then selects all, causing the toolbar to flicker
var selectAllButton = createToolbarButton2(i18n("button.select_all"), SVG.SELECT_ALL, () -> listView.getSelectionModel().selectRange(0, listView.getItems().size()));

listView.getSelectionModel().getSelectedItems().addListener(
(ListChangeListener<Object>) change -> {
Copy link

Copilot AI Mar 29, 2026

Choose a reason for hiding this comment

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

(ListChangeListener<Object>) is an unchecked/overly-broad cast and loses type safety. Use the correct generic type for the selectedItems list (e.g., ListChangeListener) or a named listener variable to disambiguate the overload without casting to Object.

Suggested change
(ListChangeListener<Object>) change -> {
(ListChangeListener<DataPack>) change -> {

Copilot uses AI. Check for mistakes.
selectAllButton.setDisable(!listView.getItems().isEmpty()
&& listView.getSelectionModel().getSelectedItems().size() == listView.getItems().size());
}
);
Comment on lines +127 to +135
Copy link

Copilot AI Mar 29, 2026

Choose a reason for hiding this comment

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

The select-all button disable state is updated only when selectedItems changes and isn’t initialized. If listView.getItems() size changes (e.g., filtering/search updates) without a selection change, the button can remain disabled/enabled incorrectly. Consider binding disableProperty to a condition that observes both items size and selectedItems size, and ensure it’s evaluated once initially.

Copilot uses AI. Check for mistakes.
selectingToolbar.getChildren().addAll(
removeButton,
enableButton,
disableButton,
createToolbarButton2(i18n("button.select_all"), SVG.SELECT_ALL, () ->
listView.getSelectionModel().selectRange(0, listView.getItems().size())),//reason for not using selectAll() is that selectAll() first clears all selected then selects all, causing the toolbar to flicker
selectAllButton,
createToolbarButton2(i18n("button.cancel"), SVG.CANCEL, () ->
listView.getSelectionModel().clearSelection())
);
Expand Down
19 changes: 16 additions & 3 deletions HMCL/src/main/java/org/jackhuang/hmcl/ui/versions/ModListPageSkin.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,10 @@
import org.jackhuang.hmcl.ui.animation.ContainerAnimations;
import org.jackhuang.hmcl.ui.animation.TransitionPane;
import org.jackhuang.hmcl.ui.construct.*;
import org.jackhuang.hmcl.util.*;
import org.jackhuang.hmcl.util.FXThread;
import org.jackhuang.hmcl.util.Lazy;
import org.jackhuang.hmcl.util.Pair;
import org.jackhuang.hmcl.util.StringUtils;
import org.jackhuang.hmcl.util.i18n.I18n;
import org.jackhuang.hmcl.util.io.CompressingUtils;
import org.jackhuang.hmcl.util.io.FileUtils;
Expand Down Expand Up @@ -154,6 +157,17 @@ final class ModListPageSkin extends SkinBase<ModListPage> {
);

// Toolbar Selecting

// reason for not using selectAll() is that selectAll() first clears all selected then selects all, causing the toolbar to flicker
var selectAll = createToolbarButton2(i18n("button.select_all"), SVG.SELECT_ALL, () -> listView.getSelectionModel().selectRange(0, listView.getItems().size()));

listView.getSelectionModel().getSelectedItems().addListener(
(ListChangeListener<Object>) change -> {
selectAll.setDisable(!listView.getItems().isEmpty()
&& listView.getSelectionModel().getSelectedItems().size() == listView.getItems().size());
}
);
Comment on lines +164 to +169
Copy link

Copilot AI Mar 29, 2026

Choose a reason for hiding this comment

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

The select-all button disable state is only recomputed on selectedItems changes and isn’t initialized. If the item list size changes (e.g., refresh/search/filter adds/removes items) without a selection change, the button can stay disabled/enabled incorrectly. Prefer binding disableProperty to an expression that observes both items size and selectedItems size (and evaluate once initially) rather than a one-off listener.

Suggested change
listView.getSelectionModel().getSelectedItems().addListener(
(ListChangeListener<Object>) change -> {
selectAll.setDisable(!listView.getItems().isEmpty()
&& listView.getSelectionModel().getSelectedItems().size() == listView.getItems().size());
}
);
selectAll.disableProperty().bind(Bindings.createBooleanBinding(
() -> !listView.getItems().isEmpty()
&& listView.getSelectionModel().getSelectedItems().size() == listView.getItems().size(),
listView.getItems(),
listView.getSelectionModel().getSelectedItems()
));

Copilot uses AI. Check for mistakes.
Comment on lines +164 to +169
Copy link

Copilot AI Mar 29, 2026

Choose a reason for hiding this comment

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

(ListChangeListener<Object>) is an unchecked/overly-broad cast here. It would be better to disambiguate the overloaded addListener call using the correct generic type (e.g., ListChangeListener or a named listener variable) so warnings aren’t suppressed and the intent stays type-safe.

Suggested change
listView.getSelectionModel().getSelectedItems().addListener(
(ListChangeListener<Object>) change -> {
selectAll.setDisable(!listView.getItems().isEmpty()
&& listView.getSelectionModel().getSelectedItems().size() == listView.getItems().size());
}
);
ListChangeListener<ModInfoObject> selectionListener = change -> {
selectAll.setDisable(!listView.getItems().isEmpty()
&& listView.getSelectionModel().getSelectedItems().size() == listView.getItems().size());
};
listView.getSelectionModel().getSelectedItems().addListener(selectionListener);

Copilot uses AI. Check for mistakes.

toolbarSelecting.getChildren().setAll(
createToolbarButton2(i18n("button.remove"), SVG.DELETE_FOREVER, () -> {
Controllers.confirm(i18n("button.remove.confirm"), i18n("button.remove"), () -> {
Expand All @@ -171,8 +185,7 @@ final class ModListPageSkin extends SkinBase<ModListPage> {
.toList()
)
),
createToolbarButton2(i18n("button.select_all"), SVG.SELECT_ALL, () ->
listView.getSelectionModel().selectAll()),
selectAll,
createToolbarButton2(i18n("button.cancel"), SVG.CANCEL, () ->
listView.getSelectionModel().clearSelection())
);
Expand Down
Loading