[P3] markers-checklist: follow-up - Sebastian UX-2 review (9 work packages)

c-sharp-tests/Checklists/ChecklistServiceBuildChecklistDataTests.cs

@@ -733,12 +733,20 @@ public void BuildChecklistData_CancellationRequested_Throws()
     [Property("BehaviorId", "BHV-100")]
     public void BuildChecklistData_ChecklistTypeMarkers_ComposesMarkersPipeline()
     {
-        // TS-053: the Markers pipeline is composed under the hood. Indirect
-        // observation via BHV-103 — MarkersDataSource.PostProcessParagraph
-        // prepends a backslash-prefixed marker TextItem at position 0 of
-        // every paragraph's Items (INV-004). If the service did NOT route
-        // through MarkersDataSource, the first item of each paragraph would
-        // not be a TextItem with text "\p" / "\q" / "\q2".
+        // TS-053 (revised post-UX-2 finding #13): the Markers pipeline is
+        // composed under the hood. We observe BHV-103 indirectly: with
+        // showVerseText=true, the original verse-text Items (verse markers
+        // and text fragments produced by the cell builder) flow through
+        // MarkersDataSource.PostProcessParagraph unchanged — they are NOT
+        // dropped, and they are NOT prefixed with a redundant `\marker`
+        // TextItem (the UI renders the marker from paragraph.Marker).
+        //
+        // If the service did NOT route through MarkersDataSource, the
+        // showVerseText flag would have no effect; the paragraph items
+        // would always be present. With showVerseText=true that's
+        // indistinguishable, so instead we assert the new INV-004 contract:
+        // paragraph.Marker is set and Items contain only content items
+        // (verse markers / text), never the prepended `\marker` TextItem.
         var scrText = RegisterDummyProject(Gm001ExoUsfm);
         var request = BuildRequest(activeProjectId: scrText.Guid.ToString(), showVerseText: true);
 
@@ -752,23 +760,26 @@ public void BuildChecklistData_ChecklistTypeMarkers_ComposesMarkersPipeline()
         foreach (var cell in row.Cells)
         foreach (var paragraph in cell.Paragraphs)
         {
-            Assume.That(
-                paragraph.Items,
-                Is.Not.Empty,
-                $"precondition — paragraph {paragraph.Marker} has items"
-            );
-            var first = paragraph.Items[0];
             Assert.That(
-                first,
-                Is.InstanceOf<TextItem>(),
-                "BHV-103 / INV-004 — first item must be TextItem carrying backslash-prefixed marker"
-            );
-            var firstText = (TextItem)first;
-            Assert.That(
-                firstText.Text,
-                Is.EqualTo("\\" + paragraph.Marker),
-                $"BHV-103 / INV-004 — first TextItem.Text must equal \\{paragraph.Marker}"
+                paragraph.Marker,
+                Is.Not.Null.And.Not.Empty,
+                "INV-004 — paragraph.Marker carries the marker name (UI renders the backslash prefix)"
             );
+            // INV-004 (revised): the redundant "\\" + Marker TextItem is no
+            // longer prepended. Verify no item in the list starts with the
+            // backslash-prefix marker as its sole text payload.
+            var backslashMarker = "\\" + paragraph.Marker;
+            foreach (var item in paragraph.Items)
+            {
+                if (item is TextItem text)
+                {
+                    Assert.That(
+                        text.Text,
+                        Is.Not.EqualTo(backslashMarker),
+                        $"INV-004 (revised) — Items must not contain the prepended marker TextItem '{backslashMarker}'"
+                    );
+                }
+            }
         }
     }
 
@@ -1264,14 +1275,19 @@ public void Gm006_PartialMappingDifferences_Replay_RetainsOnlyUnmappedDifference
     [Property("GoldenMaster", "gm-018")]
     [Property("BehaviorId", "BHV-103")]
     [Property("Invariant", "INV-004")]
