change RichMultiString.cs to store RichStrings instead of just strings#1692
Conversation
ensure that RichString serializes ignoring null properties
…ut editor based off lcm-rich-text-editor.svelte
This test ensures that the diff logic correctly captures property changes, such as toggling text formatting attributes in RichMultiStrings. It validates the generation of precise replace operations when properties are updated.
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThis change implements a comprehensive migration from plain string handling to rich text representations ( Changes
Assessment against linked issues
Poem
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
UI unit Tests10 tests - 2 10 ✅ - 2 0s ⏱️ ±0s Results for commit 766d113. ± Comparison against base commit 404ae07. This pull request removes 2 tests.♻️ This comment has been updated with latest results. |
C# Unit Tests116 tests 116 ✅ 10s ⏱️ Results for commit 766d113. ♻️ This comment has been updated with latest results. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
backend/FwLite/FwDataMiniLcmBridge/Api/RichTextMapping.cs (1)
269-306: 💡 Verification agent🧩 Analysis chain
Duplicate property writes can overwrite the intended variation.
WriteToTextPropsfirst writes allIntProps, then writes a second time forktptFontSize,ktptOffset, andktptLineHeightwith a different variation (ReverseSizeUnit).
The first write usesDefaultVariation, which is hard-coded toktpvMilliPoint; the second write overwrites with the correct variation, but the extra call is unnecessary and obscures intent.- foreach (var propType in IntProps) + // Exclude the three unit-based props here – they are handled below + foreach (var propType in IntProps.Except([ + FwTextPropType.ktptFontSize, + FwTextPropType.ktptOffset, + FwTextPropType.ktptLineHeight]))Alternatively, split
IntPropsinto two readonly sets to make the intent explicit.
To confirm the scope of
IntPropsand the behavior ofDefaultVariation, let’s locate their definitions:
🏁 Script executed:
#!/bin/bash set -e echo "=== IntProps definition ===" rg -n "IntProps" -C 3 echo echo "=== DefaultVariation implementation ===" rg -n "DefaultVariation" -C 5Length of output: 6223
Exclude unit-based IntProps from the initial write loop
The first
foreach (var propType in IntProps)writes all integer props withDefaultVariation, then you immediately overwritektptFontSize,ktptOffset, andktptLineHeightwith the correctReverseSizeUnitvariation. Excluding those three from the initial loop makes the intent explicit and avoids redundant writes.Location to update:
RichTextMapping.cs, methodWriteToTextProps(around line 269)Suggested diff:
- foreach (var propType in IntProps) + // Exclude unit-based props; they’re handled with explicit variation below + var unitBasedProps = new[] { + FwTextPropType.ktptFontSize, + FwTextPropType.ktptOffset, + FwTextPropType.ktptLineHeight + }; + foreach (var propType in IntProps.Except(unitBasedProps))For even clearer intent, you can split
IntPropsinto two readonly sets:private static readonly FrozenSet<FwTextPropType> UnitBasedIntProps = FrozenSet.Create( FwTextPropType.ktptFontSize, FwTextPropType.ktptOffset, FwTextPropType.ktptLineHeight); private static readonly FrozenSet<FwTextPropType> SimpleIntProps = IntProps.Except(UnitBasedIntProps);
🧹 Nitpick comments (12)
frontend/viewer/src/lib/demo-entry-data.ts (3)
95-95: Maintain consistent formatting for thedefinitionobject
The inline object literal is compact but differs from the surrounding code style. Expanding it to multiple lines with proper indentation improves readability and consistency.- 'definition': {'en': {spans: [{text: 'they'}]}, 'pt': {spans: [{text: 'eles'}]}}, + 'definition': { + en: { spans: [{ text: 'they' }] }, + pt: { spans: [{ text: 'eles' }] }, + },
153-153: Maintain consistent formatting for thedefinitionobject
Apply the same multiline, indented style to this second entry for a uniform appearance across demo data.- 'definition': { 'en': {spans: [{text: 'they'}]}, 'pt': {spans: [{text: 'eles'}]} }, + 'definition': { + en: { spans: [{ text: 'they' }] }, + pt: { spans: [{ text: 'eles' }] }, + },
92-156: Reduce duplication by abstracting repeated demo data
Both entries reuse the same plain-text spans, gloss, and part-of-speech data. Consider introducing a helper (e.g.,makeDemoDefinition) to generate thedefinitionobject, which will DRY up the file and simplify future edits.+// Add near the top of the file +function makeDemoDefinition(enText: string, ptText: string): IRichMultiString { + return { + en: { spans: [{ text: enText }] }, + pt: { spans: [{ text: ptText }] }, + }; +} // … { 'id': 'f53f0f28-3ec1-4051-b9a3-fafdca6209ce', 'entryId': '', - 'definition': { - en: { spans: [{ text: 'they' }] }, - pt: { spans: [{ text: 'eles' }] }, - }, + 'definition': makeDemoDefinition('they', 'eles'), 'gloss': { 'en': '3P+2', 'pt': '3P+2' }, // … }, // … { 'id': 'f53f0f29-3ec1-4051-b9a3-fafdca6209ce', 'entryId': '', - 'definition': { - en: { spans: [{ text: 'they' }] }, - pt: { spans: [{ text: 'eles' }] }, - }, + 'definition': makeDemoDefinition('they', 'eles'), 'gloss': { 'en': '3P+2', 'pt': '3P+2' }, // … },backend/FwLite/LcmCrdt.Tests/Data/RegressionDeserializationData.json (2)
47029-47060: New Entry deserialization test data with RichString
The addedEntryobject correctly applies the RichString schema forLiteralMeaning(mapping"en"to a spans array). However, theNotefield is represented as an empty object ({}). To explicitly model an empty RichMultiString and avoid ambiguity in the parser, consider using:"Note": { "en": { "Spans": [] } }
47089-47111: ExampleSentence test data with RichString
TheExampleSentencecorrectly uses RichString forSentence. Similar toNote, theTranslationfield is an empty object. For consistency, you may want to represent it explicitly as:"Translation": { "en": { "Spans": [] } }This makes empty RichMultiStrings uniform across test data.
frontend/viewer/src/lib/writing-system-service.svelte.ts (1)
158-161: Enhance asString method for better robustnessThe
asStringmethod correctly handles string conversion for rich text objects by concatenating span text content. However, it could be enhanced to be more robust.Consider these improvements:
public asString(value: IRichString | string | undefined): string | undefined { - if (!value || typeof value === 'string') return value; + if (value === null || value === undefined) return undefined; + if (typeof value === 'string') return value; + if (!Array.isArray(value.spans) || value.spans.length === 0) return ''; return value.spans.map(s => s.text).join(''); }This improves null safety and handles the case of an empty spans array.
backend/FwLite/FwDataMiniLcmBridge/Api/RichTextMapping.cs (2)
128-143: Add defensive‐null checks fortsStringandwsIdLookup.
FromTsStringassumes both parameters are non-null and thattsString.RunCount >= 0. A single call site providingnull(e.g. from malformed test data) will throw a cryptic NRE. A lightweight guard at the very top keeps the method fail-fast and self-documenting.+ if (tsString is null) + throw new ArgumentNullException(nameof(tsString)); + if (wsIdLookup is null) + throw new ArgumentNullException(nameof(wsIdLookup));
251-263: Possible performance issue: one props builder per span causes quadratic allocations.
propsBldris cleared in every loop iteration, which is good, butTsStringUtils.MakeStringinternally clones the entire props object each time. For large strings with many runs this becomes an O(n²) copy.
Consider caching identicalITsTextPropsobjects (e.g. with aDictionary<Hash, ITsTextProps>) or building the whole string in one pass withstringBuilder.SetIntPropValues/SetStrPropValuebefore appending text, thereby avoiding repeated prop cloning.backend/FwLite/MiniLcm/Models/RichMultiString.cs (1)
42-56:IDictionary.Adderror message mentions “string” but now expectsRichString.The exception text still says “to string”, which is misleading after the migration.
- throw new ArgumentException($"unable to convert value {value?.GetType().Name ?? "null"} to string", + throw new ArgumentException($"unable to convert value {value?.GetType().Name ?? "null"} to RichString",backend/FwLite/MiniLcm.Tests/RichMultiStringTests.cs (3)
136-158: Consider adding test for composite equality scenarios.The RichSpan equality tests cover individual property differences well, but consider adding a test for when multiple properties differ simultaneously to ensure the equality comparison isn't short-circuiting incorrectly.
+ [Fact] + public void RichSpanEquality_FalseWhenMultiplePropertiesDiffer() + { + var span = new RichSpan() { Text = "test1", Bold = RichTextToggle.On, Italic = RichTextToggle.Off }; + var spanCopy = new RichSpan() { Text = "test2", Bold = RichTextToggle.Off, Italic = RichTextToggle.On }; + span.Equals(spanCopy).Should().BeFalse(); + }
1-159: Consider adding tests for multi-span RichString objects.The current tests verify behavior for RichString objects with a single span, but there don't appear to be tests for RichString objects containing multiple spans. Consider adding tests that verify serialization, deserialization, and equality for multi-span scenarios.
+ [Fact] + public void RichMultiString_DeserializesMultiSpanRichString() + { + //lang=json + var json = """{"en":{"Spans":[{"Text":"first "},{"Text":"second","Bold":"On"}]}}"""; + var expectedMs = new RichMultiString() { { "en", new RichString([ + new RichSpan { Text = "first " }, + new RichSpan { Text = "second", Bold = RichTextToggle.On } + ]) } }; + var actualMs = JsonSerializer.Deserialize<RichMultiString>(json); + actualMs.Should().NotBeNull(); + actualMs.Should().ContainKey("en"); + actualMs.Should().BeEquivalentTo(expectedMs); + }
1-159: Consider adding null handling tests.The test suite doesn't appear to include tests for null handling in RichString or RichSpan objects. Consider adding tests to verify behavior when null values are encountered in various scenarios (null Text property, null Spans array, etc.).
+ [Fact] + public void RichString_HandlesNullText() + { + var span = new RichSpan { Text = null, Bold = RichTextToggle.On }; + var richString = new RichString([span]); + + // Test serialization doesn't throw + var json = JsonSerializer.Serialize(richString); + json.Should().NotBeNull(); + + // Test deserialization works correctly + var deserialized = JsonSerializer.Deserialize<RichString>(json); + deserialized.Should().BeEquivalentTo(richString); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (43)
backend/FwLite/FwDataMiniLcmBridge.Tests/MiniLcmTests/UpdateEntryTests.cs(2 hunks)backend/FwLite/FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.cs(2 hunks)backend/FwLite/FwDataMiniLcmBridge/Api/RichTextMapping.cs(2 hunks)backend/FwLite/FwDataMiniLcmBridge/Api/UpdateProxy/UpdateDictionaryProxy.cs(1 hunks)backend/FwLite/FwDataMiniLcmBridge/Api/UpdateProxy/UpdateEntryProxy.cs(1 hunks)backend/FwLite/FwLiteProjectSync.Tests/MultiStringDiffTests.cs(1 hunks)backend/FwLite/FwLiteProjectSync.Tests/SyncTests.cs(3 hunks)backend/FwLite/FwLiteShared/TypeGen/ReinforcedFwLiteTypingConfig.cs(2 hunks)backend/FwLite/FwLiteWeb/Routes/TestRoutes.cs(1 hunks)backend/FwLite/LcmCrdt.Tests/Changes/UseChangesTests.cs(1 hunks)backend/FwLite/LcmCrdt.Tests/Data/RegressionDeserializationData.json(1 hunks)backend/FwLite/LcmCrdt/CrdtProjectsService.cs(2 hunks)backend/FwLite/MiniLcm.Tests/AutoFakerHelpers/MultiStringOverride.cs(1 hunks)backend/FwLite/MiniLcm.Tests/BasicApiTestsBase.cs(15 hunks)backend/FwLite/MiniLcm.Tests/QueryEntryTestsBase.cs(4 hunks)backend/FwLite/MiniLcm.Tests/RichMultiStringTests.cs(3 hunks)backend/FwLite/MiniLcm.Tests/SerializationTests.cs(1 hunks)backend/FwLite/MiniLcm.Tests/UpdateEntryTestsBase.cs(3 hunks)backend/FwLite/MiniLcm.Tests/Validators/EntryValidatorTests.cs(1 hunks)backend/FwLite/MiniLcm.Tests/Validators/ExampleSentenceValidatorTests.cs(2 hunks)backend/FwLite/MiniLcm.Tests/Validators/SenseValidatorTests.cs(1 hunks)backend/FwLite/MiniLcm/Models/RichMultiString.cs(4 hunks)backend/FwLite/MiniLcm/Models/RichString.cs(3 hunks)backend/FwLite/MiniLcm/SyncHelpers/MultiStringDiff.cs(1 hunks)backend/FwLite/MiniLcm/Validators/MultiStringValidator.cs(1 hunks)backend/LfClassicData/LfClassicMiniLcmApi.cs(1 hunks)frontend/viewer/src/ShadcnProjectView.svelte(2 hunks)frontend/viewer/src/lib/DictionaryEntry.svelte(1 hunks)frontend/viewer/src/lib/activity/ActivityView.svelte(5 hunks)frontend/viewer/src/lib/components/field-editors/index.ts(1 hunks)frontend/viewer/src/lib/components/field-editors/rich-multi-ws-input.svelte(1 hunks)frontend/viewer/src/lib/components/lcm-rich-text-editor/lcm-rich-text-editor.svelte(4 hunks)frontend/viewer/src/lib/config-data.ts(1 hunks)frontend/viewer/src/lib/config-types.ts(3 hunks)frontend/viewer/src/lib/demo-entry-data.ts(2 hunks)frontend/viewer/src/lib/dotnet-types/generated-types/MiniLcm/Models/IRichTextObjectData.ts(0 hunks)frontend/viewer/src/lib/dotnet-types/i-multi-string.ts(1 hunks)frontend/viewer/src/lib/entry-editor/object-editors/EntryEditorPrimitive.svelte(3 hunks)frontend/viewer/src/lib/entry-editor/object-editors/ExampleEditorPrimitive.svelte(3 hunks)frontend/viewer/src/lib/entry-editor/object-editors/SenseEditorPrimitive.svelte(2 hunks)frontend/viewer/src/lib/sandbox/EditorSandbox.svelte(4 hunks)frontend/viewer/src/lib/writing-system-service.svelte.ts(2 hunks)frontend/viewer/src/project/ProjectSidebar.svelte(1 hunks)
💤 Files with no reviewable changes (1)
- frontend/viewer/src/lib/dotnet-types/generated-types/MiniLcm/Models/IRichTextObjectData.ts
🧰 Additional context used
🧬 Code Graph Analysis (13)
backend/FwLite/FwLiteProjectSync.Tests/SyncTests.cs (3)
backend/FwLite/MiniLcm/Models/RichString.cs (3)
RichString(9-11)RichString(21-31)RichString(39-43)backend/FwLite/MiniLcm/Models/Sense.cs (2)
Sense(7-54)Sense(38-53)backend/FwLite/MiniLcm/Models/ExampleSentence.cs (2)
ExampleSentence(5-42)ExampleSentence(29-41)
backend/LfClassicData/LfClassicMiniLcmApi.cs (1)
backend/FwLite/MiniLcm/Models/RichString.cs (3)
RichString(9-11)RichString(21-31)RichString(39-43)
backend/FwLite/MiniLcm.Tests/SerializationTests.cs (2)
backend/FwLite/MiniLcm/Models/RichString.cs (3)
RichString(9-11)RichString(21-31)RichString(39-43)backend/FwLite/FwDataMiniLcmBridge/Api/RichTextMapping.cs (1)
RichString(128-143)
backend/FwLite/MiniLcm.Tests/Validators/EntryValidatorTests.cs (3)
backend/FwLite/MiniLcm/Models/RichMultiString.cs (5)
RichMultiString(11-13)RichMultiString(15-17)RichMultiString(19-21)RichMultiString(29-38)RichMultiString(62-74)backend/LfClassicData/LfClassicMiniLcmApi.cs (1)
RichMultiString(326-336)backend/FwLite/MiniLcm/Models/RichString.cs (3)
RichString(9-11)RichString(21-31)RichString(39-43)
backend/FwLite/MiniLcm.Tests/QueryEntryTestsBase.cs (4)
backend/FwLite/MiniLcm/Models/RichString.cs (3)
RichString(9-11)RichString(21-31)RichString(39-43)backend/FwLite/FwDataMiniLcmBridge/Api/RichTextMapping.cs (1)
RichString(128-143)backend/FwLite/FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.cs (1)
ExampleSentence(649-660)backend/LfClassicData/LfClassicMiniLcmApi.cs (1)
ExampleSentence(302-312)
backend/FwLite/MiniLcm.Tests/AutoFakerHelpers/MultiStringOverride.cs (2)
backend/FwLite/MiniLcm/Models/RichString.cs (3)
RichString(9-11)RichString(21-31)RichString(39-43)backend/FwLite/FwDataMiniLcmBridge/Api/RichTextMapping.cs (1)
RichString(128-143)
backend/FwLite/LcmCrdt/CrdtProjectsService.cs (2)
backend/FwLite/MiniLcm/Models/RichString.cs (3)
RichString(9-11)RichString(21-31)RichString(39-43)backend/FwLite/FwDataMiniLcmBridge/Api/RichTextMapping.cs (1)
RichString(128-143)
frontend/viewer/src/lib/dotnet-types/i-multi-string.ts (1)
frontend/viewer/src/lib/dotnet-types/generated-types/MiniLcm/Models/IRichString.ts (1)
IRichString(8-11)
frontend/viewer/src/lib/config-types.ts (2)
backend/FwLite/FwLiteShared/TypeGen/ReinforcedFwLiteTypingConfig.cs (1)
T(178-182)frontend/viewer/src/lib/dotnet-types/i-multi-string.ts (1)
IRichMultiString(8-10)
backend/FwLite/MiniLcm.Tests/Validators/SenseValidatorTests.cs (3)
backend/FwLite/MiniLcm/Models/RichMultiString.cs (5)
RichMultiString(11-13)RichMultiString(15-17)RichMultiString(19-21)RichMultiString(29-38)RichMultiString(62-74)backend/LfClassicData/LfClassicMiniLcmApi.cs (1)
RichMultiString(326-336)backend/FwLite/MiniLcm/Models/RichString.cs (3)
RichString(9-11)RichString(21-31)RichString(39-43)
frontend/viewer/src/lib/writing-system-service.svelte.ts (3)
frontend/viewer/src/lib/dotnet-types/i-multi-string.ts (2)
IMultiString(3-5)IRichMultiString(8-10)frontend/viewer/src/lib/utils.ts (1)
firstTruthy(14-20)frontend/viewer/src/lib/dotnet-types/generated-types/MiniLcm/Models/IRichString.ts (1)
IRichString(8-11)
backend/FwLite/FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.cs (3)
backend/FwLite/FwDataMiniLcmBridge/Api/UpdateProxy/UpdateDictionaryProxy.cs (4)
Add(11-14)Add(16-20)Add(22-26)Add(35-50)backend/FwLite/FwDataMiniLcmBridge/Api/RichTextMapping.cs (1)
RichTextMapping(12-650)backend/FwLite/FwDataMiniLcmBridge/Api/LcmHelpers.cs (1)
GetWritingSystemHandle(80-113)
backend/FwLite/FwDataMiniLcmBridge/Api/RichTextMapping.cs (3)
backend/FwLite/MiniLcm/Models/RichString.cs (3)
RichString(9-11)RichString(21-31)RichString(39-43)backend/FwLite/FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.cs (1)
WritingSystemId(72-75)backend/FwLite/FwDataMiniLcmBridge.Tests/RichTextTests.cs (1)
WritingSystemId(529-536)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Build FwHeadless / publish-fw-headless
- GitHub Check: Build API / publish-api
- GitHub Check: Build UI / publish-ui
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: check-and-lint
- GitHub Check: Analyze (csharp)
- GitHub Check: Build FW Lite and run tests
🔇 Additional comments (91)
frontend/viewer/src/ShadcnProjectView.svelte (2)
23-23: LGTM: Import statement for ActivityView added correctlyThe import statement for the ActivityView component is correctly added to support the new activity route.
57-59: LGTM: Activity route integrationThe new route for
/activityis properly integrated into the existing routing structure, rendering the ActivityView component with theopenprop set. This matches the PR objective of reintroducing the activity page for debugging purposes.frontend/viewer/src/project/ProjectSidebar.svelte (1)
77-77: LGTM: Activity button moved outside development modeThe Activity view button has been appropriately moved outside the DevContent wrapper, making it accessible in production mode. This change correctly aligns with the addition of the activity route in ShadcnProjectView.
frontend/viewer/src/lib/activity/ActivityView.svelte (5)
13-13: LGTM: Project context import addedThe import for useProjectContext is correctly added to support obtaining the project code internally rather than through props.
20-20: LGTM: Project context initializationProject context is properly initialized to access the project code internally.
33-36: LGTM: Reactive loading logic simplifiedThe reactive loading logic now depends solely on the
openstate, which is cleaner and aligns with the component's usage as a routed view rather than a dialog.
42-42: LGTM: Project code retrieval updatedThe activity data loading correctly uses the project code from the context instead of relying on a passed prop.
76-144: LGTM: Component structure simplifiedThe component structure has been simplified by removing the dialog wrapper elements. The conditional rendering based on the loading state is preserved, and the overall layout and functionality remain intact.
The formatting of the change list display has been slightly improved with better spacing and border handling.
backend/FwLite/MiniLcm/SyncHelpers/MultiStringDiff.cs (1)
43-43: Good insight for future optimization!This comment correctly identifies a potential improvement for generating more granular diffs of rich text. Instead of treating RichString objects as atomic units, checking each property individually would allow for more precise operations that update only what has changed.
frontend/viewer/src/lib/entry-editor/object-editors/SenseEditorPrimitive.svelte (2)
10-10: LGTM - Import added for new componentThe import statement correctly adds the RichMultiWsInput component while maintaining the existing imports.
51-51: Migration to rich text input implemented correctlyThe component has been properly upgraded from MultiWsInput to RichMultiWsInput for the definition field, maintaining all existing bindings and properties. This change aligns with the PR objective to migrate editors to use the new RichString class.
backend/FwLite/MiniLcm.Tests/AutoFakerHelpers/MultiStringOverride.cs (1)
41-41: Test data generation properly updated for RichStringThe code correctly wraps the generated word in a new RichString object, aligning with the updated model where RichMultiString now stores RichString objects instead of plain strings.
frontend/viewer/src/lib/entry-editor/object-editors/ExampleEditorPrimitive.svelte (3)
10-10: LGTM - Import added for new componentThe import statement correctly adds the RichMultiWsInput component while maintaining the existing imports.
36-37: Migration to rich text input implemented correctlyThe sentence field has been properly upgraded from MultiWsInput to RichMultiWsInput while maintaining all existing bindings and properties. This change aligns with the PR objective to migrate editors to use the new RichString class.
47-48: Migration to rich text input implemented correctlyThe translation field has been properly upgraded from MultiWsInput to RichMultiWsInput while maintaining all existing bindings and properties. This supports consistent rich text editing throughout the application.
backend/LfClassicData/LfClassicMiniLcmApi.cs (1)
332-332: Implementation correctly updated for rich text representationThe change properly wraps each string value in a
RichStringobject instead of storing it directly. This aligns with the PR's goal of migrating from plain strings to rich text representations.frontend/viewer/src/lib/entry-editor/object-editors/EntryEditorPrimitive.svelte (3)
10-10: Import updated to include RichMultiWsInputThe import statement now includes the
RichMultiWsInputcomponent which is needed for the rich text fields.
102-107: literalMeaning field updated to use rich text inputThe component has been correctly changed from
MultiWsInputtoRichMultiWsInputto support rich text editing for the literal meaning field.
113-118: note field updated to use rich text inputThe component has been correctly changed from
MultiWsInputtoRichMultiWsInputto support rich text editing for the note field.backend/FwLite/MiniLcm.Tests/Validators/SenseValidatorTests.cs (1)
76-76: Test helper updated to use RichString wrapperThe test utility method now correctly wraps the content string in a
RichStringobject when setting aRichMultiStringproperty, ensuring test data matches the updated model expectations.frontend/viewer/src/lib/sandbox/EditorSandbox.svelte (4)
74-76: Added onChange handler function for tracking field changesThis function will help with debugging by logging which fields have been changed.
99-102: Added onChange callback to word fieldThe word field now correctly notifies when changes occur, which will help with tracking modifications during development.
108-112: Added onChange callback to reference fieldThe reference field now correctly notifies when changes occur, which will help with tracking modifications during development.
156-156: Added onChange callback to note fieldThe note field (using LcmRichTextEditor) now correctly notifies when changes occur, which will help with tracking modifications during development.
frontend/viewer/src/lib/dotnet-types/i-multi-string.ts (2)
1-1: Good addition of the IRichString import.This import is necessary for the type changes in the IRichMultiString interface.
8-10: Appropriate update to the IRichMultiString interface.You've correctly updated the value type from string to IRichString, which aligns with the PR objective of storing RichString objects instead of plain strings. This change is consistent with the backend modifications to RichMultiString.cs.
backend/FwLite/FwLiteShared/TypeGen/ReinforcedFwLiteTypingConfig.cs (2)
2-2: Good addition of the JsonSerializer import.This import is required for the JsonIgnoreAttribute reference used in the updated property filter.
76-76: Enhanced property filtering for TypeScript export.Good improvement by also excluding properties with
[JsonIgnore(Condition = JsonIgnoreCondition.Always)]from TypeScript export generation. This ensures consistency between JSON serialization behavior and TypeScript type definitions, particularly important for the new rich text data structures.frontend/viewer/src/lib/components/field-editors/index.ts (2)
5-5: Good import of the new RichMultiWsInput component.This import makes the rich text input component available for use throughout the application.
7-7: Correctly updated exports to include RichMultiWsInput.The export list now includes the new rich multi-writing-system input component, making it accessible to other parts of the frontend.
backend/FwLite/MiniLcm.Tests/UpdateEntryTestsBase.cs (2)
19-19: Properly updated test data to use RichString.Good job migrating the test data from plain strings to RichString objects in the InitializeAsync method. This aligns with the backend model changes and ensures that tests properly validate the new rich text functionality.
Also applies to: 21-21, 27-27, 30-30, 44-44
178-179: Correctly updated test case to use RichString objects.The ExampleSentence test data now uses RichString objects instead of plain strings, maintaining consistency with the rich text model changes throughout the codebase.
backend/FwLite/MiniLcm.Tests/Validators/EntryValidatorTests.cs (1)
150-150: Updated test helper to use RichString instead of plain stringsThe change correctly updates the
SetPropertymethod to create aRichMultiStringwith aRichStringvalue instead of a plain string. This aligns with the model migration to use rich text representations.backend/FwLite/FwLiteProjectSync.Tests/SyncTests.cs (5)
21-21: Updated Note property to use RichStringThis change correctly wraps the plain string in a RichString object, aligning with the updated RichMultiString model that now stores RichString objects instead of plain strings.
27-27: Updated Definition property to use RichStringThe Definition property is now correctly using a RichString object instead of a plain string, which aligns with the updated Sense model.
30-30: Updated Sentence property to use RichStringExample sentence now correctly uses a RichString to store the text content, maintaining consistency with the rich text model updates.
456-456: Updated Definition property in new entry test to use RichStringDefinition property in the complex form entry test is now correctly using a RichString object.
487-487: Updated Definition properties in sense tests to use RichStringDefinition properties in both senses added in the AddingASenseToAnEntryInEachProjectSyncsAcrossBoth test now correctly use RichString objects.
Also applies to: 492-492
backend/FwLite/LcmCrdt.Tests/Changes/UseChangesTests.cs (1)
105-105: Updated ExampleSentence initialization to use RichStringThis change correctly wraps the test sentence text in a RichString object when initializing the ExampleSentence's Sentence property, aligning with the updated model.
backend/FwLite/FwLiteWeb/Routes/TestRoutes.cs (1)
33-33: Updated set-entry-note endpoint to use RichStringThe API endpoint now correctly wraps the note text in a RichString object when updating an entry's Note property, ensuring consistency with the updated data model.
backend/FwLite/MiniLcm.Tests/SerializationTests.cs (1)
55-55: LGTM. The change properly updates the test to use RichString.This change correctly implements the migration from plain string to RichString for the Sentence field in ExampleSentence. The test now properly verifies serialization and deserialization of RichString objects.
backend/FwLite/FwDataMiniLcmBridge/Api/UpdateProxy/UpdateEntryProxy.cs (2)
87-87: Improved performance by initializing field once instead of creating new instances.Converting this from a property that created a new instance on each access to a field initialized once in the constructor is a good optimization.
91-92: LGTM. Correctly updated to support RichString values.This change properly implements the migration from plain string to RichString for dictionary values. The error message on line 92 provides clear context when an incompatible value type is provided.
frontend/viewer/src/lib/config-data.ts (3)
16-17: LGTM. Updated entry fields to support rich text.Correctly changed the field types from 'multi' to 'rich-multi' for literalMeaning and note fields to support rich text formatting.
24-24: LGTM. Updated definition field to support rich text.Correctly changed the field type from 'multi' to 'rich-multi' for the definition field to support rich text formatting.
32-33: LGTM. Updated example fields to support rich text.Correctly changed the field types from 'multi' to 'rich-multi' for sentence and translation fields to support rich text formatting.
backend/FwLite/FwDataMiniLcmBridge/Api/UpdateProxy/UpdateDictionaryProxy.cs (1)
22-26: LGTM. Well-implemented Add method overload for RichString.This new overload properly handles adding RichString values to the dictionary. The implementation correctly converts RichString to TsString using the RichTextMapping utility and the appropriate writing system handle.
backend/FwLite/LcmCrdt/CrdtProjectsService.cs (3)
257-257: Consistent replacement of string with RichStringThe update to use
RichStringfor theLiteralMeaningproperty is aligned with the overall migration from plain strings to rich text representation. This change properly implements the new model.
267-268: Appropriate use of RichString for DefinitionThe replacement of plain string with
RichStringfor the definition text is correctly implemented, allowing for rich text formatting capabilities that weren't possible with the previous string implementation.
271-271: Properly migrated ExampleSentences to use RichStringThe example sentence creation now uses
RichStringobjects instead of plain strings, consistent with the overall migration pattern. All text-based fields in the sample data are now using the rich text representation.backend/FwLite/MiniLcm.Tests/Validators/ExampleSentenceValidatorTests.cs (2)
13-13: Updated test initialization with RichStringGood update to the test case to use the new
RichStringmodel. This ensures the validator tests reflect the current model implementation.
57-57: Properly updated SetProperty helper methodThe helper method now correctly wraps content in a
RichStringobject when creating aRichMultiString. This ensures consistency with the model changes throughout the test suite.backend/FwLite/MiniLcm.Tests/BasicApiTestsBase.cs (7)
17-18: Updated Note and LiteralMeaning to use RichString in test dataThe test initialization has been properly updated to use
RichStringobjects for rich text fields.Also applies to: 22-23
31-32: Migrated Definition and Sentence properties to RichStringThe test data for both Definition and example Sentence fields now correctly use
RichStringobjects.Also applies to: 39-40
57-58: Updated Definition property in second test entryThe second test entry's Definition property has been properly migrated to use
RichString.
154-155: Properly migrated text fields in CreateEntry testAll text fields in the CreateEntry test now use
RichStringobjects, maintaining consistency with the model changes.Also applies to: 159-160, 168-169, 176-177
185-187: Updated assertion methods to use BeEquivalentToThe assertion methods have been correctly updated to use
BeEquivalentTo()instead of direct equality checks. This is appropriate for comparingRichStringobjects, which should be compared by content rather than by reference.Also applies to: 190-192
214-215: Updated UpdateEntry tests to use RichStringThe UpdateEntry test methods now correctly use and assert
RichStringobjects.Also applies to: 228-229, 235-236
309-310: Consistently migrated remaining test methods to use RichStringAll remaining test methods have been updated to use
RichStringobjects consistently, including tests for sense and example sentence updates.Also applies to: 340-341, 370-371, 394-395, 402-403, 414-415, 430-431, 438-439, 442-443, 454-455
backend/FwLite/FwDataMiniLcmBridge.Tests/MiniLcmTests/UpdateEntryTests.cs (2)
55-57: Updated test entries to use RichStringThe test data creation has been properly updated to use
RichStringobjects for all text fields including Note, LiteralMeaning, Definition, and Sentence.Also applies to: 63-66
89-90: Updated Translation field and assertionsThe Example Translation and its assertion have been correctly updated to use and verify
RichStringobjects. The use ofBeEquivalentTo()is appropriate for comparingRichStringcontent.Also applies to: 97-97
backend/FwLite/MiniLcm/Validators/MultiStringValidator.cs (1)
29-30: Good migration to RichString's domain-specific APIThe validator now correctly uses the
IsEmptyproperty fromRichStringinstead of the string utility methods. This change is consistent with the PR's objective of migrating from plain strings to rich text representations.Two observations:
- Access pattern changed from
ms.Values.Alltoms.All, indicatingRichMultiStringis now directly enumerable- Validation logic consistently uses
IsEmptyin both condition and error messagebackend/FwLite/LcmCrdt.Tests/Data/RegressionDeserializationData.json (1)
47061-47088: Sense test data covers RichStringDefinition
TheSenseobject correctly uses the RichString format forDefinition. Please verify thatGloss—currently a plain string under"en"—matches your model’s expected type. IfGlosshas also been migrated to RichString, adjust it to the spans-based schema; otherwise, this plain-mapping is fine.backend/FwLite/MiniLcm.Tests/QueryEntryTestsBase.cs (2)
28-28: Migration to RichString implementation looks goodThe conversion of plain strings to RichString objects is consistent and correctly applied to all Definition and Sentence properties in the test data.
Also applies to: 40-40, 47-47, 51-51, 71-71, 78-78
208-213:Details
❓ Verification inconclusive
Don't skip tests - update filtering to support RichString instead
Rather than skipping this test, consider implementing a solution that enables filtering on RichString content. The filtering functionality is important and should be updated to work with the new data model.
You could update the
CanFilterExampleSentenceTexttest by modifying the filtering mechanism to search within the text content of RichString objects. For example:
🏁 Script executed:
#!/bin/bash # Let's find related filtering implementations to understand how to fix this rg -A 10 -B 10 "Filter.*Sentence" --type csharpLength of output: 3776
We need to inspect how
GridifyFilteris parsed and applied to nestedRichStringfields before un-skipping the test. Let's locate the filter parser and its support forExampleSentences.Sentence.frontend/viewer/src/lib/config-types.ts (3)
4-4: LGTM: Added IRichMultiString importThe import of IRichMultiString is properly added to support rich text field types.
13-13: LGTM: Added rich-multi field typeThe field type enumeration is correctly extended to include the new rich text field type.
40-43: LGTM: Type definition for rich-multi fieldsThe type definition for rich-multi fields is properly structured to match the pattern of existing field types, enforcing that fields of type 'rich-multi' must have values of type IRichMultiString.
frontend/viewer/src/lib/DictionaryEntry.svelte (1)
56-56: LGTM: Properly converts rich strings to plain text for renderingThe component now uses
wsService.asString()to convert rich string objects to plain text before rendering. This is necessary since the data model has changed to use rich strings instead of plain strings.Also applies to: 64-64, 68-68
frontend/viewer/src/lib/writing-system-service.svelte.ts (2)
1-14: LGTM: Added necessary imports for rich text typesThe import statements have been properly updated to include the rich text types needed for the implementation.
154-156: LGTM: Updated first method to handle rich stringsThe
firstmethod has been properly updated to handle both regular multi-strings and rich multi-strings, and correctly uses the newasStringmethod to extract plain text content.frontend/viewer/src/lib/components/lcm-rich-text-editor/lcm-rich-text-editor.svelte (6)
51-58: New change event callback and updated value type support undefinedThe component now includes an optional
onchangecallback and allows thevalueprop to be undefined. This is a good pattern for flexible component reuse, especially when handling optional data.
61-61: Added dirty tracking for editor changesThe new
dirtystate flag improves efficiency by tracking when content has changed since the last notification, preventing unnecessary callbacks.
70-79: Improved change detection and value updating logicThe refactored
dispatchTransactionhandler now only updates the internal state when actual content changes occur (usingdoc.eqcomparison). This optimizes performance and ensures the dirty flag is only set when necessary.
85-87: Added blur handler to notify of changesThe new blur event handler properly implements the editor's change notification pattern by only triggering the
onchangecallback when content has actually changed (dirty flag is true) and then resetting the dirty state.Also applies to: 92-97
194-196: Fixed potential undefined value handlingThe
valueToDocfunction now safely handles potentially undefined values using optional chaining and a fallback empty array, preventing runtime errors when no initial value is provided.
211-212: Improved cursor styling for better user experienceSetting the cursor to
textinside the ProseMirror editor provides clearer visual feedback to users about the editable area.backend/FwLite/FwLiteProjectSync.Tests/MultiStringDiffTests.cs (2)
86-102: Good test coverage for rich text property changesThis test effectively verifies that property changes within rich text spans (specifically toggling Bold from On to Off) are correctly detected and result in a "replace" operation in the diff output.
104-117: Comprehensive test for identical rich text handlingThis test confirms that identical rich text spans (including their properties) result in no diff operations, which is important for efficiency and correctness in the synchronization system.
backend/FwLite/FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.cs (2)
682-686: Improved rich text conversion with writing system mappingThe updated
FromLcmMultiStringmethod now properly convertsITsStringtoRichStringusingRichTextMapping.FromTsString, preserving formatting and writing system information. This is a crucial improvement for the rich text migration.
1071-1071: Enhanced rich text serialization with writing system handlingThe
UpdateLcmMultiStringmethod now usesRichTextMapping.ToTsStringwith a proper writing system handle conversion delegate, ensuring formatting is preserved when updating the LCM multi-string.frontend/viewer/src/lib/components/field-editors/rich-multi-ws-input.svelte (2)
1-25: Well-structured component with clear type definitionsThe component follows best practices with explicit TypeScript type definitions for props and uses modern Svelte patterns (
$bindable,$props,$derived) for state management. The interface properly supports rich text editing across multiple writing systems.
27-36: Efficient and accessible UI layout for multi-writing-system editingThe component creates a responsive grid layout and properly passes the required props to each
LcmRichTextEditorinstance. The implementation includes:
- Clear writing system labels with abbreviations
- Title attributes for accessibility with full writing system information
- Proper binding of values by writing system ID
- Event delegation that includes context (writing system ID) in change notifications
This is a well-constructed UI component that should work effectively across various screen sizes.
backend/FwLite/MiniLcm.Tests/RichMultiStringTests.cs (8)
3-3: Added necessary import for operation-level JSON patching.The addition of the
SystemTextJsonPatch.Operationsimport supports the new test on line 109 where you directly create an Operation object to test string-to-RichString compatibility.
9-19: Clean implementation of RichString deserialization test.This test correctly verifies that a simple RichString with just text content can be properly deserialized from JSON. The expected object structure with
RichMultiStringcontaining aRichStringmatches the migration from plain strings to rich text.
20-34: Comprehensive test for styled rich text deserialization.This test properly validates that style information (Bold attribute) is correctly preserved during deserialization. The test structure follows good practices by clearly separating the setup, execution, and verification phases.
36-46: Backward compatibility test for plain string deserialization.Good test for ensuring that JSON containing plain strings can still be deserialized into the new
RichStringformat. This maintains compatibility with existing data.
69-76: Well-structured serialization test for simple rich text.This test verifies the reverse operation - that a
RichMultiStringwith a simpleRichStringserializes back to the expected JSON format with spans structure.
92-101: Updated patch test for the new RichString model.The test correctly validates that JSON Patch operations work with the new
RichStringobjects, ensuring that patching functionality continues to work after the model change.
103-113: Good test for backward compatibility with string-based patches.This test is particularly valuable as it emulates handling existing data and patches that use plain strings rather than
RichStringobjects, ensuring the system can still process legacy data formats correctly.
118-118: Consistent update to use RichString in patch tests.All JSON Patch operation tests have been consistently updated to use
RichStringobjects instead of plain strings, maintaining test integrity with the model changes.Also applies to: 128-128, 133-133
There was a problem hiding this comment.
I managed to break a project. I'm not sure how at the moment, but...I guess it's probably obvious: we probably need to always add a ws prop to runs we create.
I found a lot of quircks with our rich-text field at least some of which we probably want to fix before we ship this (not necessarily before we merge it):
#1699
I also managed to break graphite. I wrote and submitted this now not 21 hours ago: 😆
|
I should mention that I played with this a decent amount just locally in FWL with a fwdata project and it seemed to work well other than the issues I noted. |
Co-authored-by: Tim Haasdyk <tim_haasdyk@sil.org>
|
@myieye I've fixed all your comments. As for assigning a writing system. Where do you think we should do that? Is that the responsibility of the editor? Then we just add API validation to require all RichStrings to have a non default WS (to protect API users). I don't think we want to fix that up when writing changes to FW because that change will be a desync as it would get written back on sync. Alterantivly we could just fix any RichString on it's way in and if there's no writing system assign it one based on it's location? |
# Conflicts: # frontend/viewer/src/lib/entry-editor/object-editors/ExampleEditorPrimitive.svelte
|
Yeah, API validation is the most crucial piece here. Regarding fixing things up automatically in the API, that sounds like a lot more code than validating...though I suppose we could do it in the same way as we validate, but we don't have that set up atm. |
… members and no their equals method to improve assert messages
…his avoids issues when typing in a field which was undefined
myieye
left a comment
myieye
left a comment
There was a problem hiding this comment.
I must admit that my attention to detail has diminished since I started reviewing this PR, but it looks good and seems to work.
…node.attr.richSpan.text was out of date
…le span in one block
2 participants
closes #1307
migrates our model and editors to use the new RichString class.
Local testing shows no conflicts when doing a hg to harmony sync, and the changes to sync in the rich text data from FLEx just works.
I also added the activity page back in as it was useful for debugging:
