Skip to content
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.

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

Non responsive UI related to big JTable vs selection #654

Closed
Chrriis opened this issue Mar 15, 2023 · 7 comments
Closed

Non responsive UI related to big JTable vs selection #654

Chrriis opened this issue Mar 15, 2023 · 7 comments
Milestone

Comments

@Chrriis
Copy link
Contributor

Chrriis commented Mar 15, 2023

We have a complex component that is a table with a tree structure and some frozen columns. We now need to display lots of data. With FlatLaf, the UI is unresponsive when there is an active selection. This does not happen with the system look and feel.

Try it with this test case, click to select a cell and use page up/down to navigate:
BigSelectableTableTest.zip

The reason is that in our complex component, there is cell remapping taking place between tree nodes and on screen rows. It is not a free operation. I found that this line triggers remapping for each row:

protected boolean isSelectionEditable( JTable table ) {

I guess one first optimization would be to not run the second part of the algorithm if the first part returned false and both row and column selections are turned on. Still, I wonder if this can be improved altogether... Imagine the above sample with millions of rows! 😉

Let me know what you think and do let me know if I can be of any help...
And keep up the good work! 😃

@Chrriis
Copy link
Contributor Author

Chrriis commented Mar 19, 2023

I took another look at the cell border logic, and I don't understand the intent...

Here is a modified test case with full row selection and 2 editable cells (this test case is NOT to show the original problem):
BigSelectableTablePartiallyEditableTest.zip

Here is what I get when I change selection. Orange point shows which cell has the focus, red outline in image 8 shows the 2 cells that are editable:
FlatLafTableFocusSelectionIssue

Questions:

  • Why is cell 1_1 not outlined in image 1 and cell 2_1 outlined in image 2, although they are both not editable?
  • Why is cell 4_1 outlined in image 7 but not in image 6?

More generally, what is the cell border class trying to achieve by checking the editable cells in selection?

DevCharly added a commit that referenced this issue Mar 27, 2023
- was black
- now derived from selection color (usually accent color)
  - darker in light themes
  - lighter in dark themes

(issue #654)
@DevCharly
Copy link
Collaborator

Thanks for reporting. That was not my best work 😞

More generally, what is the cell border class trying to achieve by checking the editable cells in selection?

The idea for this logic was to hide the focus indicator border (the black rectangle) when it is not needed,
and only show it when there are editable cells and the user needs to know which cell is focused to start editing.

This black rectangle looks simply ugly and in most cases, if table is not editable, it is useless IMHO.
On macOS there is also no focus indicator in tables/lists.

Why is cell 1_1 not outlined in image 1 ...

Because none of the selected cells in first row are editable.

... and cell 2_1 outlined in image 2, although they are both not editable?

Because cell 2_2 is selected and editable.
If the user wants start editing (using the keyboard), he needs to know which cell is focused.

Why is cell 4_1 outlined in image 7 but not in image 6?

Because in image 7, there is a selected editable cell in row 3.

Anyway, I agree that this is very confusing.

Made some changes in commit d27a246 to hopefully make it less confusing and to fix the performance issue:

  • Focus indicator is no longer shown if multiple rows are selected (unlikely that user wants to edit cell in this case).
  • Focus indicator is shown if single row is selected and at least one cell in that row is editable. If table has more than 50 columns, focus indicator is always show (to avoid performance issues)
  • Focus indicator is always shown for column-only selection (to avoid performance issues)

So editable checking is now limited to a single row and to maximum 50 cells.
This should avoid your performance issues.

@Chrriis
Copy link
Contributor Author

Chrriis commented Mar 27, 2023

Thanks, the performance issue is solved!

As for the logic of cell focus, I am still confused. In the test case BigSelectableTablePartiallyEditableTest, uncomment the line table.setCellSelectionEnabled(true);. The editable cells are (1;1) and (2;2), but the focus is only shown for the last column whatever is selected...

As a workaround, we activated defaults.put("Table.showCellFocusIndicator", Boolean.TRUE);

Perhaps the behavior of cell selection mode should be different from row or column selection mode. In cell selection mode, focus should be shown only when there is more that one cell that is selected (which internally means >= 2 rows, or >= 2 columns).

@DevCharly
Copy link
Collaborator

There is another reason that focus border is always shown in last column, which is of type boolean.
JTable.BooleanRenderer does not use Table.focusSelectedCellHighlightBorder (IMHO a bug in JDK):

https://github.com/openjdk/jdk/blob/a06f46196afd015db300ecf10bbb2a309b74e9d8/src/java.desktop/share/classes/javax/swing/JTable.java#L5482-L5486

JTable.BooleanRenderer uses following:

if( hasFocus )
    // use Table.focusCellHighlightBorder
else
    // use Table.cellNoFocusBorder

But DefaultTableCellRenderer uses:

if( hasFocus && isSelected )
    // use Table.focusSelectedCellHighlightBorder
else if( hasFocus )
    // use Table.focusCellHighlightBorder
else
    // use Table.cellNoFocusBorder

And the FlatLaf code to hide focus border is in Table.focusSelectedCellHighlightBorder...

Fixed this in commit 2e878b6

@DevCharly
Copy link
Collaborator

Perhaps the behavior of cell selection mode should be different from row or column selection mode. In cell selection mode, focus should be shown only when there is more that one cell that is selected (which internally means >= 2 rows, or >= 2 columns).

That's what the current implementation does (but only for rows).

In the meantime I think that it would be better to never show focus in cell selection mode.
If the user uses the arrow keys to navigate to an editable cell, then there is always only one cell selected (in cell selection mode).
If the user presses Shift+Arrow to select multiple cells, he probably does not want edit a cell. He maybe wants copy cell content or do another action.

In row selection mode it is different because always all cells of a row are selected. Here it makes sense to show focus if there is an editable cell in that row.

DevCharly added a commit that referenced this issue Mar 28, 2023
- never for cell selection mode
- for single selected column if contains editable cell
@DevCharly
Copy link
Collaborator

Have reworked the code again in commit d530624 (hopefully the last time).

The cell focus is now shown under following conditions:

  • always if enabled via UI property Table.showCellFocusIndicator=true
  • for row selection mode if exactly one row is selected and at least one cell in that row is editable
  • for column selection mode if exactly one column is selected and at least one cell in that column is editable
  • never for cell selection mode

To avoid possible performance issues, checking for editable cells is limited to 50. If there are more cells to check, the focus indicator is always shown.

@Chrriis
Copy link
Contributor Author

Chrriis commented Mar 28, 2023

This is perfect! Great job ! 😉

@Chrriis Chrriis closed this as completed Mar 28, 2023
@DevCharly DevCharly added this to the 3.1 milestone Apr 8, 2023
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

No branches or pull requests

2 participants