Code review fixes

src/SIL.Converters.Usj/Usj.cs

@@ -9,12 +9,12 @@ public class Usj : UsjBase, IUsj
         /// <summary>
         /// The supported USJ spec type.
         /// </summary>
-        public const string UsjType = "USJ";
+        public static readonly string UsjType = "USJ";
 
         /// <summary>
         /// The supported USJ spec version.
         /// </summary>
-        public const string UsjVersion = "3.1";
+        public static readonly string UsjVersion = "3.1";
 
         /// <summary>
         /// The USJ spec version.

src/SIL.Converters.Usj/UsjBase.cs

@@ -22,7 +22,11 @@ public abstract class UsjBase
         /// The JSON representation of scripture contents from USFM/USX.
         /// </summary>
         /// <value>This will either be a <see cref="UsjMarker"/> or <see cref="string"/>.</value>
-        /// <remarks>Nullable. The contents will be laid out in order.</remarks>
+        /// <remarks>
+        /// Nullable.
+        /// If there are no contents, this will be null for <see cref="UsjMarker"/>, or empty for <see cref="Usj"/>.
+        /// The contents will be laid out in order.
+        /// </remarks>
         [JsonConverter(typeof(UsjContentConverter))]
         public ArrayList Content { get; set; }
 

src/SIL.Converters.Usj/UsjMarker.cs

@@ -12,13 +12,14 @@ public class UsjMarker : UsjBase
         public string Marker { get; set; }
 
         /// <summary>
-        /// Indicates the Book-chapter-verse value in the paragraph based structure.
+        /// The milestone start ID, which indicates the Book-chapter-verse value in the paragraph based structure.
         /// </summary>
         /// <remarks>Nullable.</remarks>
         public string Sid { get; set; }
 
         /// <summary>
-        /// Milestone end ID, matches start ID (not currently included in USJ spec).
+        /// Milestone end ID, which matches the milestone start ID <see cref="Sid"/>.
+        /// <see cref="Eid"/> is not specified in the USJ spec, but is kept for USX compatibility.
         /// </summary>
         /// <remarks>Nullable.</remarks>
         public string Eid { get; set; }
@@ -42,8 +43,12 @@ public class UsjMarker : UsjBase
         public string AltNumber { get; set; }
 
         /// <summary>
-        /// Published character of chapter or verse.
+        /// Published character of a chapter or verse.
         /// </summary>
+        /// <value>
+        /// This can be a letter (I, II, etc.), a number (1, 2, ...), or both.
+        /// It is only displayed in the published version of the scripture text.
+        /// </value>
         /// <remarks>Nullable.</remarks>
         public string PubNumber { get; set; }
 

src/SIL.Converters.Usj/UsjToUsx.cs

@@ -61,6 +61,13 @@ private static void SetAttributes(XmlElement element, UsjMarker markerContent)
             foreach (KeyValuePair<string, object> nonStandardAttribute in markerContent.AdditionalData)
             {
                 string key = nonStandardAttribute.Key;
+
+                // Include any non-standard attributes that are strings for compatibility with USX.
+                //
+                // Notes:
+                //  - If the non-standard attribute is not a string, discard it as it is invalid.
+                //  - Type and marker are handled above, so do not repeat them.
+                //  - Content is a collection handled in ConvertUsjRecurse.
                 if (nonStandardAttribute.Value is string value && key != "type" && key != "marker" && key != "content")
                 {
                     element.SetAttribute(key, value);
@@ -103,10 +110,14 @@ bool isLastItem
                 throw new ArgumentOutOfRangeException(nameof(markerContent));
             }
 
+            // Store the previous verse eid so we can close them in the correct place
+            string lastVerseEid = null;
+
             // Create chapter and verse end elements from SID attributes.
             if (_verseEid != null && (type == "verse" || (parentElement.Name == "para" && isLastItem)))
             {
                 eidElement = CreateVerseEndElement(usxDoc);
+                lastVerseEid = _verseEid;
                 _verseEid = null;
             }
 
@@ -126,15 +137,28 @@ bool isLastItem
                 _chapterEid = usjMarker.Sid;
             }
 
-            // Append to parent.
+            // See if we are at a new verse
+            if (eidElement != null && isLastItem && _verseEid != null && _verseEid != lastVerseEid)
+            {
+                // Write the eid element for the previous verse
+                parentElement.AppendChild(eidElement);
+
+                // Ensure that eid element for the current verse is not written
+                eidElement = null;
+                _verseEid = null;
+            }
+
+            // Append to parent to close the verse or chapter before this new node
             if (eidElement != null && !isLastItem)
             {
                 parentElement.AppendChild(eidElement);
+                eidElement = null;
             }
 
             parentElement.AppendChild(node);
 
