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

Fix copy behavior for sorted and/or filtered grid #18695

Merged
merged 7 commits into from
Feb 19, 2025
Merged

Fix copy behavior for sorted and/or filtered grid #18695

merged 7 commits into from
Feb 19, 2025

Conversation

cssuh
Copy link
Member

@cssuh cssuh commented Feb 15, 2025

Correctly copies the grid values for a grid that has been sorted and/or filtered.

Before, we were using a getRows call to STS which fetches the data based on the initial query. Once we sort or filter any data in the grid (which triggers isDataInMemory flag = true), we should use the dataProvider to tell us what data the user wants to copy, since it knows the context of the sort or filter states.

Fixes #18516
Fixes #18620

@cssuh cssuh changed the title Fix copy behavior for sorted grid Fix copy behavior for sorted and/or filtered grid Feb 15, 2025
Copy link

github-actions bot commented Feb 15, 2025

PR Changes

Category Main Branch PR Branch Difference
Code Coverage 50.40% 50.54% $${\color{lightgreen} .14\% }$$
VSIX Size 12969 KB 12143 KB $${\color{lightgreen} -826 KB \space (-6\%) }$$
Webview Bundle Size 3188 KB 3176 KB $${\color{lightgreen} -12 KB \space (0\%) }$$

@caohai
Copy link
Member

caohai commented Feb 19, 2025

Can you explain the following two things at a high level in the PR description?

  1. Root cause of the issues
  2. The fix you implemented in this PR

@cssuh cssuh requested a review from caohai February 19, 2025 19:33
@Benjin
Copy link
Contributor

Benjin commented Feb 19, 2025

Looks like this PR only addresses copying, but the same problem occurs for all copy/save targets. Maybe we need a more fundamental fix here.

Unfortunately, it seems that ADS has the same issue, where the "save" actions don't respect sorting and filtering, probably because they occur on the backend. Perhaps we need a UX solution in the meantime (e.g. "copy selection" where we can support that and "save all as .xyz" where we can't)

Comment on lines +690 to +691
let rowIdToSelectionMap = new Map<number, ISlickRange[]>();
let rowIdToRowMap = new Map<number, DbCellValue[]>();
Copy link
Contributor

Choose a reason for hiding this comment

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

A selection potentially spans multiple rows, right? So will this map have multiple rowId entries pointing to the same ISlickRange?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes that's correct

@cssuh cssuh requested a review from Benjin February 19, 2025 20:56
@caohai
Copy link
Member

caohai commented Feb 19, 2025

I gave the PR branch a quick test and can confirm copying is fixed with sorted/filtered grid.

One more question: I noticed the filter state is kept during context switch but the sorting state is not, is this intentional or do we want to fix it later?

Looks like this PR only addresses copying, but the same problem occurs for all copy/save targets. Maybe we need a more fundamental fix here.

Can confirm save is still broken, we might want to create a seperate issue for tracking.

@cssuh
Copy link
Member Author

cssuh commented Feb 19, 2025

I gave the PR branch a quick test and can confirm copying is fixed with sorted/filtered grid.

One more question: I noticed the filter state is kept during context switch but the sorting state is not, is this intentional or do we want to fix it later?

Looks like this PR only addresses copying, but the same problem occurs for all copy/save targets. Maybe we need a more fundamental fix here.

Can confirm save is still broken, we might want to create a seperate issue for tracking.

Yeah, we want to keep the sort state as well, I will add an issue to track this as well as an issue for the save targets

@cssuh cssuh merged commit b0dfdbd into main Feb 19, 2025
6 checks passed
@cssuh cssuh deleted the chsuh/copyFix branch February 19, 2025 23:46
cssuh added a commit that referenced this pull request Feb 19, 2025
* fix copy behavior for sorted grid

* cleanup

* refactor pr comments

* assign copy string

* refactor

* add comments

* remove console.log
cssuh added a commit that referenced this pull request Feb 20, 2025
* fix copy behavior for sorted grid

* cleanup

* refactor pr comments

* assign copy string

* refactor

* add comments

* remove console.log
kburtram pushed a commit that referenced this pull request Feb 20, 2025
* fix copy behavior for sorted grid

* cleanup

* refactor pr comments

* assign copy string

* refactor

* add comments

* remove console.log
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants