-
Notifications
You must be signed in to change notification settings - Fork 753
Fix clipboard copy formatting based on selection dimensions in Stack Viewer #2332
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
Conversation
When copying a single cell value from Stack Viewer, the clipboard now contains just the value without markdown table delimiters. Multiple cell selections still use markdown table format with leading "| " and trailing " |" characters. This fixes the regression where single cell copy would wrap values in "|" characters unnecessarily. Co-authored-by: brianrob <[email protected]>
|
@copilot, there is some behavior that is not correct. This mostly works, but there are a few corner cases that don't work. One or two cells on top of one another, when copied and pasted, show up incorrectly. Example: The name column: module ntoskrnl <<ntoskrnl!?>> module code <<code!?>> Should be: Name Example: The First (timestamp) column: Sometimes shows up as "27.462 27.462". Sometimes as: 27.462 Should be: First |
Track total selected cells count to properly handle: - 1 cell: no markdown delimiters - 2 cells: no markdown delimiters (range handling) - 3+ cells: markdown table format with delimiters This fixes the corner case where selecting multiple cells from the same column (e.g., "module" and "ntoskrnl" vertically) would not properly format as a markdown table. Co-authored-by: brianrob <[email protected]>
Fixed in commit a07effc. The issue was that the original fix only checked columns per row (
This correctly handles vertically stacked selections like the Name column example you mentioned. |
|
@copilot, this still isn't working properly. How it should work: Single column of data:
Multiple columns of data:
|
Updated logic to track columns and rows separately: - Single column selections: include header, NO pipes - Multiple columns + single row: NO header, NO pipes - Multiple columns + multiple rows: include header, YES pipes This properly handles all clipboard scenarios as requested. Co-authored-by: brianrob <[email protected]>
Fixed in commit afbc9d4. Now tracking selection dimensions (columns × rows) separately: Single column:
Multiple columns:
The logic now uses |
brianrob
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot please address the comment.
| // Add markdown table formatting (| symbols) only when: | ||
| // - Multiple columns AND multiple rows selected | ||
| // Single column (any rows) or multiple columns in single row: no pipes | ||
| bool shouldAddPipes = (m_numSelectedColumns > 1 && m_numSelectedRows > 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do want to add pipe symbols for single rows when there are multiple columns selected. The one case where we don't want to add pipe symbols is when there is one row selected, two columns selected, and those cells are the last two columns (First and Last). We should detect this and adjust the value of shouldAddPipes.
Also, down below, we should update Grid.ClipboardCopyMode for all cases except the following:
- A single cell is selected.
- One row and two columns selected and the columns are the last two (First and Last).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in commit 079946b. Now:
- Pipes added for all multiple column selections (including single rows)
- Special case detected: 1 row + 2 columns = FirstColumn & LastColumn
- First/Last special case: no pipes, no headers
- All other multiple column selections: pipes and headers included
Updated clipboard logic: - Add pipes for any multiple column selection (not just multiple rows) - Detect First/Last column special case (1 row, 2 cols: First & Last) - Exclude headers and pipes for First/Last special case - Include headers for all multiple column selections except First/Last This addresses the requirement to add pipes for single rows with multiple columns, while handling the First/Last time range case. Co-authored-by: brianrob <[email protected]>
|
@copilot Everything works as I want it to with the exception of one case: When a single column with exactly two rows is selected, the contents are missing a newline in between the two values. For example, when selecting two cells in the Inc column, I get this: Inc However, if I select three, I get this: Inc Somewhere, a newline is missing between 139 and 52.0. |
Only set m_clipboardRangeStart/End for First/Last special case. This prevents the range morphing logic from triggering for normal 2-cell selections (like 2 cells in Inc column), which was removing the newline between values. The range morphing (combining on one line with space) should only apply to First/Last time range selections, not to any 2-cell selection. Co-authored-by: brianrob <[email protected]>
Fixed in commit 2bb092a. The issue was that the range morphing logic (which combines 2 values on one line with a space) was being triggered for any 2-cell selection, not just the First/Last special case. Now |
brianrob
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot please address this comment.
| if (i == e.ClipboardRowContent.Count - 1) | ||
| { | ||
| morphedContent = morphedContent + " |"; | ||
| // Add a leading | character to the first column to ensure GitHub renders the content as table |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove reference to GitHub - this is just a markdown table.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in commit 2b68cf2. Removed the GitHub reference and updated the comment to simply say "for markdown table format".
Updated comment to simply refer to "markdown table format" instead of "to ensure GitHub renders the content as table" since this is a general markdown table feature, not GitHub-specific. Co-authored-by: brianrob <[email protected]>
marklio
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, I'll have to lean on the testing here for correctness as this is a pretty convoluted data flow.
I'm almost wondering if what should happen is the cipboard contents should be entirely recalculated as text from the selected cells (or dual-registered as data/text so that pasting into excel would use the data and into something else would produce the markdown).
I'm also wondering if there is a package that does this in a way that would more readily convey intent and would possibly support a broader set of Markdown scenarios/richness.
If you wanted to merge as-is, I think that's likely totally fine, but wanted to provide this feedback.
| var dataGrid = sender as DataGrid; | ||
| if (dataGrid != null && dataGrid.SelectedCells.Count > 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SUGGESTION: if (sender is Datagrid dataGrid && dataGrid.SelectedCells.Count > 0)
While this is a personal preference, it does prevent duplicative checks and also scopes dataGrid to only scenarios in which it isn't null.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this, but didn't implement it for this particular one because we depend on the variable later in the method and I didn't want to duplicate the cast.
Fix clipboard copy formatting based on selection dimensions in Stack Viewer
Issue: When copying a single cell value (e.g., "Last" column with value "16,867.276"), the clipboard contains "| 16,867.276 |" instead of just "16,867.276". Additionally, the formatting behavior was inconsistent for different selection patterns.
Root cause: The code in
PerfDataGrid.xaml.cswas adding markdown table formatting (leading "| " and trailing " |") regardless of selection dimensions (columns × rows).Changes Made:
The fix implements proper clipboard formatting based on selection dimensions:
Single column of data:
Multiple columns of data:
Implementation:
m_numSelectedColumnsandm_numSelectedRowsto distinguish selection dimensionsm_isFirstLastSelection)Technical Details:
The fix tracks:
m_numSelectedCells) - for range handlingm_numSelectedColumns) - for format decisionsm_numSelectedRows) - for format decisionsm_isFirstLastSelection) - special case detection for time range selectionsHeader inclusion:
Pipe symbols (markdown table format):
Range morphing (combining on one line):
Security Summary:
Note: Build has pre-existing dependency issues with missing NuGet packages (unrelated to this change).
Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.