Skip to content

LT-20857: Fixed a paste problem#900

Open
mark-sil wants to merge 3 commits into
mainfrom
LT-20857a
Open

LT-20857: Fixed a paste problem#900
mark-sil wants to merge 3 commits into
mainfrom
LT-20857a

Conversation

@mark-sil
Copy link
Copy Markdown
Contributor

@mark-sil mark-sil commented May 20, 2026

Fixed the paste problem when the selection is a single paragraph. In the code a paragraph is indicated by a ‘\n’.

Note: This fix is specific to a single paragraph selection. It does not add support for other complex selections.


This change is Reviewable

Fixed the paste problem when the selection is a single
paragraph. In the code a paragraph is indicated by a ‘\n’.

Note: This fix is specific to a single paragraph selection. It
does not add support for other complex selections.
@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 20, 2026

NUnit Tests

    1 files  ±0      1 suites  ±0   10m 57s ⏱️ -11s
4 205 tests ±0  4 134 ✅ ±0  71 💤 ±0  0 ❌ ±0 
4 214 runs  ±0  4 143 ✅ ±0  71 💤 ±0  0 ❌ ±0 

Results for commit 34b37e6. ± Comparison against base commit b0bf8a8.

♻️ This comment has been updated with latest results.

@johnml1135 johnml1135 requested review from Copilot and johnml1135 May 20, 2026 23:51
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Fixes paste behavior when the paste target is a single, fully-selected paragraph by avoiding paragraph-level replacement when the replacement text is also a single paragraph.

Changes:

  • Added helpers to detect whether the selection spans exactly one complete paragraph and whether the replacement text is exactly one paragraph.
  • Trimmed a trailing paragraph marker ('\n') from replacement text in the special-case scenario to force character replacement.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
Src/views/VwSelection.h Declares new helper methods for single-paragraph detection.
Src/views/VwSelection.cpp Implements helpers and applies newline-trimming logic in ReplaceWithTsString.

Comment thread Src/views/VwSelection.cpp Outdated
Comment thread Src/views/VwSelection.cpp
Comment on lines +5435 to +5444
if (!m_pvpboxEnd &&
targetMin == 0 &&
m_pvpbox->Source()->CStrings() == 1)
{
ITsString* firstStr = NULL;
m_pvpbox->Source()->StringAtIndex(0, &firstStr);
int firstStrLen = 0;
CheckHr(firstStr->get_Length(&firstStrLen));

if (targetLim == firstStrLen)
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

In the context where this is used, I don't think it is possible for CStrings() to be greater than 1. I was unable to test it greater than 1 so I didn't want to change logic that I couldn't test.

Comment thread Src/views/VwSelection.cpp Outdated
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
@github-actions

This comment has been minimized.

Copy link
Copy Markdown
Contributor

@jasonleenaylor jasonleenaylor left a comment

Choose a reason for hiding this comment

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

:lgtm:

@jasonleenaylor reviewed 2 files and all commit messages, and made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on johnml1135).

@johnml1135
Copy link
Copy Markdown
Contributor

Please look at branch LT-20857-johns-ideas for a potential small-fix direction for this PR.

The new commit is 2fe250f67f01a282871dbcd5dcac72b67b2e4772 (LT-20857: Trim terminal paste paragraph marker). It intentionally trims the solution back to the smallest behavior change I think is defensible for LT-20857:

  • Only trigger when the target selection is exactly one complete editable paragraph backed by a single source string.
  • Only trigger when the replacement text is exactly one paragraph with one terminal paragraph marker and no interior paragraph breaks.
  • Trim that terminal CRLF/CR/LF marker before the existing Views delete-and-insert pipeline runs.

Why this is needed: the current failure is the ordinary copy-whole-paragraph-over-whole-paragraph case. Keeping the terminal paragraph marker sends a simple replacement into the paragraph-replacement path, which is where the adjacent-paragraph corruption/failure risk shows up. Once the marker is removed, the edit remains a same-paragraph character replacement and continues through the existing Views edit, relayout, commit, and notification code instead of adding a new special replacement path.

This deliberately does not try to solve the broader paste problem. Multi-source rendered paragraphs, selections spanning backing properties, interlinear/table/object-run semantics, paragraph-style planning, selection-after-edit behavior, and paste-plan observability should stay in LT-22526.

Tests added in Src/views/Test/TestVwSelection.h cover CRLF, CR-only, LF-only, and backward whole-paragraph selections. They assert that the first paragraph is replaced, the terminal marker is not kept, the paragraph count remains stable, and the following paragraph remains unchanged.

Validation from this branch:

  • ./test.ps1 -SkipManaged -TestProject TestViews passes with 296-0-0.
  • CI: Full local check passed after the repository whitespace task finished its branch-file cleanup.
  • A top-level ./build.ps1 -SkipRestore -TailLines 10 still fails later in managed ManagedVwWindow with missing IVwWindow; native VwSelection.obj was refreshed before the TestViews run, so the native test result is against the updated paste code.

This response was created by AI at John's request.

@johnml1135
Copy link
Copy Markdown
Contributor

I am fine with the PR as it is, as it makes it better than it was, but to my understanding, the suggestions put in the PR by co-pilot extend it to work with CRLF while the current implementation only handles CR. It also adds a set of tests that could be helpful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants