From f92643c18ba3a8509e899520cd047e470186cb23 Mon Sep 17 00:00:00 2001 From: John Lambert Date: Thu, 13 Aug 2026 15:52:05 -0400 Subject: [PATCH 1/5] perf: speed up bulk string replacement Compute each bulk preview value once. Replace all matches through one native ICU search session. Preserve rich text, Unicode collation, and legacy fallback behavior. Record measured gains and discarded experiments. --- ...ulk-replacement-distillation-2026-08-13.md | 111 ++++ Src/Common/Controls/XMLViews/BulkEditBar.cs | 104 ++-- Src/views/Test/TestVwPattern.h | 314 +++++++++++ Src/views/Views.idh | 15 +- Src/views/VwPattern.cpp | 288 ++++++++-- Src/views/VwPattern.h | 7 +- .../Performance/ReplaceWithMethodTests.cs | 489 +++++++++++++++++ .../Performance/VwPatternReplacementTests.cs | 490 ++++++++++++++++++ Src/xWorks/xWorksTests/BulkEditBarTests.cs | 137 +++++ 9 files changed, 1855 insertions(+), 100 deletions(-) create mode 100644 Docs/superpowers/plans/evidence/bulk-replacement-distillation-2026-08-13.md create mode 100644 Src/xWorks/xWorksTests/Avalonia/Performance/ReplaceWithMethodTests.cs create mode 100644 Src/xWorks/xWorksTests/Avalonia/Performance/VwPatternReplacementTests.cs diff --git a/Docs/superpowers/plans/evidence/bulk-replacement-distillation-2026-08-13.md b/Docs/superpowers/plans/evidence/bulk-replacement-distillation-2026-08-13.md new file mode 100644 index 0000000000..adf61765e6 --- /dev/null +++ b/Docs/superpowers/plans/evidence/bulk-replacement-distillation-2026-08-13.md @@ -0,0 +1,111 @@ +# Bulk replacement optimization evidence + +## Retained scope + +The focused implementation retains four changes: + +- Avoid normalization work when a replacement result is already NFD. +- Compute each preview value once and reuse it for the enabled result. +- Expose one-session rich-string replacement through the optional `IVwPattern2` + capability without changing the `IVwPattern` ABI or coclass default. +- Use the capability when available and preserve the repeated-`FindIn` fallback. + +The correctness corpus covers literal and regular-expression replacement, +captures, zero-width matches, whole-word and case/diacritic options, Unicode +normalization and scripts, ICU tailoring, rich-text properties, object +replacement characters, capability fallback, failures, and terminal state. + +## Single-pass preview measurement + +This is an incremental comparison of the single-pass preview change, not a +branch-point-to-final result. Both builds used Debug x64, 50,000 real entries, +100 percent matching CitationForm values with one match each, one warmup, and +five timed preview repetitions per process. Process order was B-C-B-C. + +- Baseline process 1: 73.973706, 87.429190, 85.850784, 89.671968, + 93.842072 microseconds/entry; median 87.429190. +- Candidate process 1: 53.317292, 49.020970, 49.454658, 47.650816, + 54.720194; median 49.454658. +- Baseline process 2: 68.099794, 68.588244, 70.913828, 70.782702, + 69.719412; median 69.719412. +- Candidate process 2: 48.884480, 49.327828, 58.699344, 78.705420, + 90.564270; median 58.699344. +- Pooled medians: baseline 72.443767, candidate 51.385975 + microseconds/entry; 29.068 percent reduction and 1.409796x throughput. + +The second candidate process was retried unchanged after a transient test +discovery failure produced no timing result. + +## ReplaceAllIn measurement + +This is an incremental comparison of `ReplaceAllIn`, not a cumulative branch +result. Both trees used `9307443e494d23066d9d40bc41c4d4b30944b972`. +The baseline changed only the managed capability assignment from the +`IVwPattern2` cast to `null`; native and search code was identical. Both used +`build.ps1 -BuildTests -Serial`, Debug x64, and the same temporary harness +(SHA-256 `52AB194E89486BFB94722A444B077B894B84128B46F8F0CF7EDD62DAE48D5481`). + +Each fresh process used 50,000 real entries. Matching values were +`old old old old-######` (22 UTF-16 units and four matches); unmatched values +were `keep keep keep-######` (21 units). Each fresh process had one warmup and +five timed apply repetitions. Reset, garbage collection, validation, and undo +were outside the stopwatch. There was no cache prepass. All comparisons +reported zero mismatches. + +### 25 percent matching + +- Baseline: 109.070736, 108.331108, 108.476428, 109.597136, 109.750080; + median 109.070736 microseconds/entry. +- Candidate: 80.400070, 86.023792, 81.024132, 83.734014, 100.501314; + median 83.734014. +- Incremental result: 23.23 percent reduction and 1.303x throughput. + +### 50 percent matching + +- Baseline process 1: 235.016240, 278.735516, 215.068208, 291.738206, + 245.199594; median 245.199594. +- Candidate process 1: 160.970294, 161.559868, 161.132232, 161.602208, + 178.361492; median 161.559868. +- Baseline process 2: 220.388066, 222.015458, 215.116946, 233.213528, + 239.261800; median 222.015458. +- Candidate process 2: 172.943354, 173.446744, 170.288498, 194.350902, + 182.141472; median 173.446744. +- Pooled medians: baseline 234.114884, candidate 171.615926; 26.69 percent + reduction and 1.364x throughput. The two paired reductions were 34.11 and + 21.88 percent. + +### 100 percent matching + +- Baseline process 1: 468.296628, 437.760106, 437.642286, 586.596700, + 518.897366; median 468.296628. +- Candidate process 1: 316.618992, 314.000950, 337.447102, 317.614622, + 315.852194; median 316.618992. +- Baseline process 2: 421.689880, 425.018894, 428.718602, 444.715500, + 429.318474; median 428.718602. +- Candidate process 2: 325.698984, 335.498302, 325.648910, 364.891896, + 362.333156; median 335.498302. +- Pooled medians: baseline 448.507615, candidate 326.058643; 27.30 percent + reduction and 1.376x throughput. The two paired reductions were 32.39 and + 21.75 percent. + +Debug-process variance was material, especially in the first baseline +process. The raw sample bands stayed separated, and each independent 50 and +100 percent pair exceeded the 15 percent acceptance gate. These measurements +cover the tested short CitationForm values only. + +## Rejected and deferred work + +- A printable-ASCII `memcmp` shortcut was reverted because ICU collation can + equate strings that differ in punctuation and other non-ordinal ways. +- A managed ordinal negative precheck was exactly reverted because it did not + produce sufficient benefit. +- A one-character collation-ignorable cache and early extension exit were + reverted. Its answer survived a locale change and produced an incorrect + match span; the single-character premise also lacked an ICU contract for + contextual collation. +- The virtual table and budgeted coordinator were test-owned prototypes with + no shipping activation path. They and their acceptance and attribution + harnesses were discarded. +- Homograph renumber batching was deferred after characterization only. It + needs separate cross-repository design and measurements for undo, redo, + notifications, cache membership, and group numbering. diff --git a/Src/Common/Controls/XMLViews/BulkEditBar.cs b/Src/Common/Controls/XMLViews/BulkEditBar.cs index 80affc51f0..356370c26f 100644 --- a/Src/Common/Controls/XMLViews/BulkEditBar.cs +++ b/Src/Common/Controls/XMLViews/BulkEditBar.cs @@ -4827,9 +4827,10 @@ public void FakeDoit(IEnumerable itemsToChange, int tagFakeFlid, int tagEna state.PercentDone = i * 100 / itemsToChange.Count(); state.Breath(); } - bool fEnable = OkToChange(hvo); + ITsString newValue; + bool fEnable = TryGetNewValue(hvo, out newValue); if (fEnable) - m_sda.SetString(hvo, tagFakeFlid, NewValue(hvo)); + m_sda.SetString(hvo, tagFakeFlid, newValue); m_sda.SetInt(hvo, tagEnable, (fEnable ? 1 : 0)); } } @@ -4861,10 +4862,9 @@ public void Doit(IEnumerable itemsToChange, ProgressState state) /// public virtual void Doit(int hvo) { - if (OkToChange(hvo)) - { - SetNewValue(hvo, NewValue(hvo)); - } + ITsString newValue; + if (TryGetNewValue(hvo, out newValue)) + SetNewValue(hvo, newValue); } /// @@ -4903,6 +4903,18 @@ protected virtual bool OkToChange(int hvo) return true; } + /// + /// Gets the new value when the item can change. + /// + protected virtual bool TryGetNewValue(int hvo, out ITsString newValue) + { + newValue = null; + if (!OkToChange(hvo)) + return false; + newValue = NewValue(hvo); + return newValue != null; + } + protected abstract ITsString NewValue(int hvo); #region IGetReplacedObjects Members @@ -5065,35 +5077,19 @@ protected override ITsString NewValue(int hvo) internal class ReplaceWithMethod : DoItMethod { - IVwPattern m_pattern; - ITsString m_replacement; - IVwTxtSrcInit m_textSourceInit; - IVwTextSource m_ts; + private readonly IVwPattern m_pattern; + private readonly IVwPattern2 m_bulkPattern; + private readonly IVwTxtSrcInit m_textSourceInit; + private readonly IVwTextSource m_ts; public ReplaceWithMethod(LcmCache cache, ISilDataAccessManaged sda, FieldReadWriter accessor, XmlNode spec, IVwPattern pattern, ITsString replacement) : base(cache, sda, accessor, spec) { m_pattern = pattern; - m_replacement = replacement; - m_pattern.ReplaceWith = m_replacement; + m_pattern.ReplaceWith = replacement; + m_bulkPattern = m_pattern as IVwPattern2; m_textSourceInit = VwStringTextSourceClass.Create(); - m_ts = m_textSourceInit as IVwTextSource; - } - - /// - /// We can do a replace if the pattern matches. - /// - /// - /// - protected override bool OkToChange(int hvo) - { - if (!base.OkToChange(hvo)) - return false; - ITsString tss = OldValue(hvo) ?? TsStringUtils.EmptyString(m_accessor.WritingSystem); - m_textSourceInit.SetString(tss); - int ichMin, ichLim; - m_pattern.FindIn(m_ts, 0, tss.Length, true, out ichMin, out ichLim, null); - return ichMin >= 0; + m_ts = (IVwTextSource)m_textSourceInit; } /// @@ -5104,6 +5100,13 @@ protected override bool OkToChange(int hvo) protected override ITsString NewValue(int hvo) { ITsString tss = OldValue(hvo) ?? TsStringUtils.EmptyString(m_accessor.WritingSystem); + ITsString bulkResult; + int matchCount; + if (TryReplaceAllIn(tss, out bulkResult, out matchCount)) + { + return matchCount == 0 ? null : NormalizeResult(bulkResult); + } + m_textSourceInit.SetString(tss); int ichStartSearch = 0; ITsStrBldr tsb = null; @@ -5124,7 +5127,7 @@ protected override ITsString NewValue(int hvo) for ( ; ichStartSearch <= cch; ) { int ichMin, ichLim; - m_pattern.FindIn(m_ts, ichStartSearch, cch, true, out ichMin, out ichLim, null); + FindIn(ichStartSearch, cch, out ichMin, out ichLim); if (ichMin < 0) break; if (ichLim == ichLimLastMatch) @@ -5142,23 +5145,40 @@ protected override ITsString NewValue(int hvo) } if (tsb == null) return null; - return tsb.GetString().get_NormalizedForm(FwNormalizationMode.knmNFD); + return NormalizeResult(tsb.GetString()); + } + + private static ITsString NormalizeResult(ITsString tssResult) + { + string text = tssResult.Text; + if (!string.IsNullOrEmpty(text) && + CustomIcu.GetIcuNormalizer(FwNormalizationMode.knmNFD).IsNormalized(text)) + { + return tssResult; + } + return tssResult.get_NormalizedForm(FwNormalizationMode.knmNFD); } /// - /// This is very like the base Doit, but we can save a duplicate pattern search - /// by calling the BASE version of OkToChange rather than our own version, which - /// tests for at least one match. We DO need to call the base version, e.g., so - /// we don't change wordforms which shouldn't change because they are in use. + /// Attempts bulk replacement when supported. /// - /// - public override void Doit(int hvo) + protected virtual bool TryReplaceAllIn(ITsString source, out ITsString result, + out int matchCount) { - if (!base.OkToChange(hvo)) - return; - ITsString tss = NewValue(hvo); - if (tss != null) - SetNewValue(hvo, tss); + if (m_bulkPattern == null) + { + result = null; + matchCount = 0; + return false; + } + + result = m_bulkPattern.ReplaceAllIn(source, 0, source.Length, out matchCount); + return true; + } + + protected virtual void FindIn(int ichStart, int ichEnd, out int ichMin, out int ichLim) + { + m_pattern.FindIn(m_ts, ichStart, ichEnd, true, out ichMin, out ichLim, null); } } /// diff --git a/Src/views/Test/TestVwPattern.h b/Src/views/Test/TestVwPattern.h index dac2acd22b..f19d3a442d 100644 --- a/Src/views/Test/TestVwPattern.h +++ b/Src/views/Test/TestVwPattern.h @@ -73,6 +73,88 @@ namespace TestViews } }; + class FailSecondLockTsString : public ITsString + { + long m_cref; + ITsStringPtr m_qtss; + int m_cLocks; + + public: + FailSecondLockTsString(ITsString * ptss) : m_cref(1), m_qtss(ptss), m_cLocks(0) + { + } + + STDMETHOD(QueryInterface)(REFIID riid, void ** ppv) + { + if (!ppv) + return E_POINTER; + *ppv = NULL; + if (riid != IID_IUnknown && riid != IID_ITsString) + return E_NOINTERFACE; + *ppv = static_cast(this); + AddRef(); + return S_OK; + } + STDMETHOD_(UCOMINT32, AddRef)() { return InterlockedIncrement(&m_cref); } + STDMETHOD_(UCOMINT32, Release)() + { + long cref = InterlockedDecrement(&m_cref); + if (!cref) + delete this; + return cref; + } + STDMETHOD(get_Text)(BSTR * pbstr) { return m_qtss->get_Text(pbstr); } + STDMETHOD(get_Length)(int * pcch) { return m_qtss->get_Length(pcch); } + STDMETHOD(get_RunCount)(int * pcrun) { return m_qtss->get_RunCount(pcrun); } + STDMETHOD(get_RunAt)(int ich, int * pirun) { return m_qtss->get_RunAt(ich, pirun); } + STDMETHOD(get_MinOfRun)(int irun, int * pichMin) + { return m_qtss->get_MinOfRun(irun, pichMin); } + STDMETHOD(get_LimOfRun)(int irun, int * pichLim) + { return m_qtss->get_LimOfRun(irun, pichLim); } + STDMETHOD(GetBoundsOfRun)(int irun, int * pichMin, int * pichLim) + { return m_qtss->GetBoundsOfRun(irun, pichMin, pichLim); } + STDMETHOD(FetchRunInfoAt)(int ich, TsRunInfo * ptri, ITsTextProps ** ppttp) + { return m_qtss->FetchRunInfoAt(ich, ptri, ppttp); } + STDMETHOD(FetchRunInfo)(int irun, TsRunInfo * ptri, ITsTextProps ** ppttp) + { return m_qtss->FetchRunInfo(irun, ptri, ppttp); } + STDMETHOD(get_RunText)(int irun, BSTR * pbstr) + { return m_qtss->get_RunText(irun, pbstr); } + STDMETHOD(GetChars)(int ichMin, int ichLim, BSTR * pbstr) + { return m_qtss->GetChars(ichMin, ichLim, pbstr); } + STDMETHOD(FetchChars)(int ichMin, int ichLim, OLECHAR * prgch) + { return m_qtss->FetchChars(ichMin, ichLim, prgch); } + STDMETHOD(LockText)(const OLECHAR ** pprgch, int * pcch) + { + if (++m_cLocks == 2) + return E_FAIL; + return m_qtss->LockText(pprgch, pcch); + } + STDMETHOD(UnlockText)(const OLECHAR * prgch) { return m_qtss->UnlockText(prgch); } + STDMETHOD(LockRun)(int irun, const OLECHAR ** pprgch, int * pcch) + { return m_qtss->LockRun(irun, pprgch, pcch); } + STDMETHOD(UnlockRun)(int irun, const OLECHAR * prgch) + { return m_qtss->UnlockRun(irun, prgch); } + STDMETHOD(get_PropertiesAt)(int ich, ITsTextProps ** ppttp) + { return m_qtss->get_PropertiesAt(ich, ppttp); } + STDMETHOD(get_Properties)(int irun, ITsTextProps ** ppttp) + { return m_qtss->get_Properties(irun, ppttp); } + STDMETHOD(GetBldr)(ITsStrBldr ** pptsb) { return m_qtss->GetBldr(pptsb); } + STDMETHOD(GetIncBldr)(ITsIncStrBldr ** pptisb) + { return m_qtss->GetIncBldr(pptisb); } + STDMETHOD(Equals)(ITsString * ptss, ComBool * pfEqual) + { return m_qtss->Equals(ptss, pfEqual); } + STDMETHOD(get_IsNormalizedForm)(FwNormalizationMode nm, ComBool * pfRet) + { return m_qtss->get_IsNormalizedForm(nm, pfRet); } + STDMETHOD(get_NormalizedForm)(FwNormalizationMode nm, ITsString ** pptssRet) + { return m_qtss->get_NormalizedForm(nm, pptssRet); } + STDMETHOD(NfdAndFixOffsets)(ITsString ** pptssRet, int ** prgpichOffsetsToFix, + int cichOffsetsToFix) + { + return m_qtss->NfdAndFixOffsets(pptssRet, prgpichOffsetsToFix, cichOffsetsToFix); + } + STDMETHOD(GetSubstring)(int ichMin, int ichLim, ITsString ** pptssRet) + { return m_qtss->GetSubstring(ichMin, ichLim, pptssRet); } + }; class TestVwPattern : public unitpp::suite { public: @@ -80,6 +162,7 @@ namespace TestViews ILgWritingSystemPtr m_qwsEng; ITsStrFactoryPtr m_qtsf; IVwPatternPtr m_qpat; + IVwPattern2Ptr m_qpat2; VwTxtSrcPtr m_qts; VwPropertyStorePtr m_qzvps; @@ -1622,6 +1705,235 @@ namespace TestViews unitpp::assert_eq("Length of 'in' (search 3)", ichMin + 2, ichLim); } + ITsStringPtr ReplaceWithRepeatedFindIn(ITsString * ptssSource, int ichStart, + int ichEnd, int * pcMatches) + { + IVwTxtSrcInitPtr qtsi; + qtsi.CreateInstance(CLSID_VwStringTextSource); + CheckHr(qtsi->SetString(ptssSource)); + IVwTextSourcePtr qts; + CheckHr(qtsi->QueryInterface(IID_IVwTextSource, (void **)&qts)); + ITsStrBldrPtr qtsb; + int ichStartSearch = ichStart; + int ichLimLastMatch = -1; + int cchDelta = 0; + *pcMatches = 0; + while (ichStartSearch <= ichEnd) + { + int ichMin; + int ichLim; + CheckHr(m_qpat->FindIn(qts, ichStartSearch, ichEnd, true, &ichMin, &ichLim, NULL)); + if (ichMin < 0) + break; + if (ichLim == ichLimLastMatch) + { + ichStartSearch = ichLim + 1; + continue; + } + ichLimLastMatch = ichLim; + ITsStringPtr qtssReplacement; + CheckHr(m_qpat->get_ReplacementText(&qtssReplacement)); + if (!qtsb) + CheckHr(ptssSource->GetBldr(&qtsb)); + CheckHr(qtsb->ReplaceTsString(ichMin + cchDelta, ichLim + cchDelta, + qtssReplacement)); + int cchReplacement; + CheckHr(qtssReplacement->get_Length(&cchReplacement)); + cchDelta += cchReplacement - (ichLim - ichMin); + (*pcMatches)++; + ichStartSearch = ichLim; + } + ITsStringPtr qtssResult; + if (qtsb) + CheckHr(qtsb->GetString(&qtssResult)); + else + qtssResult = ptssSource; + return qtssResult; + } + + void testReplaceAllInMatchesRepeatedFindIn() + { + ITsStringPtr qtssSource; + ITsStringPtr qtssPattern; + ITsStringPtr qtssReplacement; + CheckHr(m_qtsf->MakeStringRgch(L"and and", 7, g_wsEng, &qtssSource)); + CheckHr(m_qtsf->MakeStringRgch(L"and", 3, g_wsEng, &qtssPattern)); + CheckHr(m_qtsf->MakeStringRgch(L"or", 2, g_wsEng, &qtssReplacement)); + CheckHr(m_qpat->putref_Pattern(qtssPattern)); + CheckHr(m_qpat->putref_ReplaceWith(qtssReplacement)); + int cExpected; + ITsStringPtr qtssExpected = ReplaceWithRepeatedFindIn(qtssSource, 0, 7, + &cExpected); + IVwTxtSrcInitPtr qtsi; + qtsi.CreateInstance(CLSID_VwStringTextSource); + CheckHr(qtsi->SetString(qtssSource)); + IVwTextSourcePtr qts; + CheckHr(qtsi->QueryInterface(IID_IVwTextSource, (void **)&qts)); + int cMatches = -1; + ITsStringPtr qtssResult; + HRESULT hr = m_qpat2->ReplaceAllIn(qtssSource, 0, 7, &cMatches, &qtssResult); + unitpp::assert_eq("ReplaceAllIn succeeds", (int)S_OK, (int)hr); + CheckHr(hr); + unitpp::assert_eq("literal match count", 2, cMatches); + unitpp::assert_eq("literal differential match count", cExpected, cMatches); + ComBool fEqual; + CheckHr(qtssResult->Equals(qtssExpected, &fEqual)); + unitpp::assert_true("literal result equals repeated FindIn", fEqual); + SmartBstr sbstrResult; + CheckHr(qtssResult->get_Text(&sbstrResult)); + unitpp::assert_true("literal replacement text", sbstrResult == L"or or"); + + CheckHr(m_qtsf->MakeStringRgch(L"(o|e)(ld)", 10, g_wsEng, &qtssPattern)); + CheckHr(m_qtsf->MakeStringRgch(L"$2-$1", 5, g_wsEng, &qtssReplacement)); + CheckHr(m_qtsf->MakeStringRgch(L"old eld", 7, g_wsEng, &qtssSource)); + ITsStrBldrPtr qtsbSource; + CheckHr(qtssSource->GetBldr(&qtsbSource)); + StrUni stuSourceStyle(L"source-style"); + CheckHr(qtsbSource->SetStrPropValue(4, 7, ktptNamedStyle, stuSourceStyle.Bstr())); + CheckHr(qtsbSource->GetString(&qtssSource)); + CheckHr(m_qpat->putref_Pattern(qtssPattern)); + CheckHr(m_qpat->putref_ReplaceWith(qtssReplacement)); + CheckHr(m_qpat->put_UseRegularExpressions(true)); + qtssExpected = ReplaceWithRepeatedFindIn(qtssSource, 0, 7, &cExpected); + qtsi.CreateInstance(CLSID_VwStringTextSource); + CheckHr(qtsi->SetString(qtssSource)); + CheckHr(qtsi->QueryInterface(IID_IVwTextSource, (void **)&qts)); + cMatches = -1; + qtssResult.Clear(); + CheckHr(m_qpat2->ReplaceAllIn(qtssSource, 0, 7, &cMatches, &qtssResult)); + unitpp::assert_eq("regex capture match count", 2, cMatches); + unitpp::assert_eq("regex differential match count", cExpected, cMatches); + CheckHr(qtssResult->Equals(qtssExpected, &fEqual)); + unitpp::assert_true("regex rich result equals repeated FindIn", fEqual); + CheckHr(qtssResult->get_Text(&sbstrResult)); + unitpp::assert_true("regex captures are materialized per match", + sbstrResult == L"ld-o ld-e"); + + CheckHr(m_qtsf->MakeStringRgch(L"(?=.)", 5, g_wsEng, &qtssPattern)); + CheckHr(m_qtsf->MakeStringRgch(L"x", 1, g_wsEng, &qtssReplacement)); + CheckHr(m_qtsf->MakeStringRgch(L"\xD83D\xDE00" L"a", 3, g_wsEng, &qtssSource)); + CheckHr(m_qpat->putref_Pattern(qtssPattern)); + CheckHr(m_qpat->putref_ReplaceWith(qtssReplacement)); + qtssExpected = ReplaceWithRepeatedFindIn(qtssSource, 0, 3, &cExpected); + qtsi.CreateInstance(CLSID_VwStringTextSource); + CheckHr(qtsi->SetString(qtssSource)); + CheckHr(qtsi->QueryInterface(IID_IVwTextSource, (void **)&qts)); + cMatches = -1; + qtssResult.Clear(); + CheckHr(m_qpat2->ReplaceAllIn(qtssSource, 0, 3, &cMatches, &qtssResult)); + unitpp::assert_eq("UTF-16 zero-width match count", 3, cMatches); + unitpp::assert_eq("zero-width differential match count", cExpected, cMatches); + CheckHr(qtssResult->Equals(qtssExpected, &fEqual)); + unitpp::assert_true("zero-width result equals repeated FindIn", fEqual); + CheckHr(qtssResult->get_Text(&sbstrResult)); + unitpp::assert_true("zero-width matches preserve one-code-unit advancement", + sbstrResult == L"xxx"); + + CheckHr(m_qtsf->MakeStringRgch(L"not present", 11, g_wsEng, &qtssPattern)); + CheckHr(m_qpat->putref_Pattern(qtssPattern)); + CheckHr(m_qpat->put_UseRegularExpressions(false)); + qtssExpected = ReplaceWithRepeatedFindIn(qtssSource, 0, 3, &cExpected); + qtsi.CreateInstance(CLSID_VwStringTextSource); + CheckHr(qtsi->SetString(qtssSource)); + CheckHr(qtsi->QueryInterface(IID_IVwTextSource, (void **)&qts)); + cMatches = -1; + qtssResult.Clear(); + CheckHr(m_qpat2->ReplaceAllIn(qtssSource, 0, 3, &cMatches, &qtssResult)); + unitpp::assert_eq("no-match count", 0, cMatches); + unitpp::assert_eq("no-match differential count", cExpected, cMatches); + CheckHr(qtssResult->Equals(qtssExpected, &fEqual)); + unitpp::assert_true("no-match result equals repeated FindIn", fEqual); + CheckHr(qtssResult->Equals(qtssSource, &fEqual)); + unitpp::assert_true("no-match result preserves source content and properties", fEqual); + + cMatches = 99; + ITsString * ptssRawResult = reinterpret_cast(1); + hr = m_qpat2->ReplaceAllIn(NULL, 0, 3, &cMatches, &ptssRawResult); + unitpp::assert_eq("null source is rejected", (int)E_POINTER, (int)hr); + unitpp::assert_eq("count is cleared before null-source failure", 0, cMatches); + unitpp::assert_true("result is cleared before null-source failure", + ptssRawResult == NULL); + SetErrorInfo(0, NULL); + } + + void testReplaceAllInValidatesOutputsAndRanges() + { + ITsStringPtr qtssSource; + ITsStringPtr qtssPattern; + ITsStringPtr qtssReplacement; + CheckHr(m_qtsf->MakeStringRgch(L"and", 3, g_wsEng, &qtssSource)); + CheckHr(m_qtsf->MakeStringRgch(L"and", 3, g_wsEng, &qtssPattern)); + CheckHr(m_qtsf->MakeStringRgch(L"or", 2, g_wsEng, &qtssReplacement)); + CheckHr(m_qpat->putref_Pattern(qtssPattern)); + CheckHr(m_qpat->putref_ReplaceWith(qtssReplacement)); + ITsString * ptssRawResult = reinterpret_cast(1); + HRESULT hr = m_qpat2->ReplaceAllIn(qtssSource, 0, 3, NULL, &ptssRawResult); + unitpp::assert_eq("null match count is rejected", (int)E_POINTER, (int)hr); + unitpp::assert_true("result is untouched when first output is null", + ptssRawResult == reinterpret_cast(1)); + SetErrorInfo(0, NULL); + + int cMatches = 99; + hr = m_qpat2->ReplaceAllIn(qtssSource, 0, 3, &cMatches, NULL); + unitpp::assert_eq("null result is rejected", (int)E_POINTER, (int)hr); + unitpp::assert_eq("count is cleared before null-result failure", 0, cMatches); + SetErrorInfo(0, NULL); + + cMatches = 99; + ptssRawResult = reinterpret_cast(1); + hr = m_qpat2->ReplaceAllIn(qtssSource, -1, 3, &cMatches, &ptssRawResult); + unitpp::assert_eq("negative range is rejected", (int)E_INVALIDARG, (int)hr); + unitpp::assert_eq("count is cleared before negative-range failure", 0, + cMatches); + unitpp::assert_true("result is cleared before negative-range failure", + ptssRawResult == NULL); + SetErrorInfo(0, NULL); + + cMatches = 99; + ptssRawResult = reinterpret_cast(1); + hr = m_qpat2->ReplaceAllIn(qtssSource, 2, 1, &cMatches, &ptssRawResult); + unitpp::assert_eq("reversed range is rejected", (int)E_INVALIDARG, (int)hr); + unitpp::assert_eq("count is cleared before reversed-range failure", 0, + cMatches); + unitpp::assert_true("result is cleared before reversed-range failure", + ptssRawResult == NULL); + SetErrorInfo(0, NULL); + + cMatches = 99; + ptssRawResult = reinterpret_cast(1); + hr = m_qpat2->ReplaceAllIn(qtssSource, 0, 4, &cMatches, &ptssRawResult); + unitpp::assert_eq("range past source is rejected", (int)E_INVALIDARG, + (int)hr); + unitpp::assert_eq("count is cleared before past-source failure", 0, + cMatches); + unitpp::assert_true("result is cleared before past-source failure", + ptssRawResult == NULL); + SetErrorInfo(0, NULL); + } + + void testReplaceAllInClearsOutputsAfterReplacementFailure() + { + ITsStringPtr qtssSource; + ITsStringPtr qtssPattern; + ITsStringPtr qtssReplacement; + CheckHr(m_qtsf->MakeStringRgch(L"and and", 7, g_wsEng, &qtssSource)); + CheckHr(m_qtsf->MakeStringRgch(L"(and)", 5, g_wsEng, &qtssPattern)); + CheckHr(m_qtsf->MakeStringRgch(L"$1", 2, g_wsEng, &qtssReplacement)); + ITsStringPtr qtssFailingReplacement; + qtssFailingReplacement.Attach(NewObj FailSecondLockTsString(qtssReplacement)); + CheckHr(m_qpat->putref_Pattern(qtssPattern)); + CheckHr(m_qpat->putref_ReplaceWith(qtssFailingReplacement)); + CheckHr(m_qpat->put_UseRegularExpressions(true)); + + int cMatches = 99; + ITsString * ptssResult = reinterpret_cast(1); + HRESULT hr = m_qpat2->ReplaceAllIn(qtssSource, 0, 7, &cMatches, &ptssResult); + SetErrorInfo(0, NULL); + unitpp::assert_eq("replacement failure is returned", (int)E_FAIL, (int)hr); + unitpp::assert_eq("count remains zero after replacement failure", 0, cMatches); + unitpp::assert_true("result remains null after replacement failure", + ptssResult == NULL); + } virtual void Setup() { CreateTestWritingSystemFactory(); @@ -1631,6 +1943,7 @@ namespace TestViews // Use this rather than CreateInstance, because for the ORC test the pattern // and text source need to be in the same compilation unit. m_qpat.Attach(NewObj VwPattern()); + CheckHr(m_qpat->QueryInterface(IID_IVwPattern2, (void **)&m_qpat2)); m_qts.Attach(NewObj VwSimpleTxtSrc()); m_qzvps.Attach(NewObj VwPropertyStore()); m_qts->SetWritingSystemFactory(g_qwsf); @@ -1643,6 +1956,7 @@ namespace TestViews virtual void Teardown() { m_qts.Clear(); + m_qpat2.Clear(); m_qpat.Clear(); m_qwsEng.Clear(); m_qtsf.Clear(); diff --git a/Src/views/Views.idh b/Src/views/Views.idh index 867ae01776..d45d805e66 100644 --- a/Src/views/Views.idh +++ b/Src/views/Views.idh @@ -4252,14 +4252,27 @@ Last reviewed: [propget] HRESULT Group( [in] int iGroup, [out, retval] ITsString ** pptssGroup); - //:> We may want to add a method for returning the text of the paragraph, //:> to help display concordances. }; + // Provides synchronous bulk replacement for rich strings. + DeclareInterface(VwPattern2, VwPattern, FC825E0A-CEFB-461F-AFFB-E5E0E063CB74) + { + // Replaces non-overlapping forward matches in the UTF-16 range. + // No matches returns the source. + // Success leaves the pattern in a terminal no-match state. + HRESULT ReplaceAllIn( + [in] ITsString * ptss, + [in] int ichStart, + [in] int ichEnd, + [out] int * pcMatches, + [out, retval] ITsString ** pptssResult); + }; #ifndef NO_COCLASSES DeclareCoClass(VwPattern, 6C659C76-3991-48dd-93F7-DA65847D4863) { interface IVwPattern; + interface IVwPattern2; }; #endif // !NO_COCLASSES diff --git a/Src/views/VwPattern.cpp b/Src/views/VwPattern.cpp index f0a0958e23..5278c42976 100644 --- a/Src/views/VwPattern.cpp +++ b/Src/views/VwPattern.cpp @@ -25,6 +25,12 @@ DEFINE_THIS_FILE //:> VwPattern Methods //:>******************************************************************************************** +static int CheckedPatternPosition(__int64 value) +{ + if (value < 0 || value > INT_MAX) + ThrowHr(WarnHr(HRESULT_FROM_WIN32(ERROR_ARITHMETIC_OVERFLOW))); + return static_cast(value); +} /*---------------------------------------------------------------------------------------------- Constructor. @@ -38,7 +44,6 @@ VwPattern::VwPattern() m_fMatchDiacritics = true; } - /*---------------------------------------------------------------------------------------------- Destructor. ----------------------------------------------------------------------------------------------*/ @@ -62,9 +67,12 @@ STDMETHODIMP VwPattern::QueryInterface(REFIID riid, void **ppv) *ppv = static_cast(this); else if (riid == IID_IVwPattern) *ppv = static_cast(this); + else if (riid == IID_IVwPattern2) + *ppv = static_cast(this); else if (riid == IID_ISupportErrorInfo) { - *ppv = NewObj CSupportErrorInfo(this, IID_IVwPattern); + *ppv = NewObj CSupportErrorInfo2(static_cast(this), + IID_IVwPattern, IID_IVwPattern2); return S_OK; } else @@ -566,7 +574,6 @@ STDMETHODIMP VwPattern::get_Group(int iGroup, ITsString ** pptssGroup) END_COM_METHOD(g_fact, IID_IVwPattern); } - /*---------------------------------------------------------------------------------------------- Set whether to treat character sequences that are equivalent as defined by Unicode compatibility decompositions as being identical. @@ -915,7 +922,10 @@ class FindInAlgorithmBase int m_ichMinSearch; // Range of text to search (from smallest to largest index) int m_ichLimSearch; OLECHAR * m_pchBuf; // Text contents of m_pts; + Vector m_vchBuf; UErrorCode m_error; + bool m_fSessionInitialized; + bool m_fPropertiesOnly; FindInAlgorithmBase(IVwTextSource * pts, int ichStartLog, int ichEndLog, ComBool fForward, IVwSearchKiller * pxserkl, VwPattern * pat) @@ -929,6 +939,8 @@ class FindInAlgorithmBase m_ichLimFoundSearch = -1; m_pat = pat; m_error = U_ZERO_ERROR; + m_fSessionInitialized = false; + m_fPropertiesOnly = false; } virtual ~FindInAlgorithmBase() @@ -1108,9 +1120,9 @@ class FindInAlgorithmBase virtual void InitSearcher() = 0; // Run the main search loop over a single text source. virtual bool Search() = 0; + virtual bool SearchNext() = 0; - // Run the main body of the algorithm. Return true if a match is made successfully. - bool Run() + bool Prepare() { // Get the characters (paragraph) we have to search. // We are getting the whole paragraph to give ICU enough context for whole word matching. @@ -1131,16 +1143,64 @@ class FindInAlgorithmBase m_pat->Compile(); if ((!m_pat->m_fUseRegularExpressions) && m_pat->m_stuCompiled.Length() == 0) + { + m_fPropertiesOnly = true; + return true; + } + return true; + } + + bool InitializeSession() + { + if (!Prepare()) + return false; + if (m_fPropertiesOnly) + { + m_fSessionInitialized = true; + return true; + } + + m_vchBuf.Resize(CheckedPatternPosition(static_cast<__int64>(m_cchSrcSearch) + 1)); + InitializeSearcher(m_vchBuf.Begin()); + m_fSessionInitialized = true; + return true; + } + + bool Run() + { + if (!Prepare()) + return false; + if (m_fPropertiesOnly) return SearchForProperties(); - m_pchBuf = (OLECHAR *)(_alloca((m_cchSrcSearch + 1) * isizeof(OLECHAR))); - CheckHr(m_pts->FetchSearch(0, m_cchSrcSearch, m_pchBuf)); - * (m_pchBuf + m_cchSrcSearch) = 0; // null termination required. - m_ichLimSearch = std::min(m_cchSrcSearch, m_ichLimSearch); // Because of disregarded ORCs, we might get fewer charcaters in the buffer than we asked for. + OLECHAR * pchBuf = + (OLECHAR *)(_alloca((m_cchSrcSearch + 1) * isizeof(OLECHAR))); + InitializeSearcher(pchBuf); + return Search(); + } + + void InitializeSearcher(OLECHAR * pchBuf) + { + m_pchBuf = pchBuf; + CheckHr(m_pts->FetchSearch(0, m_cchSrcSearch, m_pchBuf)); + m_pchBuf[m_cchSrcSearch] = 0; + m_ichLimSearch = std::min(m_cchSrcSearch, m_ichLimSearch); InitSearcher(); + } - return Search(); + bool NextAcceptedMatch(int ichStartLog) + { + Assert(m_fSessionInitialized); + Assert(m_fForward); + CheckHr(m_pts->LogToSearch(ichStartLog, &m_ichMinSearch)); + if (m_ichMinSearch > m_ichLimSearch) + return Fail(); + m_ichMinFoundSearch = -1; + m_ichLimFoundSearch = -1; + if (m_fPropertiesOnly) + return SearchForProperties(); + return SearchNext(); } // Checks to see if the next char in the searched string is a Diacritic. If it is a diacritic @@ -1154,13 +1214,13 @@ class FindInAlgorithmBase return true; OLECHAR rgchw[2]; - CheckHr(m_pts->FetchSearch(m_ichLimFoundSearch, m_ichLimFoundSearch + 1, &rgchw[0])); + rgchw[0] = m_pchBuf[m_ichLimFoundSearch]; uint ch32; // if chw is the first char of a surrogate pair, and , fetch the next char as well and translate the pair into a UChar32 // otherwise, copy chw to a UChar32. if (U_IS_LEAD(rgchw[0]) && m_ichLimFoundSearch + 1 < m_ichLimSearch) { - CheckHr(m_pts->FetchSearch(m_ichLimFoundSearch + 1, m_ichLimFoundSearch + 2, &rgchw[1])); + rgchw[1] = m_pchBuf[m_ichLimFoundSearch + 1]; Assert(U_IS_TRAIL(rgchw[1])); bool fSurrogateOk = FromSurrogate(rgchw[0], rgchw[1], &ch32); Assert(fSurrogateOk); @@ -1305,6 +1365,7 @@ class FindInAlgorithm : public FindInAlgorithmBase { StringSearch * m_piter; BreakIterator * m_pbi; + bool m_fSearchStarted; public: FindInAlgorithm(IVwTextSource * pts, int ichStartLog, int ichEndLog, ComBool fForward, IVwSearchKiller * pxserkl, VwPattern * pat) @@ -1312,6 +1373,7 @@ class FindInAlgorithm : public FindInAlgorithmBase { m_piter = NULL; m_pbi = NULL; + m_fSearchStarted = false; } virtual ~FindInAlgorithm() @@ -1383,29 +1445,7 @@ class FindInAlgorithm : public FindInAlgorithmBase bool Search() { if (m_fForward) - { - for (m_ichMinFoundSearch = m_piter->first(m_error); - ; // termination checks are inside the loop body - m_ichMinFoundSearch = m_piter->next(m_error) ) - { - CheckError(); // see if first() or next() call failed. - if (m_ichMinFoundSearch == USEARCH_DONE) - return Fail(); - if (m_ichMinFoundSearch < m_ichMinSearch) - continue; - if (m_ichMinFoundSearch >= m_ichLimSearch) - continue; - m_ichLimFoundSearch = m_ichMinFoundSearch + m_piter->getMatchedLength(); - AdjustSearchLimitForDiacritics(); - if (m_ichLimFoundSearch > m_ichLimSearch) - return Fail(); // The first match extends past the end of our range. - if (m_pat->m_fMatchDiacritics && !CheckMatchDiacritic()) - continue; - // We have a candidate match. See if it satisfies ws, style, tag requirements if any - if (CheckAndExtendCandidate()) - return true; - } - } + return SearchNext(); else { for (m_ichMinFoundSearch = m_piter->last(m_error); @@ -1434,6 +1474,33 @@ class FindInAlgorithm : public FindInAlgorithmBase return false; // arbitrary (should never get here). } + bool SearchNext() + { + Assert(m_fForward); + for (m_ichMinFoundSearch = m_fSearchStarted ? m_piter->next(m_error) : + m_piter->first(m_error); + ; // termination checks are inside the loop body + m_ichMinFoundSearch = m_piter->next(m_error)) + { + m_fSearchStarted = true; + CheckError(); // see if first() or next() call failed. + if (m_ichMinFoundSearch == USEARCH_DONE) + return Fail(); + if (m_ichMinFoundSearch < m_ichMinSearch) + continue; + if (m_ichMinFoundSearch >= m_ichLimSearch) + continue; + m_ichLimFoundSearch = m_ichMinFoundSearch + m_piter->getMatchedLength(); + AdjustSearchLimitForDiacritics(); + if (m_ichLimFoundSearch > m_ichLimSearch) + return Fail(); // The first match extends past the end of our range. + if (m_pat->m_fMatchDiacritics && !CheckMatchDiacritic()) + continue; + if (CheckAndExtendCandidate()) + return true; + } + } + /*------------------------------------------------------------------------------------------ As of ICU 4.0 (or at least after ICU 3.6), the string search matches diacritics past the limit even when told not to match diacritics. @@ -1505,27 +1572,7 @@ class RegExFindInAlgorithm : public FindInAlgorithmBase { bool fMatch = true; // did we get a match this iteration? if (m_fForward) - { - for (fMatch = m_pmatcher->find(m_ichMinSearch, m_error); ; fMatch = m_pmatcher->find() ) - { - if (!fMatch) - return Fail(); // no more matches. - m_ichMinFoundSearch = m_pmatcher->start(m_error); - m_ichLimFoundSearch = m_pmatcher->end(m_error); - CheckError(); // see if find() or start() or end() call failed. - if (m_ichLimFoundSearch > m_ichLimSearch) - return Fail(); // The current match extends past the end of our range. - // Doesn't seem to be any reasonable way to do this - //if (m_pat->m_fMatchDiacritics) - // if(!CheckMatchDiacritic()) - // return Fail(); - // We have a candidate match. See if it satisfies ws, style, tag requirements if any - // I don't think we can usefully do this either. - //if (CheckAndExtendCandidate()) - // return true; - return true; // got a useful match. - } - } + return SearchNext(); else { // Simulate searching backwards by searching forwards and keeping the last match in range. @@ -1562,6 +1609,19 @@ class RegExFindInAlgorithm : public FindInAlgorithmBase } return false; // arbitrary (should never get here). } + + bool SearchNext() + { + Assert(m_fForward); + if (!m_pmatcher->find(m_ichMinSearch, m_error)) + return Fail(); + m_ichMinFoundSearch = m_pmatcher->start(m_error); + m_ichLimFoundSearch = m_pmatcher->end(m_error); + CheckError(); + if (m_ichLimFoundSearch > m_ichLimSearch) + return Fail(); + return true; + } }; /*---------------------------------------------------------------------------------------------- @@ -1646,6 +1706,122 @@ STDMETHODIMP VwPattern::FindIn(IVwTextSource * pts, int ichStartLog, int ichEndL END_COM_METHOD(g_fact, IID_IVwPattern); } +void VwPattern::ReplaceAllWithAlgorithm(FindInAlgorithmBase * pfia, IVwTextSource * pts, + ITsString * ptssSource, int ichStart, int ichEnd, int * pcMatches, + ITsString ** pptssResult) +{ + struct Replacement + { + int ichMin; + int ichLim; + ITsStringPtr qtss; + }; + Vector vrepl; + ITsStringPtr qtssSource = ptssSource; + ITsStrBldrPtr qtsb; + int ichStartSearch = ichStart; + int ichLimLastMatch = -1; + int cMatches = 0; + + if (pfia->InitializeSession()) + { + while (ichStartSearch <= ichEnd && pfia->NextAcceptedMatch(ichStartSearch)) + { + int ichMin; + int ichLim; + CheckHr(pts->SearchToLog(pfia->m_ichMinFoundSearch, false, &ichMin)); + CheckHr(pts->SearchToLog(pfia->m_ichLimFoundSearch, true, &ichLim)); + if (ichMin == ichLim) + { + if (ichMin > 0 || !m_fUseRegularExpressions || !m_stuCompiled.Equals(L"^")) + { + ichLim = CheckedPatternPosition(static_cast<__int64>(ichLim) + 1); + if (ichLim > ichEnd) + ichLim = ichEnd; + } + } + + m_ichMinFoundLog = ichMin; + m_ichLimFoundLog = ichLim; + m_qtsWhereFound = pts; + if (ichLim == ichLimLastMatch) + { + ichStartSearch = CheckedPatternPosition(static_cast<__int64>(ichLim) + 1); + continue; + } + ichLimLastMatch = ichLim; + + ITsStringPtr qtssReplacement; + CheckHr(get_ReplacementText(&qtssReplacement)); + if (!qtsb) + CheckHr(qtssSource->GetBldr(&qtsb)); + int ichMinResult; + int ichLimResult; + CheckHr(pts->LogToRen(ichMin, &ichMinResult)); + CheckHr(pts->LogToRen(ichLim, &ichLimResult)); + Replacement repl = { ichMinResult, ichLimResult, qtssReplacement }; + vrepl.Push(repl); + cMatches++; + ichStartSearch = ichLim; + } + } + for (int irepl = vrepl.Size() - 1; irepl >= 0; --irepl) + { + CheckHr(qtsb->ReplaceTsString(vrepl[irepl].ichMin, vrepl[irepl].ichLim, + vrepl[irepl].qtss)); + } + + ITsStringPtr qtssResult; + if (qtsb) + CheckHr(qtsb->GetString(&qtssResult)); + else + qtssResult = qtssSource; + m_ichMinFoundLog = -1; + m_ichLimFoundLog = -1; + m_qtsWhereFound = pts; + *pptssResult = qtssResult.Detach(); + *pcMatches = cMatches; +} + +STDMETHODIMP VwPattern::ReplaceAllIn(ITsString * ptss, int ichStart, int ichEnd, + int * pcMatches, ITsString ** pptssResult) +{ + BEGIN_COM_METHOD; + ChkComOutPtr(pcMatches); + *pcMatches = 0; + ChkComOutPtr(pptssResult); + *pptssResult = NULL; + ChkComArgPtr(ptss); + if (ichStart < 0 || ichEnd < ichStart) + ThrowHr(WarnHr(E_INVALIDARG)); + int cch; + CheckHr(ptss->get_Length(&cch)); + if (ichEnd > cch) + ThrowHr(WarnHr(E_INVALIDARG)); + IVwTxtSrcInitPtr qtsi; + qtsi.CreateInstance(CLSID_VwStringTextSource); + CheckHr(qtsi->SetString(ptss)); + IVwTextSourcePtr qts; + CheckHr(qtsi->QueryInterface(IID_IVwTextSource, (void **)&qts)); + int cMatches = 0; + ITsStringPtr qtssResult; + if (m_fUseRegularExpressions) + { + RegExFindInAlgorithm refia(qts, ichStart, ichEnd, true, NULL, this); + ReplaceAllWithAlgorithm(&refia, qts, ptss, ichStart, ichEnd, &cMatches, + &qtssResult); + } + else + { + FindInAlgorithm fia(qts, ichStart, ichEnd, true, NULL, this); + ReplaceAllWithAlgorithm(&fia, qts, ptss, ichStart, ichEnd, &cMatches, + &qtssResult); + } + *pptssResult = qtssResult.Detach(); + *pcMatches = cMatches; + END_COM_METHOD(g_fact, IID_IVwPattern2); +} + /*---------------------------------------------------------------------------------------------- Install the current Find result as the active selection. ----------------------------------------------------------------------------------------------*/ diff --git a/Src/views/VwPattern.h b/Src/views/VwPattern.h index f4db25965f..032f35f532 100644 --- a/Src/views/VwPattern.h +++ b/Src/views/VwPattern.h @@ -21,7 +21,7 @@ Last reviewed: Not yet. This class implements a search pattern and the top level mechanisms to do the actual searching. @h3{Hungarian: zpat} ----------------------------------------------------------------------------------------------*/ -class VwPattern : public IVwPattern +class VwPattern : public IVwPattern2 { friend class VwLazyBox; // Can set relevant instance vars on successful find. friend class FindInAlgorithm; // used in method implementation. @@ -105,6 +105,8 @@ class VwPattern : public IVwPattern STDMETHOD(get_ErrorMessage)(BSTR * pbstrMsg); STDMETHOD(get_ReplacementText)(ITsString ** pptssText); STDMETHOD(get_Group)(int iGroup, ITsString ** pptssGroup); + STDMETHOD(ReplaceAllIn)(ITsString * ptss, int ichStart, int ichEnd, + int * pcMatches, ITsString ** pptssResult); // Other public methods @@ -180,6 +182,9 @@ class VwPattern : public IVwPattern // Other protected methods void Compile(); void CleanupRegexPattern(); + void ReplaceAllWithAlgorithm(FindInAlgorithmBase * pfia, IVwTextSource * pts, + ITsString * ptssSource, + int ichStart, int ichEnd, int * pcMatches, ITsString ** pptssResult); }; diff --git a/Src/xWorks/xWorksTests/Avalonia/Performance/ReplaceWithMethodTests.cs b/Src/xWorks/xWorksTests/Avalonia/Performance/ReplaceWithMethodTests.cs new file mode 100644 index 0000000000..8765dd73ad --- /dev/null +++ b/Src/xWorks/xWorksTests/Avalonia/Performance/ReplaceWithMethodTests.cs @@ -0,0 +1,489 @@ +// Copyright (c) 2026 SIL International +// This software is licensed under the LGPL, version 2.1 or later +// (http://www.gnu.org/licenses/lgpl-2.1.html) + +using System; +using System.Collections.Generic; +using System.Linq; +using System.Runtime.InteropServices; +using System.Text; +using System.Threading; +using System.Xml; +using NUnit.Framework; +using SIL.FieldWorks.Common.Controls; +using SIL.FieldWorks.Common.ViewsInterfaces; +using SIL.LCModel; +using SIL.LCModel.Application; +using SIL.LCModel.Core.KernelInterfaces; +using SIL.LCModel.Core.Text; +using SIL.LCModel.Infrastructure; + +namespace SIL.FieldWorks.XWorks.Performance +{ + [TestFixture] + [Category("BulkReplacement")] + [Apartment(ApartmentState.STA)] + public class ReplaceWithMethodPreviewTests : BulkEditBarTestsBase + { + [TestCase(0)] + [TestCase(1)] + [TestCase(2)] + [TestCase(4)] + public void FakeDoit_UsesOneReplacementPassAndProducesTheImmediateApplyResult(int matchingRows) + { + var entries = CreateEntries(4, matchingRows, "old", "keep"); + var document = BuildColumnSpec(); + var accessor = FieldReadWriter.Create(document.DocumentElement, Cache); + var pattern = BuildPattern("old", "new"); + var method = new CountingReplaceWithMethod(Cache, m_bv.SpecialCache, accessor, + document.DocumentElement, pattern, pattern.ReplaceWith); + var sentinels = entries.ToDictionary(entry => entry.Hvo, + entry => TsStringUtils.MakeString("previous preview " + entry.Hvo, Cache.DefaultVernWs)); + + foreach (var entry in entries) + m_bv.SpecialCache.SetString(entry.Hvo, XMLViewsDataCache.ktagAlternateValue, sentinels[entry.Hvo]); + + method.FakeDoit(entries.Select(entry => entry.Hvo), XMLViewsDataCache.ktagAlternateValue, + XMLViewsDataCache.ktagItemEnabled, new NullProgressState()); + + Assert.That(method.FindInCount, Is.Zero, + "the bulk replacement capability must not restart the public search for each match"); + Assert.That(method.ReplaceAllInCount, Is.EqualTo(entries.Count), + "each eligible value must use exactly one bulk replacement call"); + + Cache.DomainDataByFlid.BeginUndoTask("preview apply parity", "preview apply parity"); + try + { + try + { + foreach (var entry in entries) + method.Doit(entry.Hvo); + } + finally + { + Cache.DomainDataByFlid.EndUndoTask(); + } + + foreach (var entry in entries) + { + var matched = entries.IndexOf(entry) < matchingRows; + var enabled = m_bv.SpecialCache.get_IntProp(entry.Hvo, XMLViewsDataCache.ktagItemEnabled); + var preview = m_bv.SpecialCache.get_StringProp(entry.Hvo, XMLViewsDataCache.ktagAlternateValue); + Assert.That(enabled, Is.EqualTo(matched ? 1 : 0)); + Assert.That(accessor.CurrentValue(entry.Hvo).Text, Is.EqualTo(matched ? "new" : "keep")); + if (!matched) + Assert.That(preview, Is.SameAs(sentinels[entry.Hvo])); + else + Assert.That(preview.Text, Is.EqualTo(accessor.CurrentValue(entry.Hvo).Text)); + } + Assert.That(method.ReplaceAllInCount, Is.EqualTo(entries.Count * 2)); + Assert.That(method.FindInCount, Is.Zero); + } + finally + { + if (matchingRows > 0) + Cache.ActionHandlerAccessor.Undo(); + } + } + + [TestCase("old-old", "old", "new", false, false, false, false, "new-new")] + [TestCase("ab-12 cd-34", "([a-z]+)-([0-9]+)", "$2:$1", true, false, false, false, "12:ab 34:cd")] + [TestCase("cafeteria cafe", "cafe", "tea", false, true, true, true, "cafeteria tea")] + [TestCase("CAF\u00C9 cafe", "cafe", "tea", false, false, false, false, "tea tea")] + [TestCase("cafe\u0301", "caf\u00e9", "tea", false, false, true, true, "tea")] + [TestCase("old", "^", "new", true, false, true, true, "newold")] + public void FakeDoit_MatchesImmediateApplyAcrossPatternModes(string source, string find, + string replace, bool regularExpression, bool wholeWord, bool matchCase, bool matchDiacritics, + string expected) + { + var entry = CreateEntries(1, 1, source, source).Single(); + var document = BuildColumnSpec(); + var accessor = FieldReadWriter.Create(document.DocumentElement, Cache); + var pattern = BuildPattern(find, replace, regularExpression, wholeWord, matchCase, matchDiacritics); + var method = new CountingReplaceWithMethod(Cache, m_bv.SpecialCache, accessor, + document.DocumentElement, pattern, pattern.ReplaceWith); + + method.FakeDoit(new[] { entry.Hvo }, XMLViewsDataCache.ktagAlternateValue, + XMLViewsDataCache.ktagItemEnabled, new NullProgressState()); + + Assert.That(method.ReplaceAllInCount, Is.EqualTo(1)); + Assert.That(method.FindInCount, Is.Zero); + Assert.That(m_bv.SpecialCache.get_IntProp(entry.Hvo, XMLViewsDataCache.ktagItemEnabled), Is.EqualTo(1)); + Assert.That(m_bv.SpecialCache.get_StringProp(entry.Hvo, XMLViewsDataCache.ktagAlternateValue).Text, + Is.EqualTo(expected)); + + Cache.DomainDataByFlid.BeginUndoTask("preview apply parity", "preview apply parity"); + try + { + try + { + method.Doit(entry.Hvo); + } + finally + { + Cache.DomainDataByFlid.EndUndoTask(); + } + + Assert.That(accessor.CurrentValue(entry.Hvo).Text, Is.EqualTo(expected)); + Assert.That(method.ReplaceAllInCount, Is.EqualTo(2)); + Assert.That(method.FindInCount, Is.Zero); + } + finally + { + Cache.ActionHandlerAccessor.Undo(); + } + } + + [Test] + public void FakeDoit_MatchesImmediateApplyForMultiStringRichRuns() + { + ILexEntry entry = null; + ILexSense sense = null; + NonUndoableUnitOfWorkHelper.Do(Cache.ActionHandlerAccessor, () => + { + entry = Cache.ServiceLocator.GetInstance().Create(); + sense = Cache.ServiceLocator.GetInstance().Create(entry, null, string.Empty); + m_createdObjectList.Add(entry); + }); + var document = BuildColumnSpec(); + var accessor = new OwnMlPropReadWriter(Cache, LexSenseTags.kflidDefinition, Cache.DefaultAnalWs); + var sourceBuilder = TsStringUtils.MakeString("old keep", Cache.DefaultAnalWs).GetBldr(); + sourceBuilder.SetIntPropValues(4, 8, (int)FwTextPropType.ktptWs, + (int)FwTextPropVar.ktpvDefault, Cache.DefaultVernWs); + sourceBuilder.SetStrPropValue(4, 8, (int)FwTextPropType.ktptNamedStyle, "Emphasis"); + sourceBuilder.SetStrPropValue(4, 8, (int)FwTextPropType.ktptObjData, + (char)FwObjDataTypes.kodtExternalPathName + "https://example.test/keep"); + var source = sourceBuilder.GetString(); + NonUndoableUnitOfWorkHelper.Do(Cache.ActionHandlerAccessor, + () => accessor.SetNewValue(sense.Hvo, source)); + Assert.That(accessor.CurrentValue(sense.Hvo).RunCount, Is.EqualTo(2)); + + var pattern = BuildPattern("old", "new"); + var method = new CountingReplaceWithMethod(Cache, m_bv.SpecialCache, accessor, + document.DocumentElement, pattern, pattern.ReplaceWith); + method.FakeDoit(new[] { sense.Hvo }, XMLViewsDataCache.ktagAlternateValue, + XMLViewsDataCache.ktagItemEnabled, new NullProgressState()); + var preview = m_bv.SpecialCache.get_StringProp(sense.Hvo, XMLViewsDataCache.ktagAlternateValue); + + Assert.That(method.ReplaceAllInCount, Is.EqualTo(1)); + Assert.That(method.FindInCount, Is.Zero); + Assert.That(preview.Text, Is.EqualTo("new keep")); + var keepRun = Enumerable.Range(0, preview.RunCount).Single(i => preview.get_RunText(i) == "keep"); + Assert.That(TsStringUtils.GetWsOfRun(preview, keepRun), Is.EqualTo(Cache.DefaultVernWs)); + Assert.That(preview.get_Properties(keepRun).GetStrPropValue((int)FwTextPropType.ktptNamedStyle), + Is.EqualTo("Emphasis")); + Assert.That(preview.get_Properties(keepRun).GetStrPropValue((int)FwTextPropType.ktptObjData), + Is.EqualTo((char)FwObjDataTypes.kodtExternalPathName + "https://example.test/keep")); + + Cache.DomainDataByFlid.BeginUndoTask("preview apply parity", "preview apply parity"); + try + { + try + { + method.Doit(sense.Hvo); + } + finally + { + Cache.DomainDataByFlid.EndUndoTask(); + } + + Assert.That(preview.Equals(accessor.CurrentValue(sense.Hvo)), Is.True); + Assert.That(method.ReplaceAllInCount, Is.EqualTo(2)); + Assert.That(method.FindInCount, Is.Zero); + } + finally + { + Cache.ActionHandlerAccessor.Undo(); + } + } + + [TestCase("old-old", true, "new-new", 3)] + [TestCase("keep", false, null, 1)] + public void FakeDoit_FallsBackWhenBulkReplacementIsUnavailable(string source, + bool matched, string expected, int expectedFindInCount) + { + var entry = CreateEntries(1, 1, source, source).Single(); + var document = BuildColumnSpec(); + var accessor = FieldReadWriter.Create(document.DocumentElement, Cache); + var pattern = BuildPattern("old", "new"); + var method = new CountingReplaceWithMethod(Cache, m_bv.SpecialCache, accessor, + document.DocumentElement, pattern, pattern.ReplaceWith, false); + var sentinel = TsStringUtils.MakeString("previous preview", Cache.DefaultVernWs); + m_bv.SpecialCache.SetString(entry.Hvo, XMLViewsDataCache.ktagAlternateValue, sentinel); + + method.FakeDoit(new[] { entry.Hvo }, XMLViewsDataCache.ktagAlternateValue, + XMLViewsDataCache.ktagItemEnabled, new NullProgressState()); + + Assert.That(method.FindInCount, Is.EqualTo(expectedFindInCount)); + Assert.That(m_bv.SpecialCache.get_IntProp(entry.Hvo, XMLViewsDataCache.ktagItemEnabled), + Is.EqualTo(matched ? 1 : 0)); + var preview = m_bv.SpecialCache.get_StringProp(entry.Hvo, + XMLViewsDataCache.ktagAlternateValue); + if (matched) + Assert.That(preview.Text, Is.EqualTo(expected)); + else + Assert.That(preview, Is.SameAs(sentinel)); + } + + [Test] + public void FakeDoit_PropagatesBulkReplacementFailureWithoutFallback() + { + var entry = CreateEntries(1, 1, "old", "old").Single(); + var document = BuildColumnSpec(); + var accessor = FieldReadWriter.Create(document.DocumentElement, Cache); + var pattern = BuildPattern("old", "new"); + var failure = new COMException("bulk replacement failed", unchecked((int)0x80004005)); + var method = new CountingReplaceWithMethod(Cache, m_bv.SpecialCache, accessor, + document.DocumentElement, pattern, pattern.ReplaceWith, true, failure); + + var actual = Assert.Throws(() => method.FakeDoit(new[] { entry.Hvo }, + XMLViewsDataCache.ktagAlternateValue, XMLViewsDataCache.ktagItemEnabled, + new NullProgressState())); + + Assert.That(actual, Is.SameAs(failure)); + Assert.That(method.ReplaceAllInCount, Is.EqualTo(1)); + Assert.That(method.FindInCount, Is.Zero); + } + + [Test] + public void FakeDoit_NormalizesBulkReplacementResultToNfd() + { + var entry = CreateEntries(1, 1, "old caf\u00e9", "old caf\u00e9").Single(); + var document = BuildColumnSpec(); + var accessor = FieldReadWriter.Create(document.DocumentElement, Cache); + var pattern = BuildPattern("old", "new"); + var method = new CountingReplaceWithMethod(Cache, m_bv.SpecialCache, accessor, + document.DocumentElement, pattern, pattern.ReplaceWith); + + method.FakeDoit(new[] { entry.Hvo }, XMLViewsDataCache.ktagAlternateValue, + XMLViewsDataCache.ktagItemEnabled, new NullProgressState()); + + var preview = m_bv.SpecialCache.get_StringProp(entry.Hvo, + XMLViewsDataCache.ktagAlternateValue); + Assert.That(method.ReplaceAllInCount, Is.EqualTo(1)); + Assert.That(method.FindInCount, Is.Zero); + Assert.That(preview.Text, Is.EqualTo("new cafe\u0301")); + Assert.That(preview.Text.IsNormalized(NormalizationForm.FormD), Is.True); + } + + [Test] + public void FakeDoit_BaseGateRejectionDoesNotSearchOrChangePreview() + { + var entry = CreateEntries(1, 1, "old", "old").Single(); + var document = BuildColumnSpec(); + var accessor = FieldReadWriter.Create(document.DocumentElement, Cache); + var pattern = BuildPattern("old", "new"); + var method = new CountingReplaceWithMethod(Cache, m_bv.SpecialCache, accessor, + document.DocumentElement, pattern, pattern.ReplaceWith, canChange: false); + var sentinel = TsStringUtils.MakeString("previous preview", Cache.DefaultVernWs); + m_bv.SpecialCache.SetString(entry.Hvo, XMLViewsDataCache.ktagAlternateValue, sentinel); + + method.FakeDoit(new[] { entry.Hvo }, XMLViewsDataCache.ktagAlternateValue, + XMLViewsDataCache.ktagItemEnabled, new NullProgressState()); + + Assert.That(method.ReplaceAllInCount, Is.Zero); + Assert.That(method.FindInCount, Is.Zero); + Assert.That(m_bv.SpecialCache.get_IntProp(entry.Hvo, + XMLViewsDataCache.ktagItemEnabled), Is.Zero); + Assert.That(m_bv.SpecialCache.get_StringProp(entry.Hvo, + XMLViewsDataCache.ktagAlternateValue), Is.SameAs(sentinel)); + Assert.That(accessor.CurrentValue(entry.Hvo).Text, Is.EqualTo("old")); + } + + private List CreateEntries(int count, int matchingRows, string matchedValue, + string unmatchedValue) + { + var entries = new List(count); + NonUndoableUnitOfWorkHelper.Do(Cache.ActionHandlerAccessor, () => + { + for (var index = 0; index < count; index++) + { + var entry = Cache.ServiceLocator.GetInstance().Create(); + entry.CitationForm.set_String(Cache.DefaultVernWs, + index < matchingRows ? matchedValue : unmatchedValue); + entries.Add(entry); + m_createdObjectList.Add(entry); + } + }); + return entries; + } + + private static XmlDocument BuildColumnSpec() + { + var document = new XmlDocument(); + document.LoadXml(""); + return document; + } + + private IVwPattern BuildPattern(string find, string replace, bool regularExpression = false, + bool wholeWord = false, bool matchCase = true, bool matchDiacritics = true) + { + var pattern = VwPatternClass.Create(); + pattern.Pattern = TsStringUtils.MakeString(find, Cache.DefaultVernWs); + pattern.ReplaceWith = TsStringUtils.MakeString(replace, Cache.DefaultVernWs); + pattern.UseRegularExpressions = regularExpression; + pattern.MatchWholeWord = wholeWord; + pattern.MatchCase = matchCase; + pattern.MatchDiacritics = matchDiacritics; + return pattern; + } + + internal sealed class CountingReplaceWithMethod : ReplaceWithMethod + { + private readonly bool m_exposeBulkReplacement; + private readonly Exception m_bulkReplacementFailure; + private readonly bool? m_canChange; + + internal CountingReplaceWithMethod(LcmCache cache, ISilDataAccessManaged sda, + FieldReadWriter accessor, XmlNode spec, IVwPattern pattern, ITsString replacement, + bool exposeBulkReplacement = true, Exception bulkReplacementFailure = null, + bool? canChange = null) + : base(cache, sda, accessor, spec, pattern, replacement) + { + m_exposeBulkReplacement = exposeBulkReplacement; + m_bulkReplacementFailure = bulkReplacementFailure; + m_canChange = canChange; + } + + internal int FindInCount { get; private set; } + internal int ReplaceAllInCount { get; private set; } + + protected override bool OkToChange(int hvo) + { + return m_canChange ?? base.OkToChange(hvo); + } + + protected override bool TryReplaceAllIn(ITsString source, out ITsString result, + out int matchCount) + { + ReplaceAllInCount++; + if (m_bulkReplacementFailure != null) + throw m_bulkReplacementFailure; + if (!m_exposeBulkReplacement) + { + result = null; + matchCount = 0; + return false; + } + return base.TryReplaceAllIn(source, out result, out matchCount); + } + + protected override void FindIn(int ichStart, int ichEnd, out int ichMin, out int ichLim) + { + FindInCount++; + base.FindIn(ichStart, ichEnd, out ichMin, out ichLim); + } + } + } + + [TestFixture] + [Category("BulkReplacement")] + [Apartment(ApartmentState.STA)] + public class ReplaceAllInDecoratorCorrectnessTests : BulkEditBarTestsBase + { + [Test] + public void FakeDoit_MatchesOracleForDecoratorBackedUnnormalizedRichValue() + { + ILexEntry entry = null; + NonUndoableUnitOfWorkHelper.Do(Cache.ActionHandlerAccessor, () => + { + entry = Cache.ServiceLocator.GetInstance().Create(); + m_createdObjectList.Add(entry); + }); + var sourceBuilder = TsStringUtils.MakeString("old cafe\u0301 old keep", + Cache.DefaultVernWs).GetBldr(); + sourceBuilder.SetIntPropValues(4, 9, (int)FwTextPropType.ktptWs, + (int)FwTextPropVar.ktpvDefault, Cache.DefaultAnalWs); + sourceBuilder.SetStrPropValue(4, 9, (int)FwTextPropType.ktptNamedStyle, + "Unnormalized Source"); + sourceBuilder.SetStrPropValue(14, 18, (int)FwTextPropType.ktptObjData, + (char)FwObjDataTypes.kodtExternalPathName + "https://example.test/keep"); + m_bv.SpecialCache.SetString(entry.Hvo, XMLViewsDataCache.ktagAlternateValue, + sourceBuilder.GetString()); + var decoratedValue = m_bv.SpecialCache.get_StringProp(entry.Hvo, + XMLViewsDataCache.ktagAlternateValue); + Assert.That(decoratedValue.Text.IsNormalized(NormalizationForm.FormC), Is.False); + Assert.That(decoratedValue.RunCount, Is.GreaterThan(1)); + + var pattern = VwPatternClass.Create(); + pattern.Pattern = TsStringUtils.MakeString("old", Cache.DefaultVernWs); + pattern.ReplaceWith = TsStringUtils.MakeString("new", Cache.DefaultVernWs); + pattern.MatchCase = true; + pattern.MatchDiacritics = true; + var replacementBuilder = pattern.ReplaceWith.GetBldr(); + replacementBuilder.SetStrPropValue(0, 3, (int)FwTextPropType.ktptNamedStyle, + "Decorator Replacement"); + pattern.ReplaceWith = replacementBuilder.GetString(); + var oracleInit = VwStringTextSourceClass.Create(); + oracleInit.SetString(decoratedValue); + var oracleSource = (IVwTextSource)oracleInit; + var expectedBuilder = decoratedValue.GetBldr(); + var expectedCount = 0; + var ichStart = 0; + var delta = 0; + while (ichStart <= decoratedValue.Length) + { + pattern.FindIn(oracleSource, ichStart, decoratedValue.Length, true, + out var ichMin, out var ichLim, null); + if (ichMin < 0) + break; + var replacement = pattern.ReplacementText; + expectedBuilder.ReplaceTsString(ichMin + delta, ichLim + delta, replacement); + delta += replacement.Length - (ichLim - ichMin); + expectedCount++; + ichStart = ichLim; + } + + Assert.That(expectedCount, Is.EqualTo(2)); + var document = new XmlDocument(); + document.LoadXml(""); + var accessor = new DecoratorStringReadWriter(m_bv.SpecialCache, + XMLViewsDataCache.ktagAlternateValue, Cache.DefaultVernWs); + var method = new ReplaceWithMethodPreviewTests.CountingReplaceWithMethod(Cache, + m_bv.SpecialCache, accessor, document.DocumentElement, pattern, pattern.ReplaceWith); + + method.FakeDoit(new[] { entry.Hvo }, XMLViewsDataCache.ktagAlternateValue, + XMLViewsDataCache.ktagItemEnabled, new NullProgressState()); + + var actual = m_bv.SpecialCache.get_StringProp(entry.Hvo, + XMLViewsDataCache.ktagAlternateValue); + var expected = expectedBuilder.GetString().get_NormalizedForm(FwNormalizationMode.knmNFD); + Assert.That(method.ReplaceAllInCount, Is.EqualTo(1)); + Assert.That(method.FindInCount, Is.Zero); + Assert.That(m_bv.SpecialCache.get_IntProp(entry.Hvo, + XMLViewsDataCache.ktagItemEnabled), Is.EqualTo(1)); + Assert.That(actual.Text, Is.EqualTo("new cafe\u0301 new keep")); + Assert.That(actual.Text.IsNormalized(NormalizationForm.FormD), Is.True); + Assert.That(TsStringHelper.TsStringsAreEqual(expected, actual, + out var differences), Is.True, differences); + } + + private sealed class DecoratorStringReadWriter : FieldReadWriter + { + private readonly int m_tag; + private readonly int m_writingSystem; + + internal DecoratorStringReadWriter(ISilDataAccess dataAccess, int tag, int writingSystem) + : base(dataAccess) + { + m_tag = tag; + m_writingSystem = writingSystem; + } + + public override ITsString CurrentValue(int hvo) + { + return m_sda.get_StringProp(hvo, m_tag); + } + + public override void SetNewValue(int hvo, ITsString tss) + { + m_sda.SetString(hvo, m_tag, tss); + } + + public override int WritingSystem + { + get { return m_writingSystem; } + } + } + } +} diff --git a/Src/xWorks/xWorksTests/Avalonia/Performance/VwPatternReplacementTests.cs b/Src/xWorks/xWorksTests/Avalonia/Performance/VwPatternReplacementTests.cs new file mode 100644 index 0000000000..8cdf3ab56e --- /dev/null +++ b/Src/xWorks/xWorksTests/Avalonia/Performance/VwPatternReplacementTests.cs @@ -0,0 +1,490 @@ +// Copyright (c) 2026 SIL International +// This software is licensed under the LGPL, version 2.1 or later +// (http://www.gnu.org/licenses/lgpl-2.1.html) + +using System.Collections.Generic; +using System.Runtime.InteropServices; +using NUnit.Framework; +using SIL.FieldWorks.Common.ViewsInterfaces; +using SIL.LCModel; +using SIL.LCModel.Core.KernelInterfaces; +using SIL.LCModel.Core.Text; + +namespace SIL.FieldWorks.XWorks.Performance +{ + [TestFixture] + [Category("BulkReplacement")] + public class VwPatternReplacementTests : MemoryOnlyBackendProviderTestBase + { + private int Ws => Cache.DefaultVernWs; + + private IVwPattern MakePattern(string patternText, bool matchCase = false, + bool matchDiacritics = false, bool matchWholeWord = false) + { + var pattern = VwPatternClass.Create(); + pattern.Pattern = TsStringUtils.MakeString(patternText, Ws); + pattern.MatchCase = matchCase; + pattern.MatchDiacritics = matchDiacritics; + pattern.MatchWholeWord = matchWholeWord; + return pattern; + } + + private IVwTextSource MakeTextSource(string text, out int length) + { + return MakeTextSource(TsStringUtils.MakeString(text, Ws), out length); + } + + private static IVwTextSource MakeTextSource(ITsString tss, out int length) + { + length = tss.Length; + var textSourceInit = VwStringTextSourceClass.Create(); + textSourceInit.SetString(tss); + return (IVwTextSource)textSourceInit; + } + + private static void Find(IVwPattern pattern, IVwTextSource ts, int ichStart, int ichEnd, + bool forward, out int ichMin, out int ichLim) + { + pattern.FindIn(ts, ichStart, ichEnd, forward, out ichMin, out ichLim, null); + } + + private static void AssertFound(int ichMin, int ichLim, int expectedMin, int expectedLim, string why) + { + Assert.That(ichMin, Is.EqualTo(expectedMin), why + " (ichMin)"); + Assert.That(ichLim, Is.EqualTo(expectedLim), why + " (ichLim)"); + } + + private static void AssertNotFound(int ichMin, string why) + { + Assert.That(ichMin, Is.LessThan(0), why); + } + + private sealed class ReplacementMatch + { + public ReplacementMatch(int ichMin, int ichLim, ITsString replacement) + { + IchMin = ichMin; + IchLim = ichLim; + Replacement = replacement; + } + + public int IchMin { get; } + public int IchLim { get; } + public ITsString Replacement { get; } + } + + /// Describes one expected replacement match. + public sealed class ExpectedReplacementMatch + { + internal ExpectedReplacementMatch(int ichMin, int ichLim, string replacementText) + { + IchMin = ichMin; + IchLim = ichLim; + ReplacementText = replacementText; + } + + internal int IchMin { get; } + internal int IchLim { get; } + internal string ReplacementText { get; } + } + + private static ExpectedReplacementMatch ExpectedMatch(int ichMin, int ichLim, string replacementText) + { + return new ExpectedReplacementMatch(ichMin, ichLim, replacementText); + } + + private static void AssertReplacementMatches(IList actual, + params ExpectedReplacementMatch[] expected) + { + Assert.That(actual, Has.Count.EqualTo(expected.Length)); + for (var index = 0; index < expected.Length; index++) + { + Assert.That(actual[index].IchMin, Is.EqualTo(expected[index].IchMin), "ichMin at match " + index); + Assert.That(actual[index].IchLim, Is.EqualTo(expected[index].IchLim), "ichLim at match " + index); + Assert.That(actual[index].Replacement.Text, Is.EqualTo(expected[index].ReplacementText), + "replacement text at match " + index); + } + } + + private static List CollectReplacementMatches(IVwPattern pattern, + IVwTextSource ts, int cch, int ichStartSearch = 0) + { + var results = new List(); + var ichLimLastMatch = -1; + while (ichStartSearch <= cch) + { + Find(pattern, ts, ichStartSearch, cch, true, out var ichMin, out var ichLim); + if (ichMin < 0) + break; + if (ichLim == ichLimLastMatch) + { + ichStartSearch = ichLim + 1; + continue; + } + ichLimLastMatch = ichLim; + results.Add(new ReplacementMatch(ichMin, ichLim, pattern.ReplacementText)); + ichStartSearch = ichLim; + } + return results; + } + + private static void AssertReplaceAllMatchesRepeatedFindIn(IVwPattern pattern, + IVwTextSource oracleSource, int ichStart, int ichEnd, int? expectedCount = null, + string expectedText = null) + { + var source = oracleSource.GetSubString(0, oracleSource.Length); + var matches = CollectReplacementMatches(pattern, oracleSource, ichEnd, ichStart); + var expectedBuilder = source.GetBldr(); + var delta = 0; + foreach (var match in matches) + { + expectedBuilder.ReplaceTsString(match.IchMin + delta, match.IchLim + delta, + match.Replacement); + delta += match.Replacement.Length - (match.IchLim - match.IchMin); + } + + var actual = ((IVwPattern2)pattern).ReplaceAllIn(source, ichStart, ichEnd, + out var actualCount); + Assert.That(actualCount, Is.EqualTo(matches.Count), "bulk match count"); + if (expectedCount.HasValue) + Assert.That(actualCount, Is.EqualTo(expectedCount.Value), "characterized match count"); + if (expectedText != null) + Assert.That(actual.Text, Is.EqualTo(expectedText), "characterized result text"); + Assert.That(TsStringHelper.TsStringsAreEqual(expectedBuilder.GetString(), actual, + out var differences), Is.True, differences); + } + + private static IEnumerable PlainReplacementCases() + { + yield return new TestCaseData("old", "none", "new", new ExpectedReplacementMatch[0]) + .SetName("ReplacementIteration_NoMatches_ReturnsEmptySequence"); + yield return new TestCaseData("old", "old", "new", new[] { ExpectedMatch(0, 3, "new") }) + .SetName("ReplacementIteration_OneMatch_ReturnsOneReplacement"); + yield return new TestCaseData("old", "oldold", "new", new[] { ExpectedMatch(0, 3, "new"), ExpectedMatch(3, 6, "new") }) + .SetName("ReplacementIteration_AdjacentMatches_ReturnsBothReplacements"); + yield return new TestCaseData("old", "old old old", "new", new[] + { + ExpectedMatch(0, 3, "new"), ExpectedMatch(4, 7, "new"), ExpectedMatch(8, 11, "new") + }).SetName("ReplacementIteration_MultipleMatches_ReturnsOrderedReplacements"); + yield return new TestCaseData("aa", "aaa", "x", new[] { ExpectedMatch(0, 2, "x") }) + .SetName("ReplacementIteration_OverlappingCandidates_ReturnsNonOverlappingMatches"); + } + + [TestCaseSource(nameof(PlainReplacementCases))] + public void ReplacementIteration_PlainTextCases_ReturnExpectedSequence(string patternText, string text, + string replacementText, ExpectedReplacementMatch[] expected) + { + var pattern = MakePattern(patternText); + pattern.ReplaceWith = TsStringUtils.MakeString(replacementText, Ws); + var source = MakeTextSource(text, out var length); + + AssertReplacementMatches(CollectReplacementMatches(pattern, source, length), expected); + AssertReplaceAllMatchesRepeatedFindIn(pattern, source, 0, length); + } + + [TestCase("^", "abc", 0, 0)] + [TestCase("$", "abc", 3, 3)] + public void ReplacementIteration_ZeroLengthRegularExpression_ReturnsOneMatch(string expression, string text, + int ichMin, int ichLim) + { + var pattern = MakePattern(expression); + pattern.UseRegularExpressions = true; + pattern.ReplaceWith = TsStringUtils.MakeString("#", Ws); + var source = MakeTextSource(text, out var length); + + AssertReplacementMatches(CollectReplacementMatches(pattern, source, length), ExpectedMatch(ichMin, ichLim, "#")); + AssertReplaceAllMatchesRepeatedFindIn(pattern, source, 0, length); + } + + [Test] + public void ReplacementIteration_ZeroWidthLookaheadAcrossAstralCharacter_ReturnsCurrentSequence() + { + var pattern = MakePattern("(?=.)"); + pattern.UseRegularExpressions = true; + pattern.ReplaceWith = TsStringUtils.MakeString("#", Ws); + var source = MakeTextSource("\uD83D\uDE00a", out var length); + + AssertReplacementMatches(CollectReplacementMatches(pattern, source, length), + ExpectedMatch(0, 1, "#"), ExpectedMatch(1, 2, "#"), ExpectedMatch(2, 3, "#")); + AssertReplaceAllMatchesRepeatedFindIn(pattern, source, 0, length); + } + + [Test] + public void ReplacementIteration_RegularExpressionCapture_ReturnsExpandedReplacementText() + { + var pattern = MakePattern("(o|e)(ld)"); + pattern.UseRegularExpressions = true; + pattern.ReplaceWith = TsStringUtils.MakeString("$2-$1", Ws); + var source = MakeTextSource("old eld", out var length); + + AssertReplacementMatches(CollectReplacementMatches(pattern, source, length), + ExpectedMatch(0, 3, "ld-o"), ExpectedMatch(4, 7, "ld-e")); + AssertReplaceAllMatchesRepeatedFindIn(pattern, source, 0, length); + } + + /// Describes one locale-sensitive replacement case. + public sealed class CollationReplacementCase + { + internal CollationReplacementCase(string patternText, string text, string locale, string rules, + bool matchDiacritics, ExpectedReplacementMatch[] expected, bool matchCase = true) + { + PatternText = patternText; + Text = text; + Locale = locale; + Rules = rules; + MatchDiacritics = matchDiacritics; + Expected = expected; + MatchCase = matchCase; + } + + internal string PatternText { get; } + internal string Text { get; } + internal string Locale { get; } + internal string Rules { get; } + internal bool MatchDiacritics { get; } + internal ExpectedReplacementMatch[] Expected { get; } + internal bool MatchCase { get; } + } + + private static IEnumerable CollationReplacementCases() + { + yield return new TestCaseData(new CollationReplacementCase("h", "ch", "cs", null, true, + new ExpectedReplacementMatch[0])).SetName("ReplacementIteration_CzechContraction_DoesNotExposeSecondCharacter"); + yield return new TestCaseData(new CollationReplacementCase("a\u0308", " AE ", "de__PHONEBOOK", null, false, + new[] { ExpectedMatch(1, 3, "x") }, matchCase: false)).SetName("ReplacementIteration_GermanPhonebookExpansion_ReturnsOneReplacement"); + yield return new TestCaseData(new CollationReplacementCase("ab", "a-b", "de-u-co-phonebk-ka-shifted", null, true, + new[] { ExpectedMatch(0, 3, "x") })).SetName("ReplacementIteration_ShiftedPunctuation_ReturnsExpandedSpan"); + yield return new TestCaseData(new CollationReplacementCase("a", "b", "root", "&a=b", true, + new[] { ExpectedMatch(0, 1, "x") })).SetName("ReplacementIteration_CustomTailoring_ReturnsTailoredMatch"); + yield return new TestCaseData(new CollationReplacementCase("caf\u00e9", "cafe\u0301", "root", null, true, + new[] { ExpectedMatch(0, 5, "x") })).SetName("ReplacementIteration_NfcPatternAgainstNfdText_ReturnsDecomposedSpan"); + yield return new TestCaseData(new CollationReplacementCase("\u1e09", "c\u0301\u0327", "root", null, true, + new ExpectedReplacementMatch[0])).SetName("ReplacementIteration_NonCanonicalCombiningOrder_ReturnsNoMatch"); + yield return new TestCaseData(new CollationReplacementCase("a", "a\u00ad", "root", null, true, + new[] { ExpectedMatch(0, 2, "x") })).SetName("ReplacementIteration_SoftHyphen_ReturnsExtendedSpan"); + yield return new TestCaseData(new CollationReplacementCase("a", "a\u034f", "root", null, true, + new[] { ExpectedMatch(0, 2, "x") })).SetName("ReplacementIteration_CombiningGraphemeJoiner_ReturnsExtendedSpan"); + yield return new TestCaseData(new CollationReplacementCase("a", "a\u200d", "root", null, true, + new[] { ExpectedMatch(0, 2, "x") })).SetName("ReplacementIteration_ZeroWidthJoiner_ReturnsExtendedSpan"); + yield return new TestCaseData(new CollationReplacementCase("\u0628", "\u0628\u0640", "ar", null, true, + new ExpectedReplacementMatch[0])).SetName("ReplacementIteration_ArabicTatweel_ReturnsNoMatch"); + yield return new TestCaseData(new CollationReplacementCase("\u1820", "\u1820\u180b", "mn", null, true, + new[] { ExpectedMatch(0, 2, "x") })).SetName("ReplacementIteration_MongolianFvs1_ReturnsExtendedSpan"); + yield return new TestCaseData(new CollationReplacementCase("\u1100", "\u1100\u1160", "ko", null, true, + new[] { ExpectedMatch(0, 1, "x") })).SetName("ReplacementIteration_HangulFiller_DoesNotExtendSpan"); + yield return new TestCaseData(new CollationReplacementCase("\u0e01", "\u0e01\u0e4d", "th", null, true, + new[] { ExpectedMatch(0, 1, "x") })).SetName("ReplacementIteration_ThaiNikhahit_DoesNotExtendSpan"); + } + + [TestCaseSource(nameof(CollationReplacementCases))] + public void ReplacementIteration_CollationCases_ReturnExpectedSequence(CollationReplacementCase testCase) + { + var pattern = MakePattern(testCase.PatternText, matchCase: testCase.MatchCase, + matchDiacritics: testCase.MatchDiacritics); + pattern.IcuLocale = testCase.Locale; + if (testCase.Rules != null) + pattern.IcuCollatingRules = testCase.Rules; + pattern.ReplaceWith = TsStringUtils.MakeString("x", Ws); + var source = MakeTextSource(testCase.Text, out var length); + + AssertReplacementMatches(CollectReplacementMatches(pattern, source, length), testCase.Expected); + AssertReplaceAllMatchesRepeatedFindIn(pattern, source, 0, length); + } + + [Test] + public void ReplacementIteration_WholeWord_ReturnsOnlyStandaloneOccurrence() + { + var pattern = MakePattern("cafe", matchWholeWord: true); + pattern.ReplaceWith = TsStringUtils.MakeString("x", Ws); + var source = MakeTextSource("cafeteria cafe", out var length); + + AssertReplacementMatches(CollectReplacementMatches(pattern, source, length), ExpectedMatch(10, 14, "x")); + AssertReplaceAllMatchesRepeatedFindIn(pattern, source, 0, length); + } + + [Test] + public void ReplacementIteration_WritingSystem_ReturnsOnlyMatchingRun() + { + var sourceBuilder = TsStringUtils.MakeString("old old", Ws).GetBldr(); + sourceBuilder.SetIntPropValues(4, 7, (int)FwTextPropType.ktptWs, + (int)FwTextPropVar.ktpvDefault, Cache.DefaultAnalWs); + var source = MakeTextSource(sourceBuilder.GetString(), out var length); + + var propertyPatternBuilder = TsStringUtils.MakeString("old", Cache.DefaultAnalWs).GetBldr(); + var pattern = MakePattern("old"); + pattern.Pattern = propertyPatternBuilder.GetString(); + pattern.MatchOldWritingSystem = true; + pattern.ReplaceWith = TsStringUtils.MakeString("x", Ws); + + AssertReplacementMatches(CollectReplacementMatches(pattern, source, length), ExpectedMatch(4, 7, "x")); + AssertReplaceAllMatchesRepeatedFindIn(pattern, source, 0, length); + } + + [Test] + public void ReplacementIteration_Style_ReturnsOnlyMatchingRun() + { + var sourceBuilder = TsStringUtils.MakeString("old old", Ws).GetBldr(); + sourceBuilder.SetStrPropValue(4, 7, (int)FwTextPropType.ktptNamedStyle, "Emphasis"); + var source = MakeTextSource(sourceBuilder.GetString(), out var length); + + var patternBuilder = TsStringUtils.MakeString("old", Ws).GetBldr(); + patternBuilder.SetStrPropValue(0, 3, (int)FwTextPropType.ktptNamedStyle, "Emphasis"); + var pattern = MakePattern("old"); + pattern.Pattern = patternBuilder.GetString(); + pattern.ReplaceWith = TsStringUtils.MakeString("x", Ws); + + AssertReplacementMatches(CollectReplacementMatches(pattern, source, length), ExpectedMatch(4, 7, "x")); + AssertReplaceAllMatchesRepeatedFindIn(pattern, source, 0, length); + } + + [Test] + public void ReplacementIteration_Tag_ReturnsOnlyMatchingRun() + { + var sourceBuilder = TsStringUtils.MakeString("old old", Ws).GetBldr(); + sourceBuilder.SetStrPropValue(4, 7, (int)FwTextPropType.ktptTags, "tag\0value"); + var source = MakeTextSource(sourceBuilder.GetString(), out var length); + + var patternBuilder = TsStringUtils.MakeString("old", Ws).GetBldr(); + patternBuilder.SetStrPropValue(0, 3, (int)FwTextPropType.ktptTags, "tag\0value"); + var pattern = MakePattern("old"); + pattern.Pattern = patternBuilder.GetString(); + pattern.ReplaceWith = TsStringUtils.MakeString("x", Ws); + + AssertReplacementMatches(CollectReplacementMatches(pattern, source, length), ExpectedMatch(4, 7, "x")); + AssertReplaceAllMatchesRepeatedFindIn(pattern, source, 0, length); + } + + [Test] + public void ReplacementIteration_CaptureReplacement_PreservesReplacementRunProperties() + { + var pattern = MakePattern("(old)"); + pattern.UseRegularExpressions = true; + pattern.MatchOldWritingSystem = true; + var replacementBuilder = TsStringUtils.MakeString("$1", Cache.DefaultAnalWs).GetBldr(); + replacementBuilder.SetStrPropValue(0, 2, (int)FwTextPropType.ktptNamedStyle, "Replacement Style"); + pattern.ReplaceWith = replacementBuilder.GetString(); + var source = MakeTextSource("old", out var length); + + var matches = CollectReplacementMatches(pattern, source, length); + AssertReplacementMatches(matches, ExpectedMatch(0, 3, "old")); + Assert.That(TsStringUtils.GetWsOfRun(matches[0].Replacement, 0), Is.EqualTo(Cache.DefaultAnalWs)); + Assert.That(matches[0].Replacement.get_Properties(0) + .GetStrPropValue((int)FwTextPropType.ktptNamedStyle), Is.EqualTo("Replacement Style")); + AssertReplaceAllMatchesRepeatedFindIn(pattern, source, 0, length); + } + + [Test] + public void ReplaceAllIn_PartialRange_EqualsRepeatedFindIn() + { + var pattern = MakePattern("old"); + pattern.ReplaceWith = TsStringUtils.MakeString("new", Ws); + var source = MakeTextSource("old old old", out var length); + + AssertReplaceAllMatchesRepeatedFindIn(pattern, source, 4, length - 1); + } + + [TestCase(false, 2, "\u0645 \u0645")] + [TestCase(true, 2, "\u0645 \u0645")] + public void ReplaceAllIn_ArabicHarakat_EqualsRepeatedFindIn(bool matchDiacritics, + int expectedCount, string expectedText) + { + var pattern = MakePattern("\u0628\u064e", matchDiacritics: matchDiacritics); + pattern.IcuLocale = "ar"; + var replacementBuilder = TsStringUtils.MakeString("\u0645", Ws).GetBldr(); + replacementBuilder.SetStrPropValue(0, 1, (int)FwTextPropType.ktptNamedStyle, + "Arabic Replacement"); + pattern.ReplaceWith = replacementBuilder.GetString(); + var sourceBuilder = TsStringUtils.MakeString("\u0628 \u0628\u064e", Ws).GetBldr(); + sourceBuilder.SetStrPropValue(2, 4, (int)FwTextPropType.ktptNamedStyle, + "Arabic Source"); + var source = MakeTextSource(sourceBuilder.GetString(), out var length); + + AssertReplaceAllMatchesRepeatedFindIn(pattern, source, 0, length, expectedCount, + expectedText); + } + + [Test] + public void ReplaceAllIn_EmptyTextPattern_ReplacesRunMatchingAllProperties() + { + const string style = "Target Style"; + const string tags = "target\0tag"; + var sourceBuilder = TsStringUtils.MakeString("one two three", Ws).GetBldr(); + sourceBuilder.SetIntPropValues(4, 7, (int)FwTextPropType.ktptWs, + (int)FwTextPropVar.ktpvDefault, Cache.DefaultAnalWs); + sourceBuilder.SetStrPropValue(4, 7, (int)FwTextPropType.ktptNamedStyle, style); + sourceBuilder.SetStrPropValue(4, 7, (int)FwTextPropType.ktptTags, tags); + var source = sourceBuilder.GetString(); + + var patternBuilder = TsStringUtils.MakeString(string.Empty, Cache.DefaultAnalWs).GetBldr(); + patternBuilder.SetStrPropValue(0, 0, (int)FwTextPropType.ktptNamedStyle, style); + patternBuilder.SetStrPropValue(0, 0, (int)FwTextPropType.ktptTags, tags); + var pattern = MakePattern(string.Empty); + pattern.Pattern = patternBuilder.GetString(); + pattern.MatchOldWritingSystem = true; + var replacementBuilder = TsStringUtils.MakeString("X", Ws).GetBldr(); + replacementBuilder.SetStrPropValue(0, 1, (int)FwTextPropType.ktptNamedStyle, + "Replacement Style"); + pattern.ReplaceWith = replacementBuilder.GetString(); + + var actual = ((IVwPattern2)pattern).ReplaceAllIn(source, 0, source.Length, + out var count); + + Assert.That(count, Is.EqualTo(1)); + Assert.That(actual.Text, Is.EqualTo("one X three")); + Assert.That(actual.RunCount, Is.EqualTo(3)); + Assert.That(actual.get_Properties(1).GetStrPropValue( + (int)FwTextPropType.ktptNamedStyle), Is.EqualTo("Replacement Style")); + Assert.That(TsStringUtils.GetWsOfRun(actual, 1), Is.EqualTo(Ws)); + } + + [Test] + public void ReplaceAllIn_PreservesObjectReplacementCharacterAndObjectData() + { + const string objectData = "\u0001object-data"; + var sourceBuilder = TsStringUtils.MakeString("old \uFFFC keep", Ws).GetBldr(); + sourceBuilder.SetStrPropValue(4, 5, (int)FwTextPropType.ktptObjData, objectData); + var source = sourceBuilder.GetString(); + var pattern = MakePattern("old"); + pattern.ReplaceWith = TsStringUtils.MakeString("new", Ws); + + var actual = ((IVwPattern2)pattern).ReplaceAllIn(source, 0, source.Length, + out var count); + + Assert.That(count, Is.EqualTo(1)); + Assert.That(actual.Text, Is.EqualTo("new \uFFFC keep")); + var objectRun = actual.get_RunAt(4); + Assert.That(actual.get_RunText(objectRun), Is.EqualTo("\uFFFC")); + Assert.That(actual.get_Properties(objectRun).GetStrPropValue( + (int)FwTextPropType.ktptObjData), Is.EqualTo(objectData)); + } + + [Test] + public void ReplaceAllIn_LeavesTerminalNoMatchState() + { + var pattern = MakePattern("old"); + pattern.ReplaceWith = TsStringUtils.MakeString("new", Ws); + var source = TsStringUtils.MakeString("old old", Ws); + + var actual = ((IVwPattern2)pattern).ReplaceAllIn(source, 0, source.Length, + out var count); + + Assert.That(actual.Text, Is.EqualTo("new new")); + Assert.That(count, Is.EqualTo(2)); + Assert.Throws(() => { var unused = pattern.ReplacementText; }); + } + + [Test] + public void ReusedPatternAfterLocaleChange_ExtendsShiftedPunctuation() + { + var pattern = MakePattern("a", matchCase: true, matchDiacritics: true); + var source = MakeTextSource("a-", out var length); + Find(pattern, source, 0, length, true, out var rootMin, out var rootLim); + AssertFound(rootMin, rootLim, 0, 1, "root collation span"); + + pattern.IcuLocale = "de-u-co-phonebk-ka-shifted"; + Find(pattern, source, 0, length, true, out var shiftedMin, out var shiftedLim); + + AssertFound(shiftedMin, shiftedLim, 0, 2, "shifted collation span"); + } + } +} diff --git a/Src/xWorks/xWorksTests/BulkEditBarTests.cs b/Src/xWorks/xWorksTests/BulkEditBarTests.cs index 389144135d..4b49d3125b 100644 --- a/Src/xWorks/xWorksTests/BulkEditBarTests.cs +++ b/Src/xWorks/xWorksTests/BulkEditBarTests.cs @@ -10,6 +10,7 @@ using System.Windows.Forms; using System.Xml; using NUnit.Framework; +using SIL.LCModel.Core.KernelInterfaces; using SIL.LCModel.Core.Text; using SIL.FieldWorks.Common.ViewsInterfaces; using SIL.FieldWorks.Common.Controls; @@ -616,7 +617,52 @@ protected override void PersistSortSequence() [TestFixture] public class BulkEditBarTests : BulkEditBarTestsBase { + public enum ChangePath { Preview, Direct } + #region BulkEditEntries tests + [TestCase(ChangePath.Preview, true, true, TestName = "FakeDoit_ComputesAndCachesAnEnabledResultOnce")] + [TestCase(ChangePath.Preview, false, true, TestName = "FakeDoit_DisablesRowsThatCannotChange")] + [TestCase(ChangePath.Preview, true, false, TestName = "FakeDoit_DisablesRowsWithoutAValue")] + [TestCase(ChangePath.Direct, true, true, TestName = "Doit_AppliesAnEnabledResultOnce")] + [TestCase(ChangePath.Direct, false, true, TestName = "Doit_LeavesTheDestinationUnchangedWhenDisabled")] + [TestCase(ChangePath.Direct, true, false, TestName = "Doit_LeavesTheDestinationUnchangedWithoutAValue")] + public void Doit_ComputesEachResultOnce(ChangePath path, bool canChange, bool hasValue) + { + var hvo = Cache.LangProject.LexDbOA.Entries.First().Hvo; + var value = hasValue + ? TsStringUtils.MakeString("changed", Cache.DefaultVernWs) + : null; + CountingFieldReadWriter accessor; + var method = CreateCountingDoItMethod(path, canChange, value, out accessor); + var before = accessor.CurrentValue(hvo).Text; + + if (path == ChangePath.Preview) + { + var sentinel = TsStringUtils.MakeString("previous preview", Cache.DefaultVernWs); + m_bv.SpecialCache.SetString(hvo, XMLViewsDataCache.ktagAlternateValue, sentinel); + method.FakeDoit(new[] { hvo }, XMLViewsDataCache.ktagAlternateValue, + XMLViewsDataCache.ktagItemEnabled, new NullProgressState()); + + var expected = canChange && hasValue ? value : sentinel; + Assert.That(m_bv.SpecialCache.get_StringProp(hvo, + XMLViewsDataCache.ktagAlternateValue), Is.SameAs(expected)); + Assert.That(m_bv.SpecialCache.get_IntProp(hvo, + XMLViewsDataCache.ktagItemEnabled), Is.EqualTo(canChange && hasValue ? 1 : 0)); + } + else + { + DoWithUndoTask(() => method.Doit(hvo)); + Assert.That(accessor.SetNewValueCount, + Is.EqualTo(canChange && hasValue ? 1 : 0)); + Assert.That(accessor.CurrentValue(hvo).Text, + Is.EqualTo(canChange && hasValue ? value.Text : before)); + } + + Assert.That(method.TryGetNewValueCount, Is.EqualTo(1)); + Assert.That(method.OkToChangeCount, Is.EqualTo(1)); + Assert.That(method.NewValueCount, Is.EqualTo(canChange ? 1 : 0)); + } + [Test] public void FilterBar_HeaderAndFilterControlsExposeReachableBaseline() { @@ -1621,6 +1667,97 @@ private ILexEntryRef MakeComplexFormLexEntryRef(ILexEntry ownerEntry) } #endregion BulkEditEntries tests + + private CountingDoItMethod CreateCountingDoItMethod(ChangePath path, bool canChange, + ITsString value, out CountingFieldReadWriter accessor) + { + var document = new XmlDocument(); + document.LoadXml(""); + accessor = new CountingFieldReadWriter(FieldReadWriter.Create(document.DocumentElement, Cache)); + var dataAccess = path == ChangePath.Preview + ? m_bv.SpecialCache + : (ISilDataAccessManaged)Cache.DomainDataByFlid; + return new CountingDoItMethod(Cache, dataAccess, + accessor, document.DocumentElement, canChange, value); + } + + private void DoWithUndoTask(Action action) + { + Cache.DomainDataByFlid.BeginUndoTask("test", "test"); + try + { + action(); + } + finally + { + Cache.DomainDataByFlid.EndUndoTask(); + } + } + + private sealed class CountingDoItMethod : DoItMethod + { + private readonly bool m_canChange; + private readonly ITsString m_value; + + internal CountingDoItMethod(LcmCache cache, ISilDataAccessManaged sda, + FieldReadWriter accessor, XmlNode spec, bool canChange, ITsString value) + : base(cache, sda, accessor, spec) + { + m_canChange = canChange; + m_value = value; + } + + internal int TryGetNewValueCount { get; private set; } + internal int OkToChangeCount { get; private set; } + internal int NewValueCount { get; private set; } + + protected override bool TryGetNewValue(int hvo, out ITsString newValue) + { + TryGetNewValueCount++; + return base.TryGetNewValue(hvo, out newValue); + } + + protected override bool OkToChange(int hvo) + { + OkToChangeCount++; + return m_canChange; + } + + protected override ITsString NewValue(int hvo) + { + NewValueCount++; + return m_value; + } + } + + private sealed class CountingFieldReadWriter : FieldReadWriter + { + private readonly FieldReadWriter m_inner; + + internal CountingFieldReadWriter(FieldReadWriter inner) + : base(inner.DataAccess) + { + m_inner = inner; + } + + internal int SetNewValueCount { get; private set; } + + public override ITsString CurrentValue(int hvo) + { + return m_inner.CurrentValue(hvo); + } + + public override void SetNewValue(int hvo, ITsString tss) + { + SetNewValueCount++; + m_inner.SetNewValue(hvo, tss); + } + + public override int WritingSystem + { + get { return m_inner.WritingSystem; } + } + } } /// From 1fc930e5586f9da13bccf00fee9b1c6c21fd41cd Mon Sep 17 00:00:00 2001 From: John Lambert Date: Fri, 14 Aug 2026 03:06:04 -0400 Subject: [PATCH 2/5] Fix bulk-edit undo-task safety and close review gaps found in preflight Guard the outer bulk-edit undo task with try/finally so a native ReplaceAllIn fault mid-batch cannot leave it unterminated. Share one computed value between OkToChange and TryGetNewValue in BulkCopyMethod and TransduceMethod instead of computing twice, scoped to stay correct across separate preview/apply calls. Add native ReplaceAllIn coverage for ORC, writing-system restriction, case/diacritics, and canonical equivalence. Harden the NFD-normalization skip check with non-Latin script and rich-run test cases. Co-Authored-By: Claude Sonnet 5 --- Src/Common/Controls/XMLViews/BulkEditBar.cs | 66 +++- Src/views/Test/TestVwPattern.h | 306 +++++++++++++++++- .../Performance/ReplaceWithMethodTests.cs | 65 ++++ Src/xWorks/xWorksTests/BulkEditBarTests.cs | 72 +++++ 4 files changed, 490 insertions(+), 19 deletions(-) diff --git a/Src/Common/Controls/XMLViews/BulkEditBar.cs b/Src/Common/Controls/XMLViews/BulkEditBar.cs index 356370c26f..3e1167e357 100644 --- a/Src/Common/Controls/XMLViews/BulkEditBar.cs +++ b/Src/Common/Controls/XMLViews/BulkEditBar.cs @@ -4838,22 +4838,28 @@ public void FakeDoit(IEnumerable itemsToChange, int tagFakeFlid, int tagEna public void Doit(IEnumerable itemsToChange, ProgressState state) { m_sda.BeginUndoTask(XMLViewsStrings.ksUndoBulkEdit, XMLViewsStrings.ksRedoBulkEdit); - string commitChanges = XmlUtils.GetOptionalAttributeValue(m_nodeSpec, "commitChanges"); - int i = 0; - // Report progress 50 times or every 100 items, whichever is more (but no more than once per item!) - int interval = Math.Min(100, Math.Max(itemsToChange.Count() / 50, 1)); - foreach (int hvo in itemsToChange) + try { - i++; - if (i % interval == 0) + string commitChanges = XmlUtils.GetOptionalAttributeValue(m_nodeSpec, "commitChanges"); + int i = 0; + // Report progress 50 times or every 100 items, whichever is more (but no more than once per item!) + int interval = Math.Min(100, Math.Max(itemsToChange.Count() / 50, 1)); + foreach (int hvo in itemsToChange) { - state.PercentDone = i * 100 / itemsToChange.Count(); - state.Breath(); + i++; + if (i % interval == 0) + { + state.PercentDone = i * 100 / itemsToChange.Count(); + state.Breath(); + } + Doit(hvo); + BulkEditBar.CommitChanges(hvo, commitChanges, m_cache, m_accessor.WritingSystem); } - Doit(hvo); - BulkEditBar.CommitChanges(hvo, commitChanges, m_cache, m_accessor.WritingSystem); } - m_sda.EndUndoTask(); + finally + { + m_sda.EndUndoTask(); + } } /// @@ -4910,11 +4916,41 @@ protected virtual bool TryGetNewValue(int hvo, out ITsString newValue) { newValue = null; if (!OkToChange(hvo)) + { + ClearNewValueCache(); return false; - newValue = NewValue(hvo); + } + newValue = NewValueCached(hvo); + ClearNewValueCache(); return newValue != null; } + private int? m_hvoCachedNewValue; + private ITsString m_cachedNewValue; + + /// + /// Returns NewValue(hvo), reusing a value an OkToChange override already computed + /// for the same hvo within this same TryGetNewValue call (to decide whether a change + /// would occur) instead of recomputing it. The cache is cleared at the end of every + /// TryGetNewValue call: a later call for the same hvo (e.g. preview, then apply) must + /// still recompute, since the underlying data or pattern state may have changed. + /// + protected ITsString NewValueCached(int hvo) + { + if (m_hvoCachedNewValue != hvo) + { + m_cachedNewValue = NewValue(hvo); + m_hvoCachedNewValue = hvo; + } + return m_cachedNewValue; + } + + private void ClearNewValueCache() + { + m_hvoCachedNewValue = null; + m_cachedNewValue = null; + } + protected abstract ITsString NewValue(int hvo); #region IGetReplacedObjects Members @@ -4961,7 +4997,7 @@ protected override bool OkToChange(int hvo) string sOld = null; if (tssOld != null) sOld = tssOld.Text; - ITsString tssNew = NewValue(hvo); + ITsString tssNew = NewValueCached(hvo); string sNew = null; if (tssNew != null) sNew = tssNew.Text; @@ -5032,7 +5068,7 @@ protected override bool OkToChange(int hvo) return false; } ITsString tssSrc = m_srcAccessor.CurrentValue(hvo); - return tssSrc != null && !m_srcAccessor.CurrentValue(hvo).Equals(NewValue(hvo)); + return tssSrc != null && !m_srcAccessor.CurrentValue(hvo).Equals(NewValueCached(hvo)); } diff --git a/Src/views/Test/TestVwPattern.h b/Src/views/Test/TestVwPattern.h index f19d3a442d..c5c4f6d1ec 100644 --- a/Src/views/Test/TestVwPattern.h +++ b/Src/views/Test/TestVwPattern.h @@ -732,19 +732,19 @@ namespace TestViews int ichMin, ichLim; CheckHr(m_qpat->put_MatchDiacritics(false)); // This is set true by default. CheckHr(m_qpat->FindIn(m_qts, 0, stuSearch.Length(), true, &ichMin, &ichLim, NULL)); - unitpp::assert_eq("Found 'nd' at start (ignoring diacritics)", 0, ichMin); - unitpp::assert_eq("End of 'nd' at start (ignoring diacritics)", 3, ichLim); + unitpp::assert_eq("Found '�nd' at start (ignoring diacritics)", 0, ichMin); + unitpp::assert_eq("End of '�nd' at start (ignoring diacritics)", 3, ichLim); // ...but not match requiring diacritics to match CheckHr(m_qpat->put_MatchDiacritics(true)); CheckHr(m_qpat->FindIn(m_qts, 0, stuSearch.Length(), true, &ichMin, &ichLim, NULL)); - unitpp::assert_eq("Skipped 'nd' at start (match diacritics)", kichoffsetofAnd, ichMin); + unitpp::assert_eq("Skipped '�nd' at start (match diacritics)", kichoffsetofAnd, ichMin); // ...still not if we have to match case CheckHr(m_qpat->put_MatchDiacritics(false)); CheckHr(m_qpat->put_MatchCase(true)); CheckHr(m_qpat->FindIn(m_qts, 0, stuSearch.Length(), true, &ichMin, &ichLim, NULL)); - unitpp::assert_eq("Skipped 'nd' at start, match case", kichoffsetofAnd, ichMin); + unitpp::assert_eq("Skipped '�nd' at start, match case", kichoffsetofAnd, ichMin); } /*-------------------------------------------------------------------------------------- @@ -1934,6 +1934,304 @@ namespace TestViews unitpp::assert_true("result remains null after replacement failure", ptssResult == NULL); } + + // Re-run the TE4727 regression (replacing the character immediately preceding a + // trailing owned ORC) through the bulk ReplaceAllIn path instead of FindIn, and + // confirm the ORC and its footnote guid data survive the splice. + void testReplaceAllInReplaceCharPrecedingFinalORC_TE4727() + { + unitpp::assert_true("English writing system exists", m_qwsEng.Ptr()); + + ITsStrBldrPtr qtsbStringBuilder; + qtsbStringBuilder.CreateInstance(CLSID_TsStrBldr); + ITsPropsBldrPtr qtpbTextPropsBuilder; + qtpbTextPropsBuilder.CreateInstance(CLSID_TsPropsBldr); + CheckHr(qtpbTextPropsBuilder->SetIntPropValues(ktptWs, ktpvDefault, g_wsEng)); + ITsTextPropsPtr qttp; + CheckHr(qtpbTextPropsBuilder->GetTextProps(&qttp)); + StrUni stuSearch(L"Tha Tant of tha Lord's Prasance"); + CheckHr(qtsbStringBuilder->Replace(0, 0, stuSearch.Bstr(), qttp)); + + // Insert a footnote ORC as the very last character of the string. + StrUni stuData; + OLECHAR * prgchData; + GUID uidFootnote; + CheckHr(CoCreateGuid(&uidFootnote)); + stuData.SetSize(isizeof(GUID) / isizeof(OLECHAR) + 1, &prgchData); + *prgchData = kodtOwnNameGuidHot; + memmove(prgchData + 1, &uidFootnote, isizeof(uidFootnote)); + CheckHr(qtpbTextPropsBuilder->SetStrPropValue(ktptObjData, stuData.Bstr())); + CheckHr(qtpbTextPropsBuilder->GetTextProps(&qttp)); + OLECHAR chObj = kchObject; + CheckHr(qtsbStringBuilder->ReplaceRgch(stuSearch.Length(), stuSearch.Length(), + &chObj, 1, qttp)); + ITsStringPtr qtssSource; + CheckHr(qtsbStringBuilder->GetString(&qtssSource)); + + ITsStringPtr qtssPattern; + StrUni stuPattern(L"e"); + CheckHr(m_qtsf->MakeString(stuPattern.Bstr(), g_wsEng, &qtssPattern)); + CheckHr(m_qpat->putref_Pattern(qtssPattern)); + CheckHr(m_qpat->put_UseRegularExpressions(false)); + + ITsStringPtr qtssReplacement; + StrUni stuReplacement(L"E"); + CheckHr(m_qtsf->MakeString(stuReplacement.Bstr(), g_wsEng, &qtssReplacement)); + CheckHr(m_qpat->putref_ReplaceWith(qtssReplacement)); + + int cchTotal; + CheckHr(qtssSource->get_Length(&cchTotal)); + + int cExpected; + ITsStringPtr qtssExpected = ReplaceWithRepeatedFindIn(qtssSource, 0, cchTotal, + &cExpected); + + int cMatches = -1; + ITsStringPtr qtssResult; + CheckHr(m_qpat2->ReplaceAllIn(qtssSource, 0, cchTotal, &cMatches, &qtssResult)); + unitpp::assert_eq("single 'e' preceding final ORC found", 1, cMatches); + unitpp::assert_eq("match count matches repeated FindIn", cExpected, cMatches); + ComBool fEqual; + CheckHr(qtssResult->Equals(qtssExpected, &fEqual)); + unitpp::assert_true("bulk result equals repeated FindIn result", fEqual); + + // Confirm the ORC after the replaced character survived intact, with its + // footnote guid data untouched. + int crun; + CheckHr(qtssResult->get_RunCount(&crun)); + SmartBstr sbstrLastRun; + CheckHr(qtssResult->get_RunText(crun - 1, &sbstrLastRun)); + unitpp::assert_eq("last run is the ORC", kchObject, sbstrLastRun[0]); + ITsTextPropsPtr qttpLastRun; + CheckHr(qtssResult->get_Properties(crun - 1, &qttpLastRun)); + SmartBstr sbstrObjData; + CheckHr(qttpLastRun->GetStrPropValue(ktptObjData, &sbstrObjData)); + unitpp::assert_true("trailing ORC's footnote guid data preserved", + !wcscmp(stuData.Chars(), sbstrObjData.Chars())); + } + + // Re-run the canonical/writing-system-restricted matching combinations from + // testMatchingWs through ReplaceAllIn, confirming bulk match counts and results + // agree with repeated FindIn as MatchOldWritingSystem and MatchDiacritics change. + void testReplaceAllInRespectsMatchOldWritingSystem() + { + unitpp::assert_true("English writing system exists", m_qwsEng.Ptr()); + + ITsStringPtr qtssSearchT; + StrUni stuSearch(L"abc" A_WITH_DIAERESIS COMBINING_DOT_BELOW L"abcA" COMBINING_DOT_BELOW + COMBINING_DIAERESIS L"rubbish"); + CheckHr(m_qtsf->MakeString(stuSearch.Bstr(), g_wsEng, &qtssSearchT)); + ITsStringPtr qtssSearch; + CheckHr(qtssSearchT->get_NormalizedForm(knmNFD, &qtssSearch)); + ITsStrBldrPtr qtsb; + CheckHr(qtssSearch->GetBldr(&qtsb)); + // Make the first A with diacritics and the later A and combining dot french. + CheckHr(qtsb->SetIntPropValues(3, 6, ktptWs, ktpvDefault, g_wsFrn)); + CheckHr(qtsb->SetIntPropValues(9, 11, ktptWs, ktpvDefault, g_wsFrn)); + CheckHr(qtsb->GetString(&qtssSearch)); + + ITsStringPtr qtssPattern; + StrUni stuPattern(L"cA" COMBINING_DOT_BELOW COMBINING_DIAERESIS); + CheckHr(m_qtsf->MakeString(stuPattern.Bstr(), g_wsEng, &qtssPattern)); + CheckHr(qtssPattern->GetBldr(&qtsb)); + // Make the first A with diacritics and the later combining macron french. + CheckHr(qtsb->SetIntPropValues(1, 4, ktptWs, ktpvDefault, g_wsFrn)); + CheckHr(qtsb->GetString(&qtssPattern)); + CheckHr(m_qpat->putref_Pattern(qtssPattern)); + + ITsStringPtr qtssReplacement; + StrUni stuReplacement(L"X"); + CheckHr(m_qtsf->MakeString(stuReplacement.Bstr(), g_wsEng, &qtssReplacement)); + CheckHr(m_qpat->putref_ReplaceWith(qtssReplacement)); + + int len = stuSearch.Length(); + ComBool fEqual; + + // Default: MatchOldWritingSystem is off, so both canonical occurrences match. + int cExpected; + ITsStringPtr qtssExpected = ReplaceWithRepeatedFindIn(qtssSearch, 0, len, + &cExpected); + int cMatches = -1; + ITsStringPtr qtssResult; + CheckHr(m_qpat2->ReplaceAllIn(qtssSearch, 0, len, &cMatches, &qtssResult)); + unitpp::assert_eq("both canonical matches replaced without ws matching", 2, + cMatches); + unitpp::assert_eq("count matches repeated FindIn (no ws)", cExpected, cMatches); + CheckHr(qtssResult->Equals(qtssExpected, &fEqual)); + unitpp::assert_true("bulk result equals repeated FindIn (no ws)", fEqual); + + // Requiring old ws but ignoring diacritics: still both occurrences match. + CheckHr(m_qpat->put_MatchDiacritics(false)); + CheckHr(m_qpat->put_MatchOldWritingSystem(true)); + qtssExpected = ReplaceWithRepeatedFindIn(qtssSearch, 0, len, &cExpected); + cMatches = -1; + qtssResult.Clear(); + CheckHr(m_qpat2->ReplaceAllIn(qtssSearch, 0, len, &cMatches, &qtssResult)); + unitpp::assert_eq("both canonical matches replaced, ws required, diacritics ignored", + 2, cMatches); + unitpp::assert_eq("count matches repeated FindIn (ws, no diacritics)", cExpected, + cMatches); + CheckHr(qtssResult->Equals(qtssExpected, &fEqual)); + unitpp::assert_true("bulk result equals repeated FindIn (ws, no diacritics)", + fEqual); + + // Requiring both old ws and diacritics narrows the match to the first occurrence. + CheckHr(m_qpat->put_MatchDiacritics(true)); + qtssExpected = ReplaceWithRepeatedFindIn(qtssSearch, 0, len, &cExpected); + cMatches = -1; + qtssResult.Clear(); + CheckHr(m_qpat2->ReplaceAllIn(qtssSearch, 0, len, &cMatches, &qtssResult)); + unitpp::assert_eq("only first occurrence matches ws+diacritics", 1, cMatches); + unitpp::assert_eq("count matches repeated FindIn (ws + diacritics)", cExpected, + cMatches); + CheckHr(qtssResult->Equals(qtssExpected, &fEqual)); + unitpp::assert_true("bulk result equals repeated FindIn (ws + diacritics)", fEqual); + } + + // Re-run the MatchCase / MatchDiacritics option combinations from testMatchingCase + // and testMatchingDiacritics through ReplaceAllIn. + void testReplaceAllInWithCaseAndDiacriticsOptions() + { + unitpp::assert_true("English writing system exists", m_qwsEng.Ptr()); + ComBool fEqual; + + // --- Case sensitivity --- + ITsStringPtr qtssSearchCase; + StrUni stuSearchCase(L"And this sentence uses 'and' a lot"); + CheckHr(m_qtsf->MakeString(stuSearchCase.Bstr(), g_wsEng, &qtssSearchCase)); + + ITsStringPtr qtssPattern; + StrUni stuPattern(L"and"); + CheckHr(m_qtsf->MakeString(stuPattern.Bstr(), g_wsEng, &qtssPattern)); + CheckHr(m_qpat->putref_Pattern(qtssPattern)); + + ITsStringPtr qtssReplacement; + StrUni stuReplacement(L"or"); + CheckHr(m_qtsf->MakeString(stuReplacement.Bstr(), g_wsEng, &qtssReplacement)); + CheckHr(m_qpat->putref_ReplaceWith(qtssReplacement)); + + int lenCase = stuSearchCase.Length(); + CheckHr(m_qpat->put_MatchDiacritics(true)); + CheckHr(m_qpat->put_MatchCase(false)); + + int cExpected; + ITsStringPtr qtssExpected = ReplaceWithRepeatedFindIn(qtssSearchCase, 0, lenCase, + &cExpected); + int cMatches = -1; + ITsStringPtr qtssResult; + CheckHr(m_qpat2->ReplaceAllIn(qtssSearchCase, 0, lenCase, &cMatches, &qtssResult)); + unitpp::assert_eq("case-insensitive replaces both 'And' and 'and'", 2, cMatches); + unitpp::assert_eq("case-insensitive count matches repeated FindIn", cExpected, + cMatches); + CheckHr(qtssResult->Equals(qtssExpected, &fEqual)); + unitpp::assert_true("case-insensitive bulk result equals repeated FindIn", fEqual); + + CheckHr(m_qpat->put_MatchCase(true)); + qtssExpected = ReplaceWithRepeatedFindIn(qtssSearchCase, 0, lenCase, &cExpected); + cMatches = -1; + qtssResult.Clear(); + CheckHr(m_qpat2->ReplaceAllIn(qtssSearchCase, 0, lenCase, &cMatches, &qtssResult)); + unitpp::assert_eq("match case skips capitalized 'And'", 1, cMatches); + unitpp::assert_eq("match-case count matches repeated FindIn", cExpected, cMatches); + CheckHr(qtssResult->Equals(qtssExpected, &fEqual)); + unitpp::assert_true("match-case bulk result equals repeated FindIn", fEqual); + SmartBstr sbstrResult; + CheckHr(qtssResult->get_Text(&sbstrResult)); + unitpp::assert_true("only the lowercase 'and' was replaced", + sbstrResult == L"And this sentence uses 'or' a lot"); + + // --- Diacritics sensitivity --- + ITsStringPtr qtssSearchDia; + StrUni stuSearchDia(LATIN_CAPITAL_A_WITH_DIARESIS L"nd this sentence uses 'and' a lot"); + CheckHr(m_qtsf->MakeString(stuSearchDia.Bstr(), g_wsEng, &qtssSearchDia)); + CheckHr(m_qpat->putref_Pattern(qtssPattern)); + CheckHr(m_qpat->putref_ReplaceWith(qtssReplacement)); + CheckHr(m_qpat->put_MatchCase(false)); + CheckHr(m_qpat->put_MatchDiacritics(false)); + + int lenDia = stuSearchDia.Length(); + qtssExpected = ReplaceWithRepeatedFindIn(qtssSearchDia, 0, lenDia, &cExpected); + cMatches = -1; + qtssResult.Clear(); + CheckHr(m_qpat2->ReplaceAllIn(qtssSearchDia, 0, lenDia, &cMatches, &qtssResult)); + unitpp::assert_eq("ignoring diacritics replaces both occurrences", 2, cMatches); + unitpp::assert_eq("ignore-diacritics count matches repeated FindIn", cExpected, + cMatches); + CheckHr(qtssResult->Equals(qtssExpected, &fEqual)); + unitpp::assert_true("ignore-diacritics bulk result equals repeated FindIn", fEqual); + + CheckHr(m_qpat->put_MatchDiacritics(true)); + qtssExpected = ReplaceWithRepeatedFindIn(qtssSearchDia, 0, lenDia, &cExpected); + cMatches = -1; + qtssResult.Clear(); + CheckHr(m_qpat2->ReplaceAllIn(qtssSearchDia, 0, lenDia, &cMatches, &qtssResult)); + unitpp::assert_eq("requiring diacritics skips the accented occurrence", 1, + cMatches); + unitpp::assert_eq("match-diacritics count matches repeated FindIn", cExpected, + cMatches); + CheckHr(qtssResult->Equals(qtssExpected, &fEqual)); + unitpp::assert_true("match-diacritics bulk result equals repeated FindIn", fEqual); + } + + // Re-run testSearchCanonical's NFD canonical-equivalence scenario through + // ReplaceAllIn: the pattern is correctly-ordered decomposed text, while the source + // contains both a fully-composed and an out-of-order-decomposed occurrence. + void testReplaceAllInCanonicalEquivalence() + { + unitpp::assert_true("English writing system exists", m_qwsEng.Ptr()); + + ITsStringPtr qtssSearchT; + StrUni stuSearch(L"abc" A_WITH_DIAERESIS COMBINING_DOT_BELOW L"abcA" COMBINING_DOT_BELOW + COMBINING_DIAERESIS L"rubbish"); + CheckHr(m_qtsf->MakeString(stuSearch.Bstr(), g_wsEng, &qtssSearchT)); + ITsStringPtr qtssSearch; + CheckHr(qtssSearchT->get_NormalizedForm(knmNFD, &qtssSearch)); + + ITsStringPtr qtssPattern; + StrUni stuPattern(L"cA" COMBINING_DOT_BELOW COMBINING_DIAERESIS); + CheckHr(m_qtsf->MakeString(stuPattern.Bstr(), g_wsEng, &qtssPattern)); + CheckHr(m_qpat->putref_Pattern(qtssPattern)); + + ITsStringPtr qtssReplacement; + StrUni stuReplacement(L"XYZ"); + CheckHr(m_qtsf->MakeString(stuReplacement.Bstr(), g_wsEng, &qtssReplacement)); + CheckHr(m_qpat->putref_ReplaceWith(qtssReplacement)); + + int len = stuSearch.Length(); + ComBool fEqual; + + int cExpected; + ITsStringPtr qtssExpected = ReplaceWithRepeatedFindIn(qtssSearch, 0, len, + &cExpected); + int cMatches = -1; + ITsStringPtr qtssResult; + CheckHr(m_qpat2->ReplaceAllIn(qtssSearch, 0, len, &cMatches, &qtssResult)); + unitpp::assert_eq("both canonically-equivalent occurrences replaced", 2, cMatches); + unitpp::assert_eq("canonical count matches repeated FindIn", cExpected, cMatches); + CheckHr(qtssResult->Equals(qtssExpected, &fEqual)); + unitpp::assert_true("canonical bulk result equals repeated FindIn", fEqual); + SmartBstr sbstrResult; + CheckHr(qtssResult->get_Text(&sbstrResult)); + unitpp::assert_true("both occurrences replaced with XYZ", + sbstrResult == L"abXYZabXYZrubbish"); + + // Same result even requiring case and diacritics to match: the canonical + // equivalence logic still finds both. + CheckHr(m_qpat->put_MatchDiacritics(true)); + CheckHr(m_qpat->put_MatchCase(true)); + qtssExpected = ReplaceWithRepeatedFindIn(qtssSearch, 0, len, &cExpected); + cMatches = -1; + qtssResult.Clear(); + CheckHr(m_qpat2->ReplaceAllIn(qtssSearch, 0, len, &cMatches, &qtssResult)); + unitpp::assert_eq("both occurrences still replaced, match case/diacritics", 2, + cMatches); + unitpp::assert_eq("canonical count matches repeated FindIn, match case/diacritics", + cExpected, cMatches); + CheckHr(qtssResult->Equals(qtssExpected, &fEqual)); + unitpp::assert_true( + "canonical bulk result equals repeated FindIn, match case/diacritics", fEqual); + } + virtual void Setup() { CreateTestWritingSystemFactory(); diff --git a/Src/xWorks/xWorksTests/Avalonia/Performance/ReplaceWithMethodTests.cs b/Src/xWorks/xWorksTests/Avalonia/Performance/ReplaceWithMethodTests.cs index 8765dd73ad..81bd814a41 100644 --- a/Src/xWorks/xWorksTests/Avalonia/Performance/ReplaceWithMethodTests.cs +++ b/Src/xWorks/xWorksTests/Avalonia/Performance/ReplaceWithMethodTests.cs @@ -266,6 +266,71 @@ public void FakeDoit_NormalizesBulkReplacementResultToNfd() Assert.That(preview.Text.IsNormalized(NormalizationForm.FormD), Is.True); } + [Test] + public void FakeDoit_NormalizesNonLatinScriptReplacementResultToNfd() + { + // U+AC00 (Hangul syllable "GA") has a real canonical decomposition to two + // Jamo characters under NFD, unlike the single-diacritic Latin case above. + // This hardens the NormalizeResult skip-check for a non-Latin script. + var entry = CreateEntries(1, 1, "old \uac00", "old \uac00").Single(); + var document = BuildColumnSpec(); + var accessor = FieldReadWriter.Create(document.DocumentElement, Cache); + var pattern = BuildPattern("old", "new \uac00"); + var method = new CountingReplaceWithMethod(Cache, m_bv.SpecialCache, accessor, + document.DocumentElement, pattern, pattern.ReplaceWith); + + method.FakeDoit(new[] { entry.Hvo }, XMLViewsDataCache.ktagAlternateValue, + XMLViewsDataCache.ktagItemEnabled, new NullProgressState()); + + var preview = m_bv.SpecialCache.get_StringProp(entry.Hvo, + XMLViewsDataCache.ktagAlternateValue); + Assert.That(method.ReplaceAllInCount, Is.EqualTo(1)); + Assert.That(preview.Text, Is.EqualTo("new \uac00 \uac00".Normalize(NormalizationForm.FormD))); + Assert.That(preview.Text.IsNormalized(NormalizationForm.FormD), Is.True); + } + + [Test] + public void FakeDoit_PreservesRichRunPropertiesWhenNormalizingNonLatinReplacementResult() + { + ILexEntry entry = null; + ILexSense sense = null; + NonUndoableUnitOfWorkHelper.Do(Cache.ActionHandlerAccessor, () => + { + entry = Cache.ServiceLocator.GetInstance().Create(); + sense = Cache.ServiceLocator.GetInstance().Create(entry, null, string.Empty); + m_createdObjectList.Add(entry); + }); + var document = BuildColumnSpec(); + var accessor = new OwnMlPropReadWriter(Cache, LexSenseTags.kflidDefinition, Cache.DefaultAnalWs); + // U+AC01 ("GAG") canonically decomposes to THREE Jamo characters under NFD, + // so this run must grow during normalization while keeping its style intact. + var sourceBuilder = TsStringUtils.MakeString("old \uac01", Cache.DefaultAnalWs).GetBldr(); + sourceBuilder.SetIntPropValues(4, 5, (int)FwTextPropType.ktptWs, + (int)FwTextPropVar.ktpvDefault, Cache.DefaultVernWs); + sourceBuilder.SetStrPropValue(4, 5, (int)FwTextPropType.ktptNamedStyle, "Emphasis"); + var source = sourceBuilder.GetString(); + NonUndoableUnitOfWorkHelper.Do(Cache.ActionHandlerAccessor, + () => accessor.SetNewValue(sense.Hvo, source)); + Assert.That(accessor.CurrentValue(sense.Hvo).RunCount, Is.EqualTo(2)); + + // Replacement text is itself a precomposed Hangul syllable, so the result + // needs real NFD decomposition work, not the already-normalized skip path. + var pattern = BuildPattern("old", "new \uac00"); + var method = new CountingReplaceWithMethod(Cache, m_bv.SpecialCache, accessor, + document.DocumentElement, pattern, pattern.ReplaceWith); + method.FakeDoit(new[] { sense.Hvo }, XMLViewsDataCache.ktagAlternateValue, + XMLViewsDataCache.ktagItemEnabled, new NullProgressState()); + var preview = m_bv.SpecialCache.get_StringProp(sense.Hvo, XMLViewsDataCache.ktagAlternateValue); + + Assert.That(method.ReplaceAllInCount, Is.EqualTo(1)); + Assert.That(preview.Text, Is.EqualTo("new \uac00 \uac01".Normalize(NormalizationForm.FormD))); + Assert.That(preview.Text.IsNormalized(NormalizationForm.FormD), Is.True); + + var styledRun = Enumerable.Range(0, preview.RunCount) + .Single(i => preview.get_Properties(i).GetStrPropValue((int)FwTextPropType.ktptNamedStyle) == "Emphasis"); + Assert.That(TsStringUtils.GetWsOfRun(preview, styledRun), Is.EqualTo(Cache.DefaultVernWs)); + } + [Test] public void FakeDoit_BaseGateRejectionDoesNotSearchOrChangePreview() { diff --git a/Src/xWorks/xWorksTests/BulkEditBarTests.cs b/Src/xWorks/xWorksTests/BulkEditBarTests.cs index 4b49d3125b..a9bd390927 100644 --- a/Src/xWorks/xWorksTests/BulkEditBarTests.cs +++ b/Src/xWorks/xWorksTests/BulkEditBarTests.cs @@ -663,6 +663,39 @@ public void Doit_ComputesEachResultOnce(ChangePath path, bool canChange, bool ha Assert.That(method.NewValueCount, Is.EqualTo(canChange ? 1 : 0)); } + [Test] + public void Doit_OuterLoop_EndsUndoTaskWhenAnItemThrows() + { + var hvo = Cache.LangProject.LexDbOA.Entries.First().Hvo; + var document = new XmlDocument(); + document.LoadXml(""); + var accessor = FieldReadWriter.Create(document.DocumentElement, Cache); + var method = new ThrowingDoItMethod(Cache, (ISilDataAccessManaged)Cache.DomainDataByFlid, + accessor, document.DocumentElement); + + Assert.That(Cache.ActionHandlerAccessor.CurrentDepth, Is.EqualTo(0)); + + Assert.Throws(() => + method.Doit(new[] { hvo }, new NullProgressState())); + + Assert.That(Cache.ActionHandlerAccessor.CurrentDepth, Is.EqualTo(0), + "the outer bulk-edit undo task must be ended even when applying one item throws"); + } + + private sealed class ThrowingDoItMethod : DoItMethod + { + internal ThrowingDoItMethod(LcmCache cache, ISilDataAccessManaged sda, + FieldReadWriter accessor, XmlNode spec) + : base(cache, sda, accessor, spec) + { + } + + protected override ITsString NewValue(int hvo) + { + throw new InvalidOperationException("simulated failure applying one bulk-edit item"); + } + } + [Test] public void FilterBar_HeaderAndFilterControlsExposeReachableBaseline() { @@ -1730,6 +1763,37 @@ protected override ITsString NewValue(int hvo) } } + [Test] + public void BulkCopy_ComputesSourceValueOnce() + { + var hvo = Cache.LangProject.LexDbOA.Entries.First().Hvo; + + var srcDoc = new XmlDocument(); + srcDoc.LoadXml(""); + var srcAccessor = new CountingFieldReadWriter(FieldReadWriter.Create(srcDoc.DocumentElement, Cache)); + + var dstDoc = new XmlDocument(); + dstDoc.LoadXml(""); + var dstAccessor = FieldReadWriter.Create(dstDoc.DocumentElement, Cache); + + DoWithUndoTask(() => + { + srcAccessor.SetNewValue(hvo, TsStringUtils.MakeString("source note", Cache.DefaultAnalWs)); + dstAccessor.SetNewValue(hvo, TsStringUtils.MakeString("existing citation form", Cache.DefaultVernWs)); + }); + srcAccessor.ResetCounts(); + + var method = new BulkCopyMethod(Cache, m_bv.SpecialCache, dstAccessor, dstDoc.DocumentElement, + srcAccessor, null, NonEmptyTargetOptions.Overwrite); + + method.FakeDoit(new[] { hvo }, XMLViewsDataCache.ktagAlternateValue, + XMLViewsDataCache.ktagItemEnabled, new NullProgressState()); + + Assert.That(m_bv.SpecialCache.get_IntProp(hvo, XMLViewsDataCache.ktagItemEnabled), Is.EqualTo(1)); + Assert.That(srcAccessor.CurrentValueCount, Is.EqualTo(1), + "OkToChange and NewValue must share one computed source value per item instead of reading it twice"); + } + private sealed class CountingFieldReadWriter : FieldReadWriter { private readonly FieldReadWriter m_inner; @@ -1741,9 +1805,17 @@ internal CountingFieldReadWriter(FieldReadWriter inner) } internal int SetNewValueCount { get; private set; } + internal int CurrentValueCount { get; private set; } + + internal void ResetCounts() + { + SetNewValueCount = 0; + CurrentValueCount = 0; + } public override ITsString CurrentValue(int hvo) { + CurrentValueCount++; return m_inner.CurrentValue(hvo); } From d3d8bb45c64100968c6fc7e5e476f7fd1f095758 Mon Sep 17 00:00:00 2001 From: John Lambert Date: Fri, 14 Aug 2026 04:06:21 -0400 Subject: [PATCH 3/5] Remove working perf-measurement log; move reasoning to PR body Docs/superpowers/plans/evidence/bulk-replacement-distillation-2026-08-13.md was a one-time investigation log, not durable guidance. Its measurements, rejected approaches, and follow-up items now live in the PR body. Co-Authored-By: Claude Sonnet 5 --- ...ulk-replacement-distillation-2026-08-13.md | 111 ------------------ 1 file changed, 111 deletions(-) delete mode 100644 Docs/superpowers/plans/evidence/bulk-replacement-distillation-2026-08-13.md diff --git a/Docs/superpowers/plans/evidence/bulk-replacement-distillation-2026-08-13.md b/Docs/superpowers/plans/evidence/bulk-replacement-distillation-2026-08-13.md deleted file mode 100644 index adf61765e6..0000000000 --- a/Docs/superpowers/plans/evidence/bulk-replacement-distillation-2026-08-13.md +++ /dev/null @@ -1,111 +0,0 @@ -# Bulk replacement optimization evidence - -## Retained scope - -The focused implementation retains four changes: - -- Avoid normalization work when a replacement result is already NFD. -- Compute each preview value once and reuse it for the enabled result. -- Expose one-session rich-string replacement through the optional `IVwPattern2` - capability without changing the `IVwPattern` ABI or coclass default. -- Use the capability when available and preserve the repeated-`FindIn` fallback. - -The correctness corpus covers literal and regular-expression replacement, -captures, zero-width matches, whole-word and case/diacritic options, Unicode -normalization and scripts, ICU tailoring, rich-text properties, object -replacement characters, capability fallback, failures, and terminal state. - -## Single-pass preview measurement - -This is an incremental comparison of the single-pass preview change, not a -branch-point-to-final result. Both builds used Debug x64, 50,000 real entries, -100 percent matching CitationForm values with one match each, one warmup, and -five timed preview repetitions per process. Process order was B-C-B-C. - -- Baseline process 1: 73.973706, 87.429190, 85.850784, 89.671968, - 93.842072 microseconds/entry; median 87.429190. -- Candidate process 1: 53.317292, 49.020970, 49.454658, 47.650816, - 54.720194; median 49.454658. -- Baseline process 2: 68.099794, 68.588244, 70.913828, 70.782702, - 69.719412; median 69.719412. -- Candidate process 2: 48.884480, 49.327828, 58.699344, 78.705420, - 90.564270; median 58.699344. -- Pooled medians: baseline 72.443767, candidate 51.385975 - microseconds/entry; 29.068 percent reduction and 1.409796x throughput. - -The second candidate process was retried unchanged after a transient test -discovery failure produced no timing result. - -## ReplaceAllIn measurement - -This is an incremental comparison of `ReplaceAllIn`, not a cumulative branch -result. Both trees used `9307443e494d23066d9d40bc41c4d4b30944b972`. -The baseline changed only the managed capability assignment from the -`IVwPattern2` cast to `null`; native and search code was identical. Both used -`build.ps1 -BuildTests -Serial`, Debug x64, and the same temporary harness -(SHA-256 `52AB194E89486BFB94722A444B077B894B84128B46F8F0CF7EDD62DAE48D5481`). - -Each fresh process used 50,000 real entries. Matching values were -`old old old old-######` (22 UTF-16 units and four matches); unmatched values -were `keep keep keep-######` (21 units). Each fresh process had one warmup and -five timed apply repetitions. Reset, garbage collection, validation, and undo -were outside the stopwatch. There was no cache prepass. All comparisons -reported zero mismatches. - -### 25 percent matching - -- Baseline: 109.070736, 108.331108, 108.476428, 109.597136, 109.750080; - median 109.070736 microseconds/entry. -- Candidate: 80.400070, 86.023792, 81.024132, 83.734014, 100.501314; - median 83.734014. -- Incremental result: 23.23 percent reduction and 1.303x throughput. - -### 50 percent matching - -- Baseline process 1: 235.016240, 278.735516, 215.068208, 291.738206, - 245.199594; median 245.199594. -- Candidate process 1: 160.970294, 161.559868, 161.132232, 161.602208, - 178.361492; median 161.559868. -- Baseline process 2: 220.388066, 222.015458, 215.116946, 233.213528, - 239.261800; median 222.015458. -- Candidate process 2: 172.943354, 173.446744, 170.288498, 194.350902, - 182.141472; median 173.446744. -- Pooled medians: baseline 234.114884, candidate 171.615926; 26.69 percent - reduction and 1.364x throughput. The two paired reductions were 34.11 and - 21.88 percent. - -### 100 percent matching - -- Baseline process 1: 468.296628, 437.760106, 437.642286, 586.596700, - 518.897366; median 468.296628. -- Candidate process 1: 316.618992, 314.000950, 337.447102, 317.614622, - 315.852194; median 316.618992. -- Baseline process 2: 421.689880, 425.018894, 428.718602, 444.715500, - 429.318474; median 428.718602. -- Candidate process 2: 325.698984, 335.498302, 325.648910, 364.891896, - 362.333156; median 335.498302. -- Pooled medians: baseline 448.507615, candidate 326.058643; 27.30 percent - reduction and 1.376x throughput. The two paired reductions were 32.39 and - 21.75 percent. - -Debug-process variance was material, especially in the first baseline -process. The raw sample bands stayed separated, and each independent 50 and -100 percent pair exceeded the 15 percent acceptance gate. These measurements -cover the tested short CitationForm values only. - -## Rejected and deferred work - -- A printable-ASCII `memcmp` shortcut was reverted because ICU collation can - equate strings that differ in punctuation and other non-ordinal ways. -- A managed ordinal negative precheck was exactly reverted because it did not - produce sufficient benefit. -- A one-character collation-ignorable cache and early extension exit were - reverted. Its answer survived a locale change and produced an incorrect - match span; the single-character premise also lacked an ICU contract for - contextual collation. -- The virtual table and budgeted coordinator were test-owned prototypes with - no shipping activation path. They and their acceptance and attribution - harnesses were discarded. -- Homograph renumber batching was deferred after characterization only. It - needs separate cross-repository design and measurements for undo, redo, - notifications, cache membership, and group numbering. From bb9d4cf6ba9a39a05830134be55d866741a08721 Mon Sep 17 00:00:00 2001 From: John Lambert Date: Thu, 27 Aug 2026 13:29:28 -0400 Subject: [PATCH 4/5] Address review: undo safety, COM contract, and search-kill plumbing Wrap the bulk-edit apply loop in UndoableUnitOfWorkHelper.Do so a failure part way through rolls the whole batch back instead of committing the rows already applied. The old test asserted only that the undo depth returned to zero, which passes under either outcome, so it now succeeds on the first row and throws on the second and checks the first was rolled back. Add an IVwSearchKiller parameter to IVwPattern2::ReplaceAllIn while the interface is still unshipped, and thread it through both replacement algorithms instead of the hard-coded NULL. Clear both ReplaceAllIn out-parameters before rejecting a null argument so the COM contract still holds when E_POINTER comes back. Extract VwPattern::WidenZeroLengthMatch and call it from both search paths, carrying the LT-6707 explanation with it rather than duplicating the fixup. Use ITsString::get_IsNormalizedForm instead of marshalling a BSTR through CustomIcu to test whether the replacement result is already NFD. Move the bulk-replacement tests to xWorksTests/Search beside the characterization fixture, add the missing Apartment(STA) attribute, document the null return from NewValue, and bring five over-budget comments back under the hygiene limit. --- Src/Common/Controls/XMLViews/BulkEditBar.cs | 32 ++++----- Src/views/Test/TestVwPattern.h | 61 ++++++++--------- Src/views/Views.idh | 3 + Src/views/VwPattern.cpp | 67 +++++++++---------- Src/views/VwPattern.h | 4 +- Src/xWorks/xWorksTests/BulkEditBarTests.cs | 40 ++++++++++- .../ReplaceWithMethodTests.cs | 7 +- .../VwPatternReplacementTests.cs | 12 ++-- 8 files changed, 129 insertions(+), 97 deletions(-) rename Src/xWorks/xWorksTests/{Avalonia/Performance => Search}/ReplaceWithMethodTests.cs (98%) rename Src/xWorks/xWorksTests/{Avalonia/Performance => Search}/VwPatternReplacementTests.cs (99%) diff --git a/Src/Common/Controls/XMLViews/BulkEditBar.cs b/Src/Common/Controls/XMLViews/BulkEditBar.cs index 3e1167e357..b234ace1bd 100644 --- a/Src/Common/Controls/XMLViews/BulkEditBar.cs +++ b/Src/Common/Controls/XMLViews/BulkEditBar.cs @@ -4837,12 +4837,15 @@ public void FakeDoit(IEnumerable itemsToChange, int tagFakeFlid, int tagEna public void Doit(IEnumerable itemsToChange, ProgressState state) { - m_sda.BeginUndoTask(XMLViewsStrings.ksUndoBulkEdit, XMLViewsStrings.ksRedoBulkEdit); - try + // A failure part way through rolls the whole batch back; a bulk edit that stopped + // on row 30,000 must not leave the first 29,999 applied as a completed undo task. + UndoableUnitOfWorkHelper.Do(XMLViewsStrings.ksUndoBulkEdit, XMLViewsStrings.ksRedoBulkEdit, + m_cache.ActionHandlerAccessor, () => { string commitChanges = XmlUtils.GetOptionalAttributeValue(m_nodeSpec, "commitChanges"); int i = 0; - // Report progress 50 times or every 100 items, whichever is more (but no more than once per item!) + // Report progress 50 times or every 100 items, whichever is more (but no more + // than once per item!) int interval = Math.Min(100, Math.Max(itemsToChange.Count() / 50, 1)); foreach (int hvo in itemsToChange) { @@ -4855,11 +4858,7 @@ public void Doit(IEnumerable itemsToChange, ProgressState state) Doit(hvo); BulkEditBar.CommitChanges(hvo, commitChanges, m_cache, m_accessor.WritingSystem); } - } - finally - { - m_sda.EndUndoTask(); - } + }); } /// @@ -4929,11 +4928,7 @@ protected virtual bool TryGetNewValue(int hvo, out ITsString newValue) private ITsString m_cachedNewValue; /// - /// Returns NewValue(hvo), reusing a value an OkToChange override already computed - /// for the same hvo within this same TryGetNewValue call (to decide whether a change - /// would occur) instead of recomputing it. The cache is cleared at the end of every - /// TryGetNewValue call: a later call for the same hvo (e.g. preview, then apply) must - /// still recompute, since the underlying data or pattern state may have changed. + /// Returns NewValue(hvo), computing it at most once per hvo until the cache is cleared. /// protected ITsString NewValueCached(int hvo) { @@ -4951,6 +4946,9 @@ private void ClearNewValueCache() m_cachedNewValue = null; } + /// + /// The value to store for this object, or null to leave the row unchanged. + /// protected abstract ITsString NewValue(int hvo); #region IGetReplacedObjects Members @@ -5186,12 +5184,8 @@ protected override ITsString NewValue(int hvo) private static ITsString NormalizeResult(ITsString tssResult) { - string text = tssResult.Text; - if (!string.IsNullOrEmpty(text) && - CustomIcu.GetIcuNormalizer(FwNormalizationMode.knmNFD).IsNormalized(text)) - { + if (tssResult.get_IsNormalizedForm(FwNormalizationMode.knmNFD)) return tssResult; - } return tssResult.get_NormalizedForm(FwNormalizationMode.knmNFD); } @@ -5208,7 +5202,7 @@ protected virtual bool TryReplaceAllIn(ITsString source, out ITsString result, return false; } - result = m_bulkPattern.ReplaceAllIn(source, 0, source.Length, out matchCount); + result = m_bulkPattern.ReplaceAllIn(source, 0, source.Length, null, out matchCount); return true; } diff --git a/Src/views/Test/TestVwPattern.h b/Src/views/Test/TestVwPattern.h index c5c4f6d1ec..33e4aa7bc7 100644 --- a/Src/views/Test/TestVwPattern.h +++ b/Src/views/Test/TestVwPattern.h @@ -1771,7 +1771,7 @@ namespace TestViews CheckHr(qtsi->QueryInterface(IID_IVwTextSource, (void **)&qts)); int cMatches = -1; ITsStringPtr qtssResult; - HRESULT hr = m_qpat2->ReplaceAllIn(qtssSource, 0, 7, &cMatches, &qtssResult); + HRESULT hr = m_qpat2->ReplaceAllIn(qtssSource, 0, 7, NULL, &cMatches, &qtssResult); unitpp::assert_eq("ReplaceAllIn succeeds", (int)S_OK, (int)hr); CheckHr(hr); unitpp::assert_eq("literal match count", 2, cMatches); @@ -1800,7 +1800,7 @@ namespace TestViews CheckHr(qtsi->QueryInterface(IID_IVwTextSource, (void **)&qts)); cMatches = -1; qtssResult.Clear(); - CheckHr(m_qpat2->ReplaceAllIn(qtssSource, 0, 7, &cMatches, &qtssResult)); + CheckHr(m_qpat2->ReplaceAllIn(qtssSource, 0, 7, NULL, &cMatches, &qtssResult)); unitpp::assert_eq("regex capture match count", 2, cMatches); unitpp::assert_eq("regex differential match count", cExpected, cMatches); CheckHr(qtssResult->Equals(qtssExpected, &fEqual)); @@ -1820,7 +1820,7 @@ namespace TestViews CheckHr(qtsi->QueryInterface(IID_IVwTextSource, (void **)&qts)); cMatches = -1; qtssResult.Clear(); - CheckHr(m_qpat2->ReplaceAllIn(qtssSource, 0, 3, &cMatches, &qtssResult)); + CheckHr(m_qpat2->ReplaceAllIn(qtssSource, 0, 3, NULL, &cMatches, &qtssResult)); unitpp::assert_eq("UTF-16 zero-width match count", 3, cMatches); unitpp::assert_eq("zero-width differential match count", cExpected, cMatches); CheckHr(qtssResult->Equals(qtssExpected, &fEqual)); @@ -1838,7 +1838,7 @@ namespace TestViews CheckHr(qtsi->QueryInterface(IID_IVwTextSource, (void **)&qts)); cMatches = -1; qtssResult.Clear(); - CheckHr(m_qpat2->ReplaceAllIn(qtssSource, 0, 3, &cMatches, &qtssResult)); + CheckHr(m_qpat2->ReplaceAllIn(qtssSource, 0, 3, NULL, &cMatches, &qtssResult)); unitpp::assert_eq("no-match count", 0, cMatches); unitpp::assert_eq("no-match differential count", cExpected, cMatches); CheckHr(qtssResult->Equals(qtssExpected, &fEqual)); @@ -1848,7 +1848,7 @@ namespace TestViews cMatches = 99; ITsString * ptssRawResult = reinterpret_cast(1); - hr = m_qpat2->ReplaceAllIn(NULL, 0, 3, &cMatches, &ptssRawResult); + hr = m_qpat2->ReplaceAllIn(NULL, 0, 3, NULL, &cMatches, &ptssRawResult); unitpp::assert_eq("null source is rejected", (int)E_POINTER, (int)hr); unitpp::assert_eq("count is cleared before null-source failure", 0, cMatches); unitpp::assert_true("result is cleared before null-source failure", @@ -1867,21 +1867,21 @@ namespace TestViews CheckHr(m_qpat->putref_Pattern(qtssPattern)); CheckHr(m_qpat->putref_ReplaceWith(qtssReplacement)); ITsString * ptssRawResult = reinterpret_cast(1); - HRESULT hr = m_qpat2->ReplaceAllIn(qtssSource, 0, 3, NULL, &ptssRawResult); + HRESULT hr = m_qpat2->ReplaceAllIn(qtssSource, 0, 3, NULL, NULL, &ptssRawResult); unitpp::assert_eq("null match count is rejected", (int)E_POINTER, (int)hr); - unitpp::assert_true("result is untouched when first output is null", - ptssRawResult == reinterpret_cast(1)); + unitpp::assert_true("result is cleared when the match count is null", + ptssRawResult == NULL); SetErrorInfo(0, NULL); int cMatches = 99; - hr = m_qpat2->ReplaceAllIn(qtssSource, 0, 3, &cMatches, NULL); + hr = m_qpat2->ReplaceAllIn(qtssSource, 0, 3, NULL, &cMatches, NULL); unitpp::assert_eq("null result is rejected", (int)E_POINTER, (int)hr); unitpp::assert_eq("count is cleared before null-result failure", 0, cMatches); SetErrorInfo(0, NULL); cMatches = 99; ptssRawResult = reinterpret_cast(1); - hr = m_qpat2->ReplaceAllIn(qtssSource, -1, 3, &cMatches, &ptssRawResult); + hr = m_qpat2->ReplaceAllIn(qtssSource, -1, 3, NULL, &cMatches, &ptssRawResult); unitpp::assert_eq("negative range is rejected", (int)E_INVALIDARG, (int)hr); unitpp::assert_eq("count is cleared before negative-range failure", 0, cMatches); @@ -1891,7 +1891,7 @@ namespace TestViews cMatches = 99; ptssRawResult = reinterpret_cast(1); - hr = m_qpat2->ReplaceAllIn(qtssSource, 2, 1, &cMatches, &ptssRawResult); + hr = m_qpat2->ReplaceAllIn(qtssSource, 2, 1, NULL, &cMatches, &ptssRawResult); unitpp::assert_eq("reversed range is rejected", (int)E_INVALIDARG, (int)hr); unitpp::assert_eq("count is cleared before reversed-range failure", 0, cMatches); @@ -1901,7 +1901,7 @@ namespace TestViews cMatches = 99; ptssRawResult = reinterpret_cast(1); - hr = m_qpat2->ReplaceAllIn(qtssSource, 0, 4, &cMatches, &ptssRawResult); + hr = m_qpat2->ReplaceAllIn(qtssSource, 0, 4, NULL, &cMatches, &ptssRawResult); unitpp::assert_eq("range past source is rejected", (int)E_INVALIDARG, (int)hr); unitpp::assert_eq("count is cleared before past-source failure", 0, @@ -1927,7 +1927,7 @@ namespace TestViews int cMatches = 99; ITsString * ptssResult = reinterpret_cast(1); - HRESULT hr = m_qpat2->ReplaceAllIn(qtssSource, 0, 7, &cMatches, &ptssResult); + HRESULT hr = m_qpat2->ReplaceAllIn(qtssSource, 0, 7, NULL, &cMatches, &ptssResult); SetErrorInfo(0, NULL); unitpp::assert_eq("replacement failure is returned", (int)E_FAIL, (int)hr); unitpp::assert_eq("count remains zero after replacement failure", 0, cMatches); @@ -1935,9 +1935,8 @@ namespace TestViews ptssResult == NULL); } - // Re-run the TE4727 regression (replacing the character immediately preceding a - // trailing owned ORC) through the bulk ReplaceAllIn path instead of FindIn, and - // confirm the ORC and its footnote guid data survive the splice. + // TE4727: replacing the character before a trailing owned ORC must leave the ORC + // and its footnote guid data intact through the splice. void testReplaceAllInReplaceCharPrecedingFinalORC_TE4727() { unitpp::assert_true("English writing system exists", m_qwsEng.Ptr()); @@ -1988,7 +1987,7 @@ namespace TestViews int cMatches = -1; ITsStringPtr qtssResult; - CheckHr(m_qpat2->ReplaceAllIn(qtssSource, 0, cchTotal, &cMatches, &qtssResult)); + CheckHr(m_qpat2->ReplaceAllIn(qtssSource, 0, cchTotal, NULL, &cMatches, &qtssResult)); unitpp::assert_eq("single 'e' preceding final ORC found", 1, cMatches); unitpp::assert_eq("match count matches repeated FindIn", cExpected, cMatches); ComBool fEqual; @@ -2010,9 +2009,8 @@ namespace TestViews !wcscmp(stuData.Chars(), sbstrObjData.Chars())); } - // Re-run the canonical/writing-system-restricted matching combinations from - // testMatchingWs through ReplaceAllIn, confirming bulk match counts and results - // agree with repeated FindIn as MatchOldWritingSystem and MatchDiacritics change. + // Bulk match counts and results must agree with repeated FindIn as + // MatchOldWritingSystem and MatchDiacritics change. void testReplaceAllInRespectsMatchOldWritingSystem() { unitpp::assert_true("English writing system exists", m_qwsEng.Ptr()); @@ -2053,7 +2051,7 @@ namespace TestViews &cExpected); int cMatches = -1; ITsStringPtr qtssResult; - CheckHr(m_qpat2->ReplaceAllIn(qtssSearch, 0, len, &cMatches, &qtssResult)); + CheckHr(m_qpat2->ReplaceAllIn(qtssSearch, 0, len, NULL, &cMatches, &qtssResult)); unitpp::assert_eq("both canonical matches replaced without ws matching", 2, cMatches); unitpp::assert_eq("count matches repeated FindIn (no ws)", cExpected, cMatches); @@ -2066,7 +2064,7 @@ namespace TestViews qtssExpected = ReplaceWithRepeatedFindIn(qtssSearch, 0, len, &cExpected); cMatches = -1; qtssResult.Clear(); - CheckHr(m_qpat2->ReplaceAllIn(qtssSearch, 0, len, &cMatches, &qtssResult)); + CheckHr(m_qpat2->ReplaceAllIn(qtssSearch, 0, len, NULL, &cMatches, &qtssResult)); unitpp::assert_eq("both canonical matches replaced, ws required, diacritics ignored", 2, cMatches); unitpp::assert_eq("count matches repeated FindIn (ws, no diacritics)", cExpected, @@ -2080,7 +2078,7 @@ namespace TestViews qtssExpected = ReplaceWithRepeatedFindIn(qtssSearch, 0, len, &cExpected); cMatches = -1; qtssResult.Clear(); - CheckHr(m_qpat2->ReplaceAllIn(qtssSearch, 0, len, &cMatches, &qtssResult)); + CheckHr(m_qpat2->ReplaceAllIn(qtssSearch, 0, len, NULL, &cMatches, &qtssResult)); unitpp::assert_eq("only first occurrence matches ws+diacritics", 1, cMatches); unitpp::assert_eq("count matches repeated FindIn (ws + diacritics)", cExpected, cMatches); @@ -2119,7 +2117,7 @@ namespace TestViews &cExpected); int cMatches = -1; ITsStringPtr qtssResult; - CheckHr(m_qpat2->ReplaceAllIn(qtssSearchCase, 0, lenCase, &cMatches, &qtssResult)); + CheckHr(m_qpat2->ReplaceAllIn(qtssSearchCase, 0, lenCase, NULL, &cMatches, &qtssResult)); unitpp::assert_eq("case-insensitive replaces both 'And' and 'and'", 2, cMatches); unitpp::assert_eq("case-insensitive count matches repeated FindIn", cExpected, cMatches); @@ -2130,7 +2128,7 @@ namespace TestViews qtssExpected = ReplaceWithRepeatedFindIn(qtssSearchCase, 0, lenCase, &cExpected); cMatches = -1; qtssResult.Clear(); - CheckHr(m_qpat2->ReplaceAllIn(qtssSearchCase, 0, lenCase, &cMatches, &qtssResult)); + CheckHr(m_qpat2->ReplaceAllIn(qtssSearchCase, 0, lenCase, NULL, &cMatches, &qtssResult)); unitpp::assert_eq("match case skips capitalized 'And'", 1, cMatches); unitpp::assert_eq("match-case count matches repeated FindIn", cExpected, cMatches); CheckHr(qtssResult->Equals(qtssExpected, &fEqual)); @@ -2153,7 +2151,7 @@ namespace TestViews qtssExpected = ReplaceWithRepeatedFindIn(qtssSearchDia, 0, lenDia, &cExpected); cMatches = -1; qtssResult.Clear(); - CheckHr(m_qpat2->ReplaceAllIn(qtssSearchDia, 0, lenDia, &cMatches, &qtssResult)); + CheckHr(m_qpat2->ReplaceAllIn(qtssSearchDia, 0, lenDia, NULL, &cMatches, &qtssResult)); unitpp::assert_eq("ignoring diacritics replaces both occurrences", 2, cMatches); unitpp::assert_eq("ignore-diacritics count matches repeated FindIn", cExpected, cMatches); @@ -2164,7 +2162,7 @@ namespace TestViews qtssExpected = ReplaceWithRepeatedFindIn(qtssSearchDia, 0, lenDia, &cExpected); cMatches = -1; qtssResult.Clear(); - CheckHr(m_qpat2->ReplaceAllIn(qtssSearchDia, 0, lenDia, &cMatches, &qtssResult)); + CheckHr(m_qpat2->ReplaceAllIn(qtssSearchDia, 0, lenDia, NULL, &cMatches, &qtssResult)); unitpp::assert_eq("requiring diacritics skips the accented occurrence", 1, cMatches); unitpp::assert_eq("match-diacritics count matches repeated FindIn", cExpected, @@ -2173,9 +2171,8 @@ namespace TestViews unitpp::assert_true("match-diacritics bulk result equals repeated FindIn", fEqual); } - // Re-run testSearchCanonical's NFD canonical-equivalence scenario through - // ReplaceAllIn: the pattern is correctly-ordered decomposed text, while the source - // contains both a fully-composed and an out-of-order-decomposed occurrence. + // The pattern is correctly-ordered decomposed text; the source holds both a + // fully-composed and an out-of-order-decomposed occurrence of it. void testReplaceAllInCanonicalEquivalence() { unitpp::assert_true("English writing system exists", m_qwsEng.Ptr()); @@ -2205,7 +2202,7 @@ namespace TestViews &cExpected); int cMatches = -1; ITsStringPtr qtssResult; - CheckHr(m_qpat2->ReplaceAllIn(qtssSearch, 0, len, &cMatches, &qtssResult)); + CheckHr(m_qpat2->ReplaceAllIn(qtssSearch, 0, len, NULL, &cMatches, &qtssResult)); unitpp::assert_eq("both canonically-equivalent occurrences replaced", 2, cMatches); unitpp::assert_eq("canonical count matches repeated FindIn", cExpected, cMatches); CheckHr(qtssResult->Equals(qtssExpected, &fEqual)); @@ -2222,7 +2219,7 @@ namespace TestViews qtssExpected = ReplaceWithRepeatedFindIn(qtssSearch, 0, len, &cExpected); cMatches = -1; qtssResult.Clear(); - CheckHr(m_qpat2->ReplaceAllIn(qtssSearch, 0, len, &cMatches, &qtssResult)); + CheckHr(m_qpat2->ReplaceAllIn(qtssSearch, 0, len, NULL, &cMatches, &qtssResult)); unitpp::assert_eq("both occurrences still replaced, match case/diacritics", 2, cMatches); unitpp::assert_eq("canonical count matches repeated FindIn, match case/diacritics", diff --git a/Src/views/Views.idh b/Src/views/Views.idh index d45d805e66..71e344162b 100644 --- a/Src/views/Views.idh +++ b/Src/views/Views.idh @@ -4261,10 +4261,13 @@ Last reviewed: // Replaces non-overlapping forward matches in the UTF-16 range. // No matches returns the source. // Success leaves the pattern in a terminal no-match state. + // pxserkl may be NULL; a regular-expression replace over a long string is + // otherwise uninterruptible. HRESULT ReplaceAllIn( [in] ITsString * ptss, [in] int ichStart, [in] int ichEnd, + [in] IVwSearchKiller * pxserkl, [out] int * pcMatches, [out, retval] ITsString ** pptssResult); }; diff --git a/Src/views/VwPattern.cpp b/Src/views/VwPattern.cpp index 5278c42976..fa45221705 100644 --- a/Src/views/VwPattern.cpp +++ b/Src/views/VwPattern.cpp @@ -32,6 +32,25 @@ static int CheckedPatternPosition(__int64 value) return static_cast(value); } +/*---------------------------------------------------------------------------------------------- + Widen a match that came back empty so it covers one character, clamped to ichEndLog. + A zero-length match is otherwise found again by every subsequent search from the same + spot. The only non-regular-expression way to get one is a match inside the text of a hot + link, where the whole link is selected. A zero-length match at position 0 is legitimate + for the regular expression "^" and is left alone. See LT-6707. +----------------------------------------------------------------------------------------------*/ +void VwPattern::WidenZeroLengthMatch(int ichMin, int * pichLim, int ichEndLog) +{ + AssertPtr(pichLim); + if (ichMin != *pichLim) + return; + if (ichMin == 0 && m_fUseRegularExpressions && m_stuCompiled.Equals(L"^")) + return; + *pichLim = CheckedPatternPosition(static_cast<__int64>(*pichLim) + 1); + if (*pichLim > ichEndLog) + *pichLim = ichEndLog; +} + /*---------------------------------------------------------------------------------------------- Constructor. ----------------------------------------------------------------------------------------------*/ @@ -1493,7 +1512,7 @@ class FindInAlgorithm : public FindInAlgorithmBase m_ichLimFoundSearch = m_ichMinFoundSearch + m_piter->getMatchedLength(); AdjustSearchLimitForDiacritics(); if (m_ichLimFoundSearch > m_ichLimSearch) - return Fail(); // The first match extends past the end of our range. + return Fail(); // This match extends past the end of our range. if (m_pat->m_fMatchDiacritics && !CheckMatchDiacritic()) continue; if (CheckAndExtendCandidate()) @@ -1680,23 +1699,7 @@ STDMETHODIMP VwPattern::FindIn(IVwTextSource * pts, int ichStartLog, int ichEndL CheckHr(pts->SearchToLog(ichMinFoundSearch, false, pichMinFoundLog)); CheckHr(pts->SearchToLog(ichLimFoundSearch, true, pichLimFoundLog)); - if (*pichMinFoundLog == *pichLimFoundLog) - { - // Zero length match at beginning is okay for regular expression "^". See LT-6707. - if (*pichMinFoundLog > 0 || !m_fUseRegularExpressions || !m_stuCompiled.Equals(L"^")) - { - // The only way this can happen is a match inside the text of a hot link. - // Select the whole link. (Other solutions are possible, but beware of the - // previous behavior of leaving them the same: this produces an IP at the - // start of the hot link, and then every subsequent search finds it again.) - (*pichLimFoundLog)++; - // not so not so.... - // a search like \n* or \s* or * also comes through here... and crashes - // with out the following check. - if (*pichLimFoundLog > ichEndLog) - *pichLimFoundLog = ichEndLog; // cant be larger than lim - } - } + WidenZeroLengthMatch(*pichMinFoundLog, pichLimFoundLog, ichEndLog); } m_ichMinFoundLog = *pichMinFoundLog; @@ -1731,15 +1734,7 @@ void VwPattern::ReplaceAllWithAlgorithm(FindInAlgorithmBase * pfia, IVwTextSourc int ichLim; CheckHr(pts->SearchToLog(pfia->m_ichMinFoundSearch, false, &ichMin)); CheckHr(pts->SearchToLog(pfia->m_ichLimFoundSearch, true, &ichLim)); - if (ichMin == ichLim) - { - if (ichMin > 0 || !m_fUseRegularExpressions || !m_stuCompiled.Equals(L"^")) - { - ichLim = CheckedPatternPosition(static_cast<__int64>(ichLim) + 1); - if (ichLim > ichEnd) - ichLim = ichEnd; - } - } + WidenZeroLengthMatch(ichMin, &ichLim, ichEnd); m_ichMinFoundLog = ichMin; m_ichLimFoundLog = ichLim; @@ -1784,13 +1779,17 @@ void VwPattern::ReplaceAllWithAlgorithm(FindInAlgorithmBase * pfia, IVwTextSourc } STDMETHODIMP VwPattern::ReplaceAllIn(ITsString * ptss, int ichStart, int ichEnd, - int * pcMatches, ITsString ** pptssResult) + IVwSearchKiller * pxserkl, int * pcMatches, ITsString ** pptssResult) { BEGIN_COM_METHOD; - ChkComOutPtr(pcMatches); - *pcMatches = 0; - ChkComOutPtr(pptssResult); - *pptssResult = NULL; + // Clear every output actually supplied before rejecting a null one: COM requires an + // [out] parameter be cleared on failure, including when a different one is at fault. + if (pcMatches) + *pcMatches = 0; + if (pptssResult) + *pptssResult = NULL; + ChkComArgPtr(pcMatches); + ChkComArgPtr(pptssResult); ChkComArgPtr(ptss); if (ichStart < 0 || ichEnd < ichStart) ThrowHr(WarnHr(E_INVALIDARG)); @@ -1807,13 +1806,13 @@ STDMETHODIMP VwPattern::ReplaceAllIn(ITsString * ptss, int ichStart, int ichEnd, ITsStringPtr qtssResult; if (m_fUseRegularExpressions) { - RegExFindInAlgorithm refia(qts, ichStart, ichEnd, true, NULL, this); + RegExFindInAlgorithm refia(qts, ichStart, ichEnd, true, pxserkl, this); ReplaceAllWithAlgorithm(&refia, qts, ptss, ichStart, ichEnd, &cMatches, &qtssResult); } else { - FindInAlgorithm fia(qts, ichStart, ichEnd, true, NULL, this); + FindInAlgorithm fia(qts, ichStart, ichEnd, true, pxserkl, this); ReplaceAllWithAlgorithm(&fia, qts, ptss, ichStart, ichEnd, &cMatches, &qtssResult); } diff --git a/Src/views/VwPattern.h b/Src/views/VwPattern.h index 032f35f532..43f27b72e3 100644 --- a/Src/views/VwPattern.h +++ b/Src/views/VwPattern.h @@ -106,7 +106,7 @@ class VwPattern : public IVwPattern2 STDMETHOD(get_ReplacementText)(ITsString ** pptssText); STDMETHOD(get_Group)(int iGroup, ITsString ** pptssGroup); STDMETHOD(ReplaceAllIn)(ITsString * ptss, int ichStart, int ichEnd, - int * pcMatches, ITsString ** pptssResult); + IVwSearchKiller * pxserkl, int * pcMatches, ITsString ** pptssResult); // Other public methods @@ -117,6 +117,8 @@ class VwPattern : public IVwPattern2 void SetFound(bool fFound = true) {m_fFound = fFound;} void RemoveIgnorableRuns(ITsString * ptssIn, ITsString ** pptssOut); + void WidenZeroLengthMatch(int ichMin, int * pichLim, int ichEndLog); + protected: // Member variables ITsStringPtr m_qtssPattern; // Characters to search for, writing system may be significant. diff --git a/Src/xWorks/xWorksTests/BulkEditBarTests.cs b/Src/xWorks/xWorksTests/BulkEditBarTests.cs index a9bd390927..78e68e413f 100644 --- a/Src/xWorks/xWorksTests/BulkEditBarTests.cs +++ b/Src/xWorks/xWorksTests/BulkEditBarTests.cs @@ -682,17 +682,53 @@ public void Doit_OuterLoop_EndsUndoTaskWhenAnItemThrows() "the outer bulk-edit undo task must be ended even when applying one item throws"); } + [Test] + public void Doit_OuterLoop_RollsBackItemsAppliedBeforeTheFailure() + { + var entries = Cache.LangProject.LexDbOA.Entries.Take(2).ToList(); + Assert.That(entries.Count, Is.EqualTo(2), "this test needs two entries to work with"); + var hvoApplied = entries[0].Hvo; + var hvoThatThrows = entries[1].Hvo; + + var document = new XmlDocument(); + document.LoadXml(""); + var accessor = FieldReadWriter.Create(document.DocumentElement, Cache); + var originalValue = accessor.CurrentValue(hvoApplied); + var newValue = TsStringUtils.MakeString("bulk edited", Cache.DefaultVernWs); + + var method = new ThrowingDoItMethod(Cache, (ISilDataAccessManaged)Cache.DomainDataByFlid, + accessor, document.DocumentElement, hvoThatThrows, newValue); + + Assert.Throws(() => + method.Doit(new[] { hvoApplied, hvoThatThrows }, new NullProgressState())); + + Assert.That(accessor.CurrentValue(hvoApplied).Text, Is.EqualTo(originalValue?.Text), + "a bulk edit that failed part way through must roll back the rows it had already applied, " + + "not commit them as a completed undo task"); + Assert.That(Cache.ActionHandlerAccessor.CurrentDepth, Is.EqualTo(0)); + } + private sealed class ThrowingDoItMethod : DoItMethod { + private readonly int m_hvoThatThrows; + private readonly ITsString m_valueForOthers; + + /// Applies to every hvo except + /// , which throws instead. internal ThrowingDoItMethod(LcmCache cache, ISilDataAccessManaged sda, - FieldReadWriter accessor, XmlNode spec) + FieldReadWriter accessor, XmlNode spec, int hvoThatThrows = 0, + ITsString valueForOthers = null) : base(cache, sda, accessor, spec) { + m_hvoThatThrows = hvoThatThrows; + m_valueForOthers = valueForOthers; } protected override ITsString NewValue(int hvo) { - throw new InvalidOperationException("simulated failure applying one bulk-edit item"); + if (m_valueForOthers == null || hvo == m_hvoThatThrows) + throw new InvalidOperationException("simulated failure applying one bulk-edit item"); + return m_valueForOthers; } } diff --git a/Src/xWorks/xWorksTests/Avalonia/Performance/ReplaceWithMethodTests.cs b/Src/xWorks/xWorksTests/Search/ReplaceWithMethodTests.cs similarity index 98% rename from Src/xWorks/xWorksTests/Avalonia/Performance/ReplaceWithMethodTests.cs rename to Src/xWorks/xWorksTests/Search/ReplaceWithMethodTests.cs index 81bd814a41..e6de253a33 100644 --- a/Src/xWorks/xWorksTests/Avalonia/Performance/ReplaceWithMethodTests.cs +++ b/Src/xWorks/xWorksTests/Search/ReplaceWithMethodTests.cs @@ -18,7 +18,7 @@ using SIL.LCModel.Core.Text; using SIL.LCModel.Infrastructure; -namespace SIL.FieldWorks.XWorks.Performance +namespace SIL.FieldWorks.XWorks.Search { [TestFixture] [Category("BulkReplacement")] @@ -269,9 +269,8 @@ public void FakeDoit_NormalizesBulkReplacementResultToNfd() [Test] public void FakeDoit_NormalizesNonLatinScriptReplacementResultToNfd() { - // U+AC00 (Hangul syllable "GA") has a real canonical decomposition to two - // Jamo characters under NFD, unlike the single-diacritic Latin case above. - // This hardens the NormalizeResult skip-check for a non-Latin script. + // U+AC00 (Hangul syllable "GA") decomposes to two Jamo under NFD, so this + // exercises the NormalizeResult skip-check on a non-Latin script. var entry = CreateEntries(1, 1, "old \uac00", "old \uac00").Single(); var document = BuildColumnSpec(); var accessor = FieldReadWriter.Create(document.DocumentElement, Cache); diff --git a/Src/xWorks/xWorksTests/Avalonia/Performance/VwPatternReplacementTests.cs b/Src/xWorks/xWorksTests/Search/VwPatternReplacementTests.cs similarity index 99% rename from Src/xWorks/xWorksTests/Avalonia/Performance/VwPatternReplacementTests.cs rename to Src/xWorks/xWorksTests/Search/VwPatternReplacementTests.cs index 8cdf3ab56e..caee33c7f3 100644 --- a/Src/xWorks/xWorksTests/Avalonia/Performance/VwPatternReplacementTests.cs +++ b/Src/xWorks/xWorksTests/Search/VwPatternReplacementTests.cs @@ -4,15 +4,17 @@ using System.Collections.Generic; using System.Runtime.InteropServices; +using System.Threading; using NUnit.Framework; using SIL.FieldWorks.Common.ViewsInterfaces; using SIL.LCModel; using SIL.LCModel.Core.KernelInterfaces; using SIL.LCModel.Core.Text; -namespace SIL.FieldWorks.XWorks.Performance +namespace SIL.FieldWorks.XWorks.Search { [TestFixture] + [Apartment(ApartmentState.STA)] [Category("BulkReplacement")] public class VwPatternReplacementTests : MemoryOnlyBackendProviderTestBase { @@ -143,7 +145,7 @@ private static void AssertReplaceAllMatchesRepeatedFindIn(IVwPattern pattern, delta += match.Replacement.Length - (match.IchLim - match.IchMin); } - var actual = ((IVwPattern2)pattern).ReplaceAllIn(source, ichStart, ichEnd, + var actual = ((IVwPattern2)pattern).ReplaceAllIn(source, ichStart, ichEnd, null, out var actualCount); Assert.That(actualCount, Is.EqualTo(matches.Count), "bulk match count"); if (expectedCount.HasValue) @@ -426,7 +428,7 @@ public void ReplaceAllIn_EmptyTextPattern_ReplacesRunMatchingAllProperties() "Replacement Style"); pattern.ReplaceWith = replacementBuilder.GetString(); - var actual = ((IVwPattern2)pattern).ReplaceAllIn(source, 0, source.Length, + var actual = ((IVwPattern2)pattern).ReplaceAllIn(source, 0, source.Length, null, out var count); Assert.That(count, Is.EqualTo(1)); @@ -447,7 +449,7 @@ public void ReplaceAllIn_PreservesObjectReplacementCharacterAndObjectData() var pattern = MakePattern("old"); pattern.ReplaceWith = TsStringUtils.MakeString("new", Ws); - var actual = ((IVwPattern2)pattern).ReplaceAllIn(source, 0, source.Length, + var actual = ((IVwPattern2)pattern).ReplaceAllIn(source, 0, source.Length, null, out var count); Assert.That(count, Is.EqualTo(1)); @@ -465,7 +467,7 @@ public void ReplaceAllIn_LeavesTerminalNoMatchState() pattern.ReplaceWith = TsStringUtils.MakeString("new", Ws); var source = TsStringUtils.MakeString("old old", Ws); - var actual = ((IVwPattern2)pattern).ReplaceAllIn(source, 0, source.Length, + var actual = ((IVwPattern2)pattern).ReplaceAllIn(source, 0, source.Length, null, out var count); Assert.That(actual.Text, Is.EqualTo("new new")); From 39c91223e346ef9fd5b3bbf8934d368fe14fbf9e Mon Sep 17 00:00:00 2001 From: John Lambert Date: Thu, 27 Aug 2026 14:22:03 -0400 Subject: [PATCH 5/5] Pin the two replacement engines against each other in tests Every case in BulkEditReplaceCharacterizationTests now runs through both the ReplaceAllIn path and the forced repeated-FindIn fallback, and the fixture requires identical text, run structure and writing system from both. That fixture already existed to record bulk replacement's preview and apply behaviour, so extending it is a better answer than writing new tests beside it, and it catches the failure mode this branch introduces: two engines that drift apart. Add a second normalization case where a run boundary falls between a base character and its combining mark in text that is already NFD. The skip path added here returns the builder's string untouched in that case, so run segmentation has to survive exactly as get_NormalizedForm would leave it, and one oracle case was thin evidence for that. --- .../BulkEditReplaceCharacterizationTests.cs | 80 ++++++++++++++++++- .../Search/ReplaceWithMethodTests.cs | 74 +++++++++++++++++ 2 files changed, 153 insertions(+), 1 deletion(-) diff --git a/Src/xWorks/xWorksTests/Search/BulkEditReplaceCharacterizationTests.cs b/Src/xWorks/xWorksTests/Search/BulkEditReplaceCharacterizationTests.cs index 1c6916f4b0..3add8b7d67 100644 --- a/Src/xWorks/xWorksTests/Search/BulkEditReplaceCharacterizationTests.cs +++ b/Src/xWorks/xWorksTests/Search/BulkEditReplaceCharacterizationTests.cs @@ -2,12 +2,14 @@ // This software is licensed under the LGPL, version 2.1 or later // (http://www.gnu.org/licenses/lgpl-2.1.html) +using System; using System.Collections.Generic; using System.Xml; using NUnit.Framework; using SIL.FieldWorks.Common.Controls; using SIL.FieldWorks.Common.ViewsInterfaces; using SIL.LCModel; +using SIL.LCModel.Application; using SIL.LCModel.Core.KernelInterfaces; using SIL.LCModel.Core.Text; using SIL.LCModel.Infrastructure; @@ -122,6 +124,55 @@ public void UnmatchedRow_IsDisabledAndHasNoPreviewReplacement() Assert.That(entry.CitationForm.get_String(Cache.DefaultVernWs).Text, Is.EqualTo("abc")); } + /// + /// Every case must come out the same through the bulk ReplaceAllIn path and through the + /// repeated-FindIn fallback, so the two engines cannot drift apart unnoticed. + /// + [Test] + public void BulkAndFallbackEngines_ProduceTheSameResult() + { + foreach (var replacementCase in ReplacementCases) + { + var bulk = RunCase(replacementCase, false); + var fallback = RunCase(replacementCase, true); + AssertSameString(bulk.Item1, fallback.Item1, replacementCase.Name + " preview"); + AssertSameString(bulk.Item2, fallback.Item2, replacementCase.Name + " apply"); + } + } + + private Tuple RunCase(ReplacementCase replacementCase, bool fallback) + { + var entry = CreateEntry(replacementCase.Input); + var method = CreateReplaceMethod(replacementCase, fallback); + + method.FakeDoit(new[] { entry.Hvo }, XMLViewsDataCache.ktagAlternateValue, + XMLViewsDataCache.ktagItemEnabled, new NullProgressState()); + var preview = m_bv.SpecialCache.get_StringProp(entry.Hvo, + XMLViewsDataCache.ktagAlternateValue); + + Cache.DomainDataByFlid.BeginUndoTask("characterization engine apply", + "characterization engine apply"); + method.Doit(entry.Hvo); + Cache.DomainDataByFlid.EndUndoTask(); + var applied = entry.CitationForm.get_String(Cache.DefaultVernWs); + Cache.ActionHandlerAccessor.Undo(); + + return new Tuple(preview, applied); + } + + private static void AssertSameString(ITsString expected, ITsString actual, string message) + { + Assert.That(actual.Text, Is.EqualTo(expected.Text), message); + Assert.That(actual.RunCount, Is.EqualTo(expected.RunCount), message); + for (var run = 0; run < expected.RunCount; run++) + { + Assert.That(actual.get_RunText(run), Is.EqualTo(expected.get_RunText(run)), + message); + Assert.That(TsStringUtils.GetWsOfRun(actual, run), + Is.EqualTo(TsStringUtils.GetWsOfRun(expected, run)), message); + } + } + private ILexEntry CreateEntry(string text) { ILexEntry entry = null; @@ -135,7 +186,8 @@ private ILexEntry CreateEntry(string text) return entry; } - private ReplaceWithMethod CreateReplaceMethod(ReplacementCase replacementCase) + private ReplaceWithMethod CreateReplaceMethod(ReplacementCase replacementCase, + bool fallback = false) { var document = new XmlDocument(); document.LoadXml(""); @@ -148,10 +200,36 @@ private ReplaceWithMethod CreateReplaceMethod(ReplacementCase replacementCase) pattern.UseRegularExpressions = replacementCase.UseRegularExpressions; pattern.ReplaceWith = TsStringUtils.MakeString(replacementCase.Replacement, Cache.DefaultVernWs); + if (fallback) + { + return new FallbackReplaceWithMethod(Cache, m_bv.SpecialCache, accessor, + document.DocumentElement, pattern, pattern.ReplaceWith); + } return new ReplaceWithMethod(Cache, m_bv.SpecialCache, accessor, document.DocumentElement, pattern, pattern.ReplaceWith); } + /// + /// Declines the bulk replacement capability so the caller runs on the repeated-FindIn + /// path instead. + /// + private sealed class FallbackReplaceWithMethod : ReplaceWithMethod + { + internal FallbackReplaceWithMethod(LcmCache cache, ISilDataAccessManaged sda, + FieldReadWriter accessor, XmlNode spec, IVwPattern pattern, ITsString replacement) + : base(cache, sda, accessor, spec, pattern, replacement) + { + } + + protected override bool TryReplaceAllIn(ITsString source, out ITsString result, + out int matchCount) + { + result = null; + matchCount = 0; + return false; + } + } + private void AssertResult(ITsString result, string expectedText, string message) { Assert.That(result, Is.Not.Null, message); diff --git a/Src/xWorks/xWorksTests/Search/ReplaceWithMethodTests.cs b/Src/xWorks/xWorksTests/Search/ReplaceWithMethodTests.cs index e6de253a33..a53bd9e1cb 100644 --- a/Src/xWorks/xWorksTests/Search/ReplaceWithMethodTests.cs +++ b/Src/xWorks/xWorksTests/Search/ReplaceWithMethodTests.cs @@ -522,6 +522,80 @@ public void FakeDoit_MatchesOracleForDecoratorBackedUnnormalizedRichValue() out var differences), Is.True, differences); } + /// + /// The skip path returns the builder's string when the text is already NFD, so a run + /// boundary that falls between a base character and its combining mark must survive + /// exactly as get_NormalizedForm would leave it. + /// + [Test] + public void FakeDoit_MatchesOracleForAlreadyNfdValueWithRunSplitInsideCombiningSequence() + { + ILexEntry entry = null; + NonUndoableUnitOfWorkHelper.Do(Cache.ActionHandlerAccessor, () => + { + entry = Cache.ServiceLocator.GetInstance().Create(); + m_createdObjectList.Add(entry); + }); + var sourceBuilder = TsStringUtils.MakeString("old á old", Cache.DefaultVernWs) + .GetBldr(); + sourceBuilder.SetStrPropValue(5, 6, (int)FwTextPropType.ktptNamedStyle, + "Split Combining Mark"); + m_bv.SpecialCache.SetString(entry.Hvo, XMLViewsDataCache.ktagAlternateValue, + sourceBuilder.GetString()); + var decoratedValue = m_bv.SpecialCache.get_StringProp(entry.Hvo, + XMLViewsDataCache.ktagAlternateValue); + Assert.That(decoratedValue.Text.IsNormalized(NormalizationForm.FormD), Is.True); + Assert.That(decoratedValue.RunCount, Is.EqualTo(3)); + + var pattern = VwPatternClass.Create(); + pattern.Pattern = TsStringUtils.MakeString("old", Cache.DefaultVernWs); + pattern.ReplaceWith = TsStringUtils.MakeString("new", Cache.DefaultVernWs); + pattern.MatchCase = true; + pattern.MatchDiacritics = true; + var oracleInit = VwStringTextSourceClass.Create(); + oracleInit.SetString(decoratedValue); + var oracleSource = (IVwTextSource)oracleInit; + var expectedBuilder = decoratedValue.GetBldr(); + var ichStart = 0; + var delta = 0; + var expectedCount = 0; + while (ichStart <= decoratedValue.Length) + { + pattern.FindIn(oracleSource, ichStart, decoratedValue.Length, true, + out var ichMin, out var ichLim, null); + if (ichMin < 0) + break; + var replacement = pattern.ReplacementText; + expectedBuilder.ReplaceTsString(ichMin + delta, ichLim + delta, replacement); + delta += replacement.Length - (ichLim - ichMin); + expectedCount++; + ichStart = ichLim; + } + + Assert.That(expectedCount, Is.EqualTo(2)); + var document = new XmlDocument(); + document.LoadXml(""); + var accessor = new DecoratorStringReadWriter(m_bv.SpecialCache, + XMLViewsDataCache.ktagAlternateValue, Cache.DefaultVernWs); + var method = new ReplaceWithMethodPreviewTests.CountingReplaceWithMethod(Cache, + m_bv.SpecialCache, accessor, document.DocumentElement, pattern, + pattern.ReplaceWith); + + method.FakeDoit(new[] { entry.Hvo }, XMLViewsDataCache.ktagAlternateValue, + XMLViewsDataCache.ktagItemEnabled, new NullProgressState()); + + var actual = m_bv.SpecialCache.get_StringProp(entry.Hvo, + XMLViewsDataCache.ktagAlternateValue); + var expected = expectedBuilder.GetString() + .get_NormalizedForm(FwNormalizationMode.knmNFD); + Assert.That(method.ReplaceAllInCount, Is.EqualTo(1)); + Assert.That(method.FindInCount, Is.Zero); + Assert.That(actual.Text, Is.EqualTo("new á new")); + Assert.That(actual.RunCount, Is.EqualTo(expected.RunCount)); + Assert.That(TsStringHelper.TsStringsAreEqual(expected, actual, + out var differences), Is.True, differences); + } + private sealed class DecoratorStringReadWriter : FieldReadWriter { private readonly int m_tag;