-    public void Gm018_MarkerDisplayFormat_Replay_ProducesBackslashPrefixedMarkerItems()
+    public void Gm018_MarkerDisplayFormat_Replay_ProducesMarkerOnRecordOnly()
     {
-        // gm-018 exercises INV-004 (backslash-prefixed marker display) via the
-        // BuildChecklistData pipeline. Same USFM as gm-001 but with
-        // showVerseText=false so the only text item emitted per paragraph is
-        // the backslash-marker name. Expected (per gm-018/expected-output.json):
-        // rowCount=2, excludedCount=0, every paragraph's first content item is
-        // a TextItem whose Text starts with "\".
+        // gm-018 (revised post-UX-2 finding #13): exercises INV-004 (marker
+        // display) via the BuildChecklistData pipeline. Same USFM as gm-001
+        // but with showVerseText=false so the paragraph items list is
+        // emptied. The marker is reported via paragraph.Marker only; the UI
+        // renders the backslash prefix.
+        //
+        // The gm-018 expected-output.json was captured before this revision;
+        // its "items[0].text = \\p / \\q / \\q2" rows reflect the now-defunct
+        // pre-UX-2 contract. The PT9 byte-for-byte fidelity is preserved
+        // semantically (showVerseText=false drops verse text) — only the
+        // PT10 wire shape changed: paragraph.Marker is now authoritative.
         var active = RegisterDummyProject(Gm001ExoUsfm);
         var request = BuildRequest(
             activeProjectId: active.Guid.ToString(),
@@ -1296,39 +1312,39 @@ public void Gm018_MarkerDisplayFormat_Replay_ProducesBackslashPrefixedMarkerItem
             "gm-018 — excludedCount=0 (hideMatches=false)"
         );
 
-        // INV-004: every paragraph's first content item (the marker name item)
-        // must carry the backslash-prefixed marker as its Text. The gm-018
-        // expected-output shows "\\p", "\\q", "\\q2" as the Text value of the
-        // CLText item at position 0 in each paragraph. PostProcessParagraph
-        // prepends this; when showVerseText=false the following text items
-        // are dropped (BHV-103), so the marker item is often the ONLY item.
+        // INV-004 (revised post-UX-2): paragraph.Marker is the single
+        // source of truth for the marker display. With showVerseText=false,
+        // the verse-text items are dropped by PostProcessParagraph; the
+        // only items that remain are downstream additions (e.g. CAP-012's
+        // EditLinkItem appended to the last paragraph of each cell). What
+        // is forbidden is the now-deprecated prepended "\\" + Marker
+        // TextItem (markers-checklist follow-up finding #13).
         foreach (var row in result.Rows)
         {
             foreach (var cell in row.Cells)
             {
                 foreach (var paragraph in cell.Paragraphs)
                 {
                     Assert.That(
-                        paragraph.Items,
-                        Is.Not.Empty,
-                        "INV-004 — every paragraph has at least the marker-name item"
-                    );
-                    Assert.That(
-                        paragraph.Items[0],
-                        Is.InstanceOf<TextItem>(),
-                        $"INV-004 — first item of paragraph '{paragraph.Marker}' must be the marker-name TextItem"
-                    );
-                    var markerItem = (TextItem)paragraph.Items[0];
-                    Assert.That(
-                        markerItem.Text,
-                        Does.StartWith(@"\"),
-                        $"INV-004 — marker-name TextItem must start with '\\' for paragraph '{paragraph.Marker}'"
-                    );
-                    Assert.That(
-                        markerItem.Text,
-                        Is.EqualTo(@"\" + paragraph.Marker),
-                        $"INV-004 — marker-name text is '\\{paragraph.Marker}'"
+                        paragraph.Marker,
+                        Is.Not.Null.And.Not.Empty,
+                        "INV-004 — paragraph.Marker carries the marker name (UI renders the backslash prefix)"
                     );
+                    // No TextItem in Items may carry the deprecated
+                    // "\\" + Marker payload — the UI sources that from
+                    // paragraph.Marker now.
+                    var backslashMarker = "\\" + paragraph.Marker;
+                    foreach (var item in paragraph.Items)
+                    {
+                        if (item is TextItem text)
+                        {
+                            Assert.That(
+                                text.Text,
+                                Is.Not.EqualTo(backslashMarker),
+                                $"INV-004 (revised) — Items must not contain the prepended marker TextItem '{backslashMarker}'"
+                            );
+                        }
+                    }
                 }
             }
         }

c-sharp-tests/Checklists/Markers/MarkersDataSourceTests.cs

@@ -182,10 +182,12 @@ public void ParagraphMarkers_WithEmptyFilter_ReturnsAllParagraphMarkers()
     [Property("ScenarioId", "TS-009")]
     [Property("BehaviorId", "BHV-103")]
     [Property("InvariantId", "INV-004")]
-    public void PostProcessParagraph_ShowVerseTextFalse_ClearsItemsAndInsertsMarkerOnly()
+    public void PostProcessParagraph_ShowVerseTextFalse_DropsAllItems()
     {
-        // BHV-103 / INV-004: with showVerseText=false, existing items are cleared
-        // and a single TextItem("\\" + marker) is inserted at position 0.
+        // BHV-103 / INV-004 (revised post-UX-2): with showVerseText=false, the
+        // paragraph items list is emptied. The marker itself is rendered by the
+        // UI via paragraph.Marker (the marker is no longer prepended as a
+        // TextItem — see markers-checklist follow-up finding #13).
         var input = new ChecklistParagraph(
             "p",
             new List<ChecklistContentItem>
@@ -199,9 +201,7 @@ public void PostProcessParagraph_ShowVerseTextFalse_ClearsItemsAndInsertsMarkerO
 
         Assert.That(result, Is.Not.Null);
         Assert.That(result.Marker, Is.EqualTo("p"));
-        Assert.That(result.Items.Count, Is.EqualTo(1));
-        Assert.That(result.Items[0], Is.InstanceOf<TextItem>());
-        Assert.That(((TextItem)result.Items[0]).Text, Is.EqualTo("\\p"));
+        Assert.That(result.Items, Is.Empty);
     }
 
     [Test]
@@ -211,10 +211,11 @@ public void PostProcessParagraph_ShowVerseTextFalse_ClearsItemsAndInsertsMarkerO
     [Property("ScenarioId", "TS-010")]
     [Property("BehaviorId", "BHV-103")]
     [Property("InvariantId", "INV-004")]
-    public void PostProcessParagraph_ShowVerseTextTrue_PrependsMarkerBeforeText()
+    public void PostProcessParagraph_ShowVerseTextTrue_PreservesOriginalItems()
     {
-        // BHV-103: with showVerseText=true, marker text is inserted at index 0
-        // and the original items are preserved at positions 1..N.
+        // BHV-103 (revised post-UX-2): with showVerseText=true, the original
+        // items are preserved verbatim. The marker is NOT prepended (the UI
+        // renders it from paragraph.Marker).
         var input = new ChecklistParagraph(
             "q2",
             new List<ChecklistContentItem>
@@ -226,11 +227,10 @@ public void PostProcessParagraph_ShowVerseTextTrue_PrependsMarkerBeforeText()
 
         var result = MarkersDataSource.PostProcessParagraph(input, showVerseText: true);
 
-        Assert.That(result.Items.Count, Is.EqualTo(3));
-        Assert.That(result.Items[0], Is.InstanceOf<TextItem>());
-        Assert.That(((TextItem)result.Items[0]).Text, Is.EqualTo("\\q2"));
-        Assert.That(((TextItem)result.Items[1]).Text, Is.EqualTo("indented "));
-        Assert.That(((TextItem)result.Items[2]).Text, Is.EqualTo("poetry"));
+        Assert.That(result.Marker, Is.EqualTo("q2"));
+        Assert.That(result.Items.Count, Is.EqualTo(2));
+        Assert.That(((TextItem)result.Items[0]).Text, Is.EqualTo("indented "));
+        Assert.That(((TextItem)result.Items[1]).Text, Is.EqualTo("poetry"));
     }
 
     [Test]
@@ -239,18 +239,19 @@ public void PostProcessParagraph_ShowVerseTextTrue_PrependsMarkerBeforeText()
     [Property("Contract", "PostProcessParagraph")]
     [Property("ScenarioId", "TS-067")]
     [Property("BehaviorId", "BHV-103")]
-    public void PostProcessParagraph_ShowVerseTextFalse_WithMarkerQ1_DisplaysBackslashQ1Only()
+    public void PostProcessParagraph_ShowVerseTextFalse_WithMarkerQ1_DropsAllItems()
     {
-        // TS-067: q1 marker with showVerseText=false displays exactly "\q1".
+        // TS-067 (revised post-UX-2): q1 marker with showVerseText=false ->
+        // items emptied. UI renders "\q1" from paragraph.Marker.
         var input = new ChecklistParagraph(
             "q1",
             new List<ChecklistContentItem> { new TextItem("some content", null) }
         );
 
         var result = MarkersDataSource.PostProcessParagraph(input, showVerseText: false);
 
-        Assert.That(result.Items.Count, Is.EqualTo(1));
-        Assert.That(((TextItem)result.Items[0]).Text, Is.EqualTo("\\q1"));
+        Assert.That(result.Marker, Is.EqualTo("q1"));
+        Assert.That(result.Items, Is.Empty);
     }
 
     // =====================================================================
@@ -652,18 +653,27 @@ public void InitializeMarkerMappings_EmptyMappingString_ReturnsEmptyDictionary()
     [Property("InvariantId", "INV-008")]
     public void PostProcessRows_EmptyRowsNoFilter_ReturnsIdenticalMarkersMessage()
     {
-        // TS-021 / INV-008: with no rows and no filter active, the service
-        // returns an EmptyResultMessage with variant="identical" and the
-        // paranext-core localize key for the PT9 message. Per the
+        // TS-021 / INV-008: with no rows and no filter active AND at least
+        // one comparative configured, the service returns an
+        // EmptyResultMessage with variant="identical" and the paranext-core
+        // localize key for the PT9 message. Per the
         // patterns.errorHandling.backendLocalization registry entry, the
         // static service returns the KEY; the wrapping ChecklistNetworkObject
         // resolves it via LocalizationService.GetLocalizedString before the
         // wire response is serialized. Maps to PT9 CLParagraphCellsDataSource_1.
+        // Post-UX-2 finding #3: this case is gated on hasComparativeTexts:true
+        // — see PostProcessRows_EmptyRowsNoFilter_NoComparatives_ReturnsNoResults
+        // for the no-comparatives counterpart.
         var emptyRows = new List<ChecklistRow>();
         var emptyFilter = new HashSet<string>();
         var books = new List<string> { "GEN" };
 
-        var result = MarkersDataSource.PostProcessRows(emptyRows, emptyFilter, books);
+        var result = MarkersDataSource.PostProcessRows(
+            emptyRows,
+            emptyFilter,
+            books,
+            hasComparativeTexts: true
+        );
 
         Assert.That(result, Is.Not.Null, "empty results must always produce a message (INV-008)");
         Assert.That(result!.Variant, Is.EqualTo("identical"));
@@ -694,7 +704,12 @@ public void PostProcessRows_EmptyRowsWithFilter_ReturnsNoResultsMessage()
         var filter = new HashSet<string> { "p", "q1" };
         var books = new List<string> { "GEN", "EXO" };
 
-        var result = MarkersDataSource.PostProcessRows(emptyRows, filter, books);
+        var result = MarkersDataSource.PostProcessRows(
+            emptyRows,
+            filter,
+            books,
+            hasComparativeTexts: true
+        );
 
         Assert.That(result, Is.Not.Null);
         Assert.That(result!.Variant, Is.EqualTo("noResults"));
@@ -715,7 +730,12 @@ public void PostProcessRows_EmptyRowsWithFilter_MessageListsSearchedMarkersAndBo
         var filter = new HashSet<string> { "p", "q1" };
         var books = new List<string> { "GEN", "EXO" };
 
-        var result = MarkersDataSource.PostProcessRows(emptyRows, filter, books);
+        var result = MarkersDataSource.PostProcessRows(
+            emptyRows,
+            filter,
+            books,
+            hasComparativeTexts: true
+        );
 
         Assert.That(result, Is.Not.Null);
         Assert.That(result!.SearchedMarkers, Is.Not.Null);
@@ -738,11 +758,68 @@ public void PostProcessRows_NonEmptyRows_ReturnsNull()
         var emptyFilter = new HashSet<string>();
         var books = new List<string> { "GEN" };
 
-        var result = MarkersDataSource.PostProcessRows(rows, emptyFilter, books);
+        var result = MarkersDataSource.PostProcessRows(
+            rows,
+            emptyFilter,
+            books,
+            hasComparativeTexts: true
+        );
 
         Assert.That(result, Is.Null, "non-empty rows must not produce an EmptyResultMessage");
     }
 
+    [Test]
+    [Category("Contract")]
+    [Property("CapabilityId", "CAP-007")]
+    [Property("Contract", "PostProcessRows")]
+    [Property("ScenarioId", "TS-UX2-003")]
+    [Property("BehaviorId", "BHV-106")]
+    public void PostProcessRows_EmptyRowsNoFilter_NoComparatives_ReturnsNoResults()
+    {
+        // UX-2 finding #3: the "Comparative texts have identical markers."
+        // message must NOT fire when there are no comparatives configured.
+        // It only makes sense when at least one comparative is in play.
+        var emptyRows = new List<ChecklistRow>();
+        var emptyFilter = new HashSet<string>();
+        var books = new List<string> { "GEN" };
+
+        var result = MarkersDataSource.PostProcessRows(
+            emptyRows,
+            emptyFilter,
+            books,
+            hasComparativeTexts: false
+        );
+
+        Assert.That(result, Is.Not.Null);
+        Assert.That(result!.Variant, Is.EqualTo(EmptyResultMessageVariant.NoResults));
+    }
+
+    [Test]
+    [Category("Contract")]
+    [Property("CapabilityId", "CAP-007")]
+    [Property("Contract", "PostProcessRows")]
+    [Property("ScenarioId", "TS-UX2-003b")]
+    [Property("BehaviorId", "BHV-106")]
+    public void PostProcessRows_EmptyRowsNoFilter_WithComparatives_ReturnsIdenticalMarkers()
+    {
+        // Regression guard for the original positive case: when comparatives
+        // ARE in play and rows still came back empty with no filter, the
+        // identical-markers message is the correct one.
+        var emptyRows = new List<ChecklistRow>();
+        var emptyFilter = new HashSet<string>();
+        var books = new List<string> { "GEN" };
+
+        var result = MarkersDataSource.PostProcessRows(
+            emptyRows,
+            emptyFilter,
+            books,
+            hasComparativeTexts: true
+        );
+
+        Assert.That(result, Is.Not.Null);
+        Assert.That(result!.Variant, Is.EqualTo(EmptyResultMessageVariant.Identical));
+    }
+
     // =====================================================================
     // BHV-120 / EXT-013 — HeadingMarkers / NonHeadingParagraphMarkers
     // =====================================================================

c-sharp/Checklists/ChecklistService.cs

@@ -168,7 +168,8 @@ public static ChecklistResult BuildChecklistData(ChecklistRequest request, Cance
         EmptyResultMessage? emptyResultMessage = MarkersDataSource.PostProcessRows(
             rows,
             markerFilter,
-            searchedBookNames
+            searchedBookNames,
+            hasComparativeTexts: request.ComparativeTextIds.Count > 0
         );
 
         // Step 10: parallel ColumnHeaders / ColumnProjectIds (INV-C15).