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

feat: multiselect values #1736

Merged
merged 23 commits into from
Feb 6, 2024
Merged

Conversation

wusteven815
Copy link
Contributor

@wusteven815 wusteven815 commented Jan 19, 2024

  • Feature: Right-click filtering should filter column to all values in selection, not just the row clicked #1233
    • Moved cell filter actions to its own async function that gets a snapshot to supports multiple values
    • A maximum of 1000 (MAX_MULTISELECT_ROWS) rows will be snapshotted for unique values. The reason why its rows and not unique values is to prevent the case of a very large amount of rows with a very small amount of unique values.
    • Add tests for above by running E2E tests on all the filters
  • Changed right-click behaviour such that if right-clicked outside of the current selection, the new selection will be the row the cursor is on
    • Add tests for above, add function to right-click in Grid tests
  • Add assert to check if an array/map is non-empty, and if an number is not NaN
    • Add tests for above

Copy link

codecov bot commented Jan 19, 2024

Codecov Report

Attention: 154 lines in your changes are missing coverage. Please review.

Comparison is base (ac40c6f) 46.06% compared to head (a47b9ff) 45.99%.
Report is 3 commits behind head on main.

❗ Current head a47b9ff differs from pull request most recent head 15bca84. Consider uploading reports for the commit 15bca84 to get more accurate results

Files Patch % Lines
...d/src/mousehandlers/IrisGridContextMenuHandler.tsx 0.64% 154 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1736      +/-   ##
==========================================
- Coverage   46.06%   45.99%   -0.08%     
==========================================
  Files         626      626              
  Lines       37596    37716     +120     
  Branches     9460     9493      +33     
==========================================
+ Hits        17319    17346      +27     
- Misses      20222    20315      +93     
  Partials       55       55              
Flag Coverage Δ
unit 45.99% <11.49%> (-0.08%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@wusteven815 wusteven815 marked this pull request as ready for review January 19, 2024 22:21
@wusteven815 wusteven815 self-assigned this Jan 19, 2024
@dsmmcken dsmmcken self-requested a review January 22, 2024 15:59
@dsmmcken
Copy link
Contributor

I'll want to review the right click behaviour changes when available.

@wusteven815 wusteven815 marked this pull request as draft January 22, 2024 18:59
@wusteven815 wusteven815 marked this pull request as ready for review January 23, 2024 15:48
Copy link
Contributor

@dsmmcken dsmmcken left a comment

Choose a reason for hiding this comment

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

equals/not equals should be ORs and contains/starts with/ends with should be ANDs

  1. I know I said this in the ticket. I tried it and I was clearly wrong. It does not make sense for strings. It should really be just ORs for all string comparisons types.
  2. If you right click outside of the selection it should actually be changing the selection to the row you right-clicked on first.

Right-clicking on a row that isn’t selected should immediately change the selection to that row and present the menu options for that row
like this:
selection

I see what you did with the greater than/less than for numbers. I like that.

It's a little confusing at first if your set contains infinity, but I guess that's reasonable as it can't have greater than or less. Wonder if there's some sort of user feedback we should be giving though. Thoughts?

@wusteven815
Copy link
Contributor Author

It's a little confusing at first if your set contains infinity, but I guess that's reasonable as it can't have greater than or less. Wonder if there's some sort of user feedback we should be giving though. Thoughts?

How about if positive infinity is present, then > and >= are disabled, and the similarly for positive infinity?

@mofojed
Copy link
Member

mofojed commented Jan 25, 2024

It's a little confusing at first if your set contains infinity, but I guess that's reasonable as it can't have greater than or less. Wonder if there's some sort of user feedback we should be giving though. Thoughts?

How about if positive infinity is present, then > and >= are disabled, and the similarly for positive infinity?

infinity is similar to null, as the only options that make sense are is equal to and is not equal to. Doing <infinity is the same as saying !=infinity, and doing <=infinity doesn't make any sense. Converse is true for negative infinity. So if infinity and/or -infinity are present, only show the equals/not equals options.

@dsmmcken
Copy link
Contributor

So if infinity and/or -infinity are present, only show the equals/not equals options.

Yes, that is what is presently happening. However, I am saying It can be confusing. For example, say you select 500 rows. On screen you see 1, 2, 3, 4 etc. but somewhere in that selection set is -infinity. You don't know it's there you only know that for some reason you can only select equals/not equals and it's unclear as to why.

Maybe we should treat it like null and exclude it from the set if selection an option other than equals not equals?

@wusteven815
Copy link
Contributor Author

wusteven815 commented Jan 25, 2024

Since the <, <=, >, >= functions take min/max value of the snapshotted range, another possibility would be to disable their respective operations and add a message in the tooltip.

For example, if the selection was 1 and infinity:

  • >, >= would be disabled since the max is infinity and a tooltip like "Greater than infinity comparisons are redundant" would show
  • <, <= would still be enabled and would filter by < 1, <= 1

If there's a NaN, then filter can also be disabled with a "Selection includes NaN" tooltip.

@dsmmcken
Copy link
Contributor

Tooltips in a context menu seems like something one shouldn't do.

I am gonna recommend dropping it from the filter for cases other than equals/not equals, as that would probably behave inline with a users intent of selecting less than/greater than.

@@ -355,6 +355,7 @@ class Grid extends PureComponent<GridProps, GridState> {
this.handleMouseUp = this.handleMouseUp.bind(this);
this.handleResize = this.handleResize.bind(this);
this.handleWheel = this.handleWheel.bind(this);
this.getSelectedRanges = this.getSelectedRanges.bind(this);
Copy link
Member

Choose a reason for hiding this comment

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

Binding this here is not strictly necessary, as getSelectedRanges isn't passed in anywhere. Should just be able to call grid.getSelectedRanges directly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm getting TypeError: Cannot read properties of undefined (reading 'state') when I'm not binding it. I believe it's because the function is reading selectedRanges from this.state.

Comment on lines 248 to 250
grid.clearSelectedRanges();
grid.moveSelection(gridPoint.column, gridPoint.row);
grid.commitSelection();
Copy link
Member

Choose a reason for hiding this comment

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

I think you can just call moveCursorToPosition instead:

Suggested change
grid.clearSelectedRanges();
grid.moveSelection(gridPoint.column, gridPoint.row);
grid.commitSelection();
grid.moveCursorToPosition(gridPoint.column, gridPoint.row);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've replaced

grid.moveSelection(gridPoint.column, gridPoint.row);
grid.commitSelection();

with

grid.moveCursorToPosition(gridPoint.column, gridPoint.row);

It seems like clearSelectedRanges is needed, otherwise the the right click will just add to the selection.

packages/utils/src/TextUtils.ts Outdated Show resolved Hide resolved
packages/utils/src/TextUtils.ts Outdated Show resolved Hide resolved
mofojed
mofojed previously approved these changes Feb 5, 2024
Copy link
Member

@mofojed mofojed left a comment

Choose a reason for hiding this comment

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

Looks good. A couple suggestions for optimizations/refactors.

Copy link
Member

@mofojed mofojed left a comment

Choose a reason for hiding this comment

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

Excellent work!

@wusteven815 wusteven815 merged commit e6955c1 into deephaven:main Feb 6, 2024
6 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Feb 6, 2024
@wusteven815 wusteven815 deleted the feat-1233-multiselect branch February 9, 2024 15:30
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Right-click filtering should filter column to all values in selection, not just the row clicked
3 participants