feat: multiselect values#1736
feat: multiselect values#1736wusteven815 merged 23 commits intodeephaven:mainfrom wusteven815:feat-1233-multiselect
Conversation
Codecov ReportAttention:
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
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
I'll want to review the right click behaviour changes when available. |
dsmmcken
left a comment
There was a problem hiding this comment.
equals/not equals should be ORs and contains/starts with/ends with should be ANDs
- 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.
- 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:
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?
How about if positive infinity is present, then > and >= are disabled, and the similarly for positive infinity? |
|
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? |
|
Since the For example, if the selection was 1 and infinity:
If there's a NaN, then filter can also be disabled with a "Selection includes NaN" tooltip. |
|
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. |
| this.handleMouseUp = this.handleMouseUp.bind(this); | ||
| this.handleResize = this.handleResize.bind(this); | ||
| this.handleWheel = this.handleWheel.bind(this); | ||
| this.getSelectedRanges = this.getSelectedRanges.bind(this); |
There was a problem hiding this comment.
Binding this here is not strictly necessary, as getSelectedRanges isn't passed in anywhere. Should just be able to call grid.getSelectedRanges directly.
There was a problem hiding this comment.
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.
| grid.clearSelectedRanges(); | ||
| grid.moveSelection(gridPoint.column, gridPoint.row); | ||
| grid.commitSelection(); |
There was a problem hiding this comment.
I think you can just call moveCursorToPosition instead:
| grid.clearSelectedRanges(); | |
| grid.moveSelection(gridPoint.column, gridPoint.row); | |
| grid.commitSelection(); | |
| grid.moveCursorToPosition(gridPoint.column, gridPoint.row); |
There was a problem hiding this comment.
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.
Co-authored-by: Mike Bender <mikebender@deephaven.io>
Co-authored-by: Mike Bender <mikebender@deephaven.io>
Co-authored-by: Mike Bender <mikebender@deephaven.io>
mofojed
left a comment
There was a problem hiding this comment.
Looks good. A couple suggestions for optimizations/refactors.

asyncfunction that gets a snapshot to supports multiple valuesMAX_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.