Skip to content

fix: select logs outside the screen was not deselected correctly#5955

Open
pynickle wants to merge 3 commits intoHMCL-dev:mainfrom
pynickle:fix/5884
Open

fix: select logs outside the screen was not deselected correctly#5955
pynickle wants to merge 3 commits intoHMCL-dev:mainfrom
pynickle:fix/5884

Conversation

@pynickle
Copy link
Copy Markdown
Contributor

Fix #5884

使用 JavaFX 原生的 SelectionModel 代替自建 selected

@Glavo
Copy link
Copy Markdown
Member

Glavo commented Apr 13, 2026

/gemini review

Copy link
Copy Markdown

@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 refactors the log selection mechanism by removing the manual selection state from the Log model and LogWindowSkin, utilizing the built-in JavaFX SelectionModel instead. Feedback highlights that this change might result in the loss of selection persistence during log filtering and suggests moving the selection mode configuration to the control's constructor. Furthermore, it is recommended to verify if the selection is empty before executing the copy logic to optimize performance and UI behavior.


private final String log;
private Log4jLevel level;
private boolean selected = false;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

Removing the selected field from the Log model means that selection state is no longer persistent when logs are filtered. In the previous implementation, selection was stored in the data model, allowing it to be preserved even if an item was temporarily hidden by a filter (e.g., toggling log levels). With this change, selection is tied strictly to the ListView's current items and will be lost whenever shakeLogs() (which calls setAll()) is triggered. If persistent selection across filtering is a requirement, consider maintaining the selected state in the Log class and synchronizing it with the SelectionModel.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

考虑过这一点 但是感觉这点行为差异并不是很要紧的事情)应该不会有人在多选之后 filter 还要求保留之前的多选吧


{
ListView<Log> listView = control.listView;
listView.getSelectionModel().setSelectionMode(SelectionMode.MULTIPLE);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

While setting the selection mode in the skin works, it is generally better practice to configure the control's properties (like the selection model's mode) within the control's constructor (LogWindowImpl) rather than in the skin. This ensures the control is correctly initialized regardless of the skin being used.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] 日志窗口日志行选中状态异常

2 participants