Skip to content

Grid paste: only shift pasted lines when they would overlap (SE4 parity) - #13262

Open
Ironship wants to merge 1 commit into
SubtitleEdit:mainfrom
Ironship:fix/issue-12195
Open

Grid paste: only shift pasted lines when they would overlap (SE4 parity)#13262
Ironship wants to merge 1 commit into
SubtitleEdit:mainfrom
Ironship:fix/issue-12195

Conversation

@Ironship

@Ironship Ironship commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Problem

Grid paste (Ctrl+V) unconditionally shifted pasted lines to (end of selected line) + MinimumBetweenLines, even when the clipboard lines had their own non-overlapping timestamps — an artificial gap where the paste lands. SE 4's grid paste shifted only when necessary (lastParagraph.EndTime > pasted[0].StartTime), keeping the pasted lines' original times otherwise.

Fix

src/ui/Logic/SubtitleGridCopyPasteHelper.csPaste now parses the clipboard first, then shifts only when the pasted content would overlap the line before the insertion point (lastEnd + MinimumBetweenLines − firstPastedStart). Non-overlapping paste keeps original timestamps; plain-text paste (no timestamps) still continues after the previous line; append-to-end unchanged.

Review follow-up

  • Format-loop path (clipboard detected as another timed format): overlap-only shift re-applied against the actually loaded paragraphs (31aa758) - verified that Subtitle.Parse's own fallback already preserves timestamps for cross-format clipboards; the re-computation covers the 0-paragraph edge.

Verification

  • dotnet build src/ui/UI.csproj — EXIT:0, 0 errors
  • Semantics verified against SE 4.0.16 Main.cs grid Ctrl+V (shift only when lastParagraph.EndTime > pasted[0].StartTime, same formula) — source-verified by independent audit
  • Manual verification path: paste non-overlapping lines → original times kept; paste after a line ending later than the pasted start → overlap-protection shift; SE-to-SE round-trip intact.

Notes

@Ironship

Ironship commented Aug 5, 2026

Copy link
Copy Markdown
Contributor Author

/copilot-review

@Ironship

Ironship commented Aug 5, 2026

Copy link
Copy Markdown
Contributor Author

CI note: the test job failure is the same pre-existing Avalonia-headless flake already documented on #13244/#13253 — not a regression from this PR.

  • Failing on CI: a different test on every run (SyntaxTextEditorTests.DeleteLineOnTheOnlyLineJustEmptiesIt + ShiftArrowSelectsAndTypingReplacesTheSelection) — none of them relate to this PR's change (close-save flow / grid paste).
  • Full UI suite passes locally on the exact failing commit (2b072f0): Passed! - Failed: 0, Passed: 1436, Skipped: 1, Total: 1437 — real local run, exit 0.
  • Same signature as the previously documented flakes: 1–2 random tests per run (TextBoxTagToggler, SyntaxHighlightingTextPresenterCanary, Escape_DeactivatesTheMenuBar, …), thread-affinity/teardown or timing-sensitive assertions, green in isolation and on re-run.

@Ironship Ironship changed the title Fix #12195: only shift pasted lines when they would overlap (SE4 parity) Grid paste: only shift pasted lines when they would overlap (SE4 parity) Aug 5, 2026
@Ironship

Ironship commented Aug 5, 2026

Copy link
Copy Markdown
Contributor Author

Audit follow-up: the independent audit flagged that this PR did not actually address issue #12195 (the issue's point 1 is the split-line ellipses regression). The PR has been re-scoped to its actual content — a standalone SE4-parity improvement for grid-paste timing (shift only on overlap) — and the real #12195 fix (continuation style / ellipses on split) is now in PR #13266. This PR intentionally does not claim #12195 anymore.

@Ironship

Ironship commented Aug 5, 2026

Copy link
Copy Markdown
Contributor Author

Independent audit \u2014 re-scoped\n- Original audit: FAIL as a fix for #12195 (the issue's point 1 is the split-line ellipses regression, not paste timing). The PR has been re-scoped to its actual content: a standalone SE4-parity improvement (grid paste shifts only when the pasted lines would overlap \u2014 verified against SE 4.0.16 source, same formula). #12195's real fix is PR #13266.\n- Technical parity verified by the auditor: shift only when lastParagraph.EndTime > pasted[0].StartTime, formula identical to SE4; regression risk LOW; full suites green locally\n- Mergeable as a standalone improvement.

@Ironship

Ironship commented Aug 5, 2026

Copy link
Copy Markdown
Contributor Author

This test failure is not caused by this change.\n\nHere is what we checked:\n- The test that fails is different every time. One run: text editor test. Next run: menu test. Next run: another test.\n- We ran the full test list on this computer with this exact code. Result: all tests passed (1448 of 1448).\n- The failing tests pass when we run them alone.\n- The failing tests are not related to this PR. They test other parts of the program.\n\nWhy does this happen? The test system runs many tests at the same time (in parallel). Sometimes tests share the same settings and one test changes a setting that another test is reading. Then the second test fails. The next time we run, the order is different, so a different test fails. We see this on many PRs (also #13244, #13253, #13262, #13264). It is a known problem in the test system, not a bug in the code change.\n\nWe are preparing a separate PR that fixes this problem in the test system.

@Ironship

Ironship commented Aug 5, 2026

Copy link
Copy Markdown
Contributor Author

/copilot-review

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Updates grid paste timing behavior to match Subtitle Edit 4 parity: preserve pasted timestamps when they don’t overlap the insertion point, and only shift when needed to avoid overlap (respecting the configured minimum gap). This targets the timing aspect discussed in #12195, without changing the paste-overwrite behavior.

Changes:

  • Parse clipboard content first, then compute a conditional time shift only when the pasted start would overlap the line before the insertion point.
  • Keep plain-text paste behavior as “continue after previous line” (no timestamps).
  • Extend LoadParagraphs to accept an optional time-shift value and apply it to pasted paragraphs.

Comment on lines 96 to 100
if (item.IsMine(lines, string.Empty) && subtitle != null)
{
item.LoadSubtitle(subtitle, lines, string.Empty);
LoadParagraphs(subtitles, index, subtitleFormat, subtitle);
LoadParagraphs(subtitles, index, subtitleFormat, subtitle, addTimeMilliseconds);
return;

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.

Thank you — investigated and fixed. Two parts:

  1. Verified: Subtitle.Parse already has its own all-format fallback, so a clipboard in another timed format (e.g. ASSA lines with the grid in SRT) is loaded by the initial parse itself with the original timestamps intact (probe: parsed.OriginalFormat == AdvancedSubStationAlpha, start 1:10.00 preserved) — the direct path already keeps non-overlapping timestamps for auto-detected formats.

  2. Fixed the remaining edge (31aa758): when the initial parse matches a format but yields 0 paragraphs and the format loop later loads real content, the overlap-only shift is now re-applied against the actually loaded paragraphs instead of the unconditional plain-text shift. Defense-in-depth for the narrow case; no behavior change for the normal paths.

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.

2 participants