-            if (eidElement != null && isLastItem)
+            // Append the eid element as this is the last element
+            if (eidElement != null)
             {
                 parentElement.AppendChild(eidElement);
             }
@@ -167,18 +191,35 @@ private void UsjToUsxDom(IUsj usj, XmlDocument usxDoc)
         }
 
         /// <summary>
-        /// Converts a USJ object to a USX string.
+        /// Converts a USJ object to a USX <see cref="XmlDocument"/>.
         /// </summary>
         /// <param name="usj">The USJ object.</param>
-        /// <returns>The USX as a string.</returns>
-        public string UsjToUsxString(IUsj usj)
+        /// <returns>The XML Document.</returns>
+        public XmlDocument UsjToUsxXmlDocument(IUsj usj)
         {
+            // Reset any instance variables
+            _chapterEid = null;
+            _verseEid = null;
+
+            // Create the USX document
             XmlDocument usxDoc = new XmlDocument { PreserveWhitespace = true };
             XmlElement documentElement = usxDoc.CreateElement(Usx.UsxType);
             documentElement.SetAttribute("version", Usx.UsxVersion);
             usxDoc.AppendChild(documentElement);
             UsjToUsxDom(usj, usxDoc);
+            return usxDoc;
+        }
+
+        /// <summary>
+        /// Converts a USJ object to a USX string.
+        /// </summary>
+        /// <param name="usj">The USJ object.</param>
+        /// <returns>The USX as a string.</returns>
+        public string UsjToUsxString(IUsj usj)
+        {
+            XmlDocument usxDoc = UsjToUsxXmlDocument(usj);
 
+            // Output as a string
             using (StringWriter stringWriter = new StringWriter())
             {
                 // These settings conform to ParatextData.UsfmToUsx

src/SIL.Converters.Usj/Usx.cs

@@ -9,11 +9,11 @@ public static class Usx
         /// <summary>
         /// The USX spec type.
         /// </summary>
-        public const string UsxType = "usx";
+        public static readonly string UsxType = "usx";
 
         /// <summary>
         /// The USX spec version.
         /// </summary>
-        public const string UsxVersion = "3.0";
+        public static readonly string UsxVersion = "3.0";
     }
 }

src/SIL.Converters.Usj/UsxToUsj.cs

@@ -1,3 +1,4 @@
+using System;
 using System.Collections;
 using System.Collections.Generic;
 using System.Linq;
@@ -33,7 +34,7 @@ private static (T, bool) UsxDomToUsjRecurse<T>(XmlElement usxElement)
                 .Attributes.Cast<XmlAttribute>()
                 .ToDictionary(attrib => attrib.Name, attrib => attrib.Value);
 
-            // If style is present, make that the market
+            // If style is present, make that the marker
             if (attributes.TryGetValue("style", out string marker))
             {
                 attributes.Remove("style");
@@ -105,6 +106,9 @@ private static (T, bool) UsxDomToUsjRecurse<T>(XmlElement usxElement)
                     outObj.Content.Add(childDict);
                 }
 
+                // If the next sibling is text or a user inputted space (a single space), add it to the content.
+                // We skip whitespace nodes with more than one space because they are padding, not formatting spaces.
+                // Note: Any Paratext 9.5 special whitespace characters will have a node type of XmlNodeType.Text.
                 if (
                     child.NextSibling?.NodeType == XmlNodeType.Text
                     || (child.NextSibling?.NodeType == XmlNodeType.Whitespace && child.NextSibling.Value == " ")
@@ -121,7 +125,7 @@ private static (T, bool) UsxDomToUsjRecurse<T>(XmlElement usxElement)
 
             if (outObj is UsjMarker usjMarker2 && usjMarker2.Eid != null && (type == "verse" || type == "chapter"))
             {
-                // Ignore
+                // Omit any verse or chapter eid elements
                 append = false;
             }
 
@@ -173,6 +177,17 @@ public static Usj UsxStringToUsj(string usx)
         /// <remarks>
         /// The <see cref="XmlDocument"/> should have <see cref="XmlDocument.PreserveWhitespace"/> set to <c>true</c>.
         /// </remarks>
-        public static Usj UsxXmlDocumentToUsj(XmlDocument xmlDocument) => UsxDomToUsj(xmlDocument?.DocumentElement);
+        public static Usj UsxXmlDocumentToUsj(XmlDocument xmlDocument)
+        {
+            if (xmlDocument?.PreserveWhitespace != true)
+            {
+                throw new ArgumentException(
+                    "The XmlDocument should have PreserveWhitespace set to true.",
+                    nameof(xmlDocument)
+                );
+            }
+
+            return UsxDomToUsj(xmlDocument.DocumentElement);
+        }
     }
 }