Skip to content

fix: "Delete Selected Rows" bug for tables with no key columns#1996

Merged
AkshatJawne merged 9 commits intodeephaven:mainfrom
AkshatJawne:1868-delete-selected-rows-fix
May 15, 2024
Merged

fix: "Delete Selected Rows" bug for tables with no key columns#1996
AkshatJawne merged 9 commits intodeephaven:mainfrom
AkshatJawne:1868-delete-selected-rows-fix

Conversation

@AkshatJawne
Copy link
Copy Markdown
Contributor

@AkshatJawne AkshatJawne commented May 8, 2024

Resolves #1868

Changes:

  • Added DeletableGridModel interface and corresponding isDeletableRange and isDeletableRanges methods as well as isDeletableGridModel and assertIsDeletableGridModel
  • Logic is shifted such that IrisGridProxyModel implements DeletableGridModel, and IrisGridTableModelTemplate implements the DeletableGridModel

@AkshatJawne AkshatJawne requested review from dsmmcken and mattrunyon May 8, 2024 20:40
@AkshatJawne AkshatJawne self-assigned this May 8, 2024
@AkshatJawne AkshatJawne changed the title feat: DRAFT: fix "Delete Selected Rows" bug for tables with no key columns fix "Delete Selected Rows" bug for tables with no key columns May 8, 2024
@AkshatJawne AkshatJawne changed the title fix "Delete Selected Rows" bug for tables with no key columns fix: "Delete Selected Rows" bug for tables with no key columns May 8, 2024
@AkshatJawne AkshatJawne marked this pull request as draft May 8, 2024 20:46
Comment thread packages/iris-grid/src/mousehandlers/IrisGridContextMenuHandler.tsx Outdated
@dsmmcken
Copy link
Copy Markdown
Contributor

dsmmcken commented May 9, 2024

Need to clarify whether the desired behaviour is for the the tab to be disabled as shown below, or should it not be present at all

If we know it can never be shown for a given table it should be hidden rather than disabled. Only disable it if there's a condition in which it could be come enabled.

@AkshatJawne
Copy link
Copy Markdown
Contributor Author

Need to clarify whether the desired behaviour is for the the tab to be disabled as shown below, or should it not be present at all

If we know it can never be shown for a given table it should be hidden rather than disabled. Only disable it if there's a condition in which it could be come enabled.

Will make this change right now

@AkshatJawne AkshatJawne requested review from mofojed May 9, 2024 15:44
@AkshatJawne
Copy link
Copy Markdown
Contributor Author

Will look into e2e tests failing

@AkshatJawne AkshatJawne marked this pull request as ready for review May 9, 2024 19:49
Comment thread packages/iris-grid/src/IrisGridProxyModel.ts Outdated
Comment thread packages/iris-grid/src/IrisGridProxyModel.ts Outdated
model.isEditable &&
selectedRanges.length > 0
selectedRanges.length > 0 &&
model.isDeletableRanges(selectedRanges)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

From Don's comment:

If we know it can never be shown for a given table it should be hidden rather than disabled. Only disable it if there's a condition in which it could be come enabled.

In this case you're just hiding it if the selected range isn't deletable, which if you have a totals row visible isn't quite correct - in that case, we should be showing the menu item, just disabled (since you can delete some rows, just not the totals rows). Other tables where you can't delete anything is where it shouldn't be shown at all.

So there's a few different types of tables we should be testing here (input_table docs here):

from deephaven import agg, empty_table, input_table

# Immutable table - should not show Delete Selected Rows, should not be editable
immutable_table = empty_table(10).update(["X = i", "Y=Math.sin(i)"])

# Append only table - should be able to add new rows, Delete Selected Rows should not be visible
append_only_table = input_table(init_table=source)

# Keyed row table - should be able to add new rows, Delete Selected Rows should be visible
keyed_table = input_table(init_table=source, key_cols=["X"])

Then, after making the keyed_table, you should be able to go to the table menu, hit Aggregate Columns, and add an aggregation (such as Sum). (BUG NOTE: Looks like you'll need to position it at the "Top" or you can't see the totals - should log that as a separate issue). Right clicking a keyed row should show "Delete Selected Rows" and enabled, but right-clicking the totals row should just show "Delete Selected Rows" and have it disabled.

I think to achieve all that we'll change up the design a little bit... it will be a bit more complicated, but it will be a cleaner design.

  • Add a DeletableGridModel.ts next to EditableGridModel.ts, and define an interface for a grid model that is deletable (e.g. an isDeletable: boolean; property, and isDeletableRange and isDeletableRanges methods). Also create the type predicates (isDeletableGridModel and assertIsDeletableGridModel. See isEditableGridModel and assertIsEditableGridModel in EditableGridModel for examples).
  • Have IrisGridProxyModel implement DeletableGridModel, and delegate to it's underlying model if the underlying model is DeletableGridModel (much like the EditableGridModel methods work in IrisGridProxyModel)
  • Have IrisGridTableModelTemplate implement the DeletableGridModel.
    • No need to change the other models, since they won't implement DeletableGridModel.
  • Check isDeleteable for adding the menu, then disable with !isDeletableRange

Let me know if you have questions. Making this a little more complicated, but now it will be more robust.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Also update your PR description after updating the content.

Comment thread packages/iris-grid/src/IrisGridModel.ts Outdated
Comment thread packages/code-studio/src/styleguide/MockIrisGridTreeModel.ts Outdated
Comment thread packages/grid/src/DeletableGridModel.ts
Comment thread packages/grid/src/DeletableGridModel.ts Outdated
Comment thread packages/grid/src/DeletableGridModel.ts Outdated
Comment thread packages/grid/src/DeletableGridModel.ts
Comment thread packages/iris-grid/src/EmptyIrisGridModel.ts Outdated
Comment thread packages/iris-grid/src/IrisGridTableModelTemplate.ts
@AkshatJawne AkshatJawne requested a review from mofojed May 13, 2024 18:15
Comment thread packages/iris-grid/src/IrisGrid.tsx Outdated
Comment thread packages/iris-grid/src/IrisGridTableModelTemplate.ts
@AkshatJawne AkshatJawne requested a review from mofojed May 13, 2024 20:01
Comment thread packages/iris-grid/src/IrisGridTableModelTemplate.ts Outdated
@AkshatJawne AkshatJawne requested a review from mofojed May 14, 2024 18:59
@AkshatJawne
Copy link
Copy Markdown
Contributor Author

Merging now!

@AkshatJawne AkshatJawne merged commit 37fe009 into deephaven:main May 15, 2024
@github-actions github-actions Bot locked and limited conversation to collaborators May 15, 2024
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.

Do not show "Delete Selected Rows" for input tables without columns

3 participants