diff --git a/src/SIL.XForge.Scripture/Services/CommentManagerExtensions.cs b/src/SIL.XForge.Scripture/Services/CommentManagerExtensions.cs new file mode 100644 index 00000000000..c5b10357d44 --- /dev/null +++ b/src/SIL.XForge.Scripture/Services/CommentManagerExtensions.cs @@ -0,0 +1,57 @@ +using System; +using System.Collections.Generic; +using System.Linq; +using Paratext.Data.ProjectComments; + +namespace SIL.XForge.Scripture.Services; + +/// Extension methods for ParatextData's . +public static class CommentManagerExtensions +{ + /// + /// Finds comment threads, like + /// , but with + /// every thread guaranteed to be complete. FindThreads sorts all comments and then groups only + /// adjacent comments that share a thread id. Its sort order is undefined for a thread whose + /// comments do not all share the same anchor, so FindThreads can return such a thread split + /// into fragments that share an id. (Biblical-term and spelling-note threads have this shape: + /// their location-independent ids, such as BT_term and project_word, collect comments + /// anchored at different verses.) Instead, build every thread the way + /// does (select comments by thread id and + /// sort them), but in one pass over the comments rather than one full scan per thread. + /// Threads are returned ordered by their oldest comment's anchor. The filters are applied to + /// the complete threads. + /// + public static List FindCompleteThreads( + this CommentManager manager, + Func? shouldThreadBeIncluded = null, + bool activeOnly = false + ) + { + // Unlike FindThreads and FindThread, AllComments does not take the manager's lock, so + // this must not run concurrently with mutations of the same project's comments. + Dictionary threadsById = []; + foreach (Comment comment in manager.AllComments) + { + if (!threadsById.TryGetValue(comment.Thread, out CommentThread? thread)) + { + thread = new CommentThread { ScrText = manager.ScrText }; + threadsById[comment.Thread] = thread; + } + thread.Comments.Add(comment); + } + List threads = [.. threadsById.Values]; + // Comment.CompareTo compares same-thread comments by date and different-thread comments + // by anchor, so this sorts each thread's comments oldest first (as FindThread does) and + // then orders the threads by the anchor of each thread's oldest comment. + foreach (CommentThread thread in threads) + thread.Comments.Sort(); + threads.Sort((a, b) => a.Comments[0].CompareTo(b.Comments[0])); + IEnumerable result = threads; + if (activeOnly) + result = result.Where(t => t.Comments.Any(c => !c.Deleted)); + if (shouldThreadBeIncluded is not null) + result = result.Where(shouldThreadBeIncluded); + return [.. result]; + } +} diff --git a/src/SIL.XForge.Scripture/Services/ParatextDataHelper.cs b/src/SIL.XForge.Scripture/Services/ParatextDataHelper.cs index 4fb55bce71a..15a8829ccf8 100644 --- a/src/SIL.XForge.Scripture/Services/ParatextDataHelper.cs +++ b/src/SIL.XForge.Scripture/Services/ParatextDataHelper.cs @@ -42,7 +42,7 @@ public void CommitVersionedText(ScrText scrText, string comment) public IReadOnlyList GetNotes(CommentManager commentManager, CommentTags commentTags) { // Only return note threads that are not resolved and active - IEnumerable threads = commentManager.FindThreads( + IEnumerable threads = commentManager.FindCompleteThreads( t => t.Status != NoteStatus.Resolved, activeOnly: true ); diff --git a/src/SIL.XForge.Scripture/Services/ParatextService.cs b/src/SIL.XForge.Scripture/Services/ParatextService.cs index ad63bdfe3b6..c4993ad8117 100644 --- a/src/SIL.XForge.Scripture/Services/ParatextService.cs +++ b/src/SIL.XForge.Scripture/Services/ParatextService.cs @@ -1334,10 +1334,10 @@ public string GetNotes(UserSecret userSecret, string paratextId, int bookNum) // CommentThread.VerseRef determines the location of a thread, even if moved. However, in Paratext a note can // only be relocated within the chapter, so for our query, we only need to look at the first note location. - var threads = manager.FindThreads( + var threads = manager.FindCompleteThreads( commentThread => commentThread.Comments[0].VerseRefStr.StartsWith(verseRefBook, StringComparison.OrdinalIgnoreCase), - true + activeOnly: true ); return NotesFormatter.FormatNotes(threads); } @@ -1372,6 +1372,13 @@ Dictionary ptProjectUsers nt.Data.Notes.Any(n => !n.Deleted) ); + // FindThread(id) answers by scanning the project's entire comment list, so calling it for + // each of the project's threads costs one full scan per thread (minutes of work at tens + // of thousands of threads). Instead, get every thread in one pass and index them by id. + Dictionary commentThreadsById = commentManager + .FindCompleteThreads() + .ToDictionary(t => t.Id); + foreach (var threadDoc in activeNoteThreadDocs) { List matchedCommentIds = []; @@ -1388,7 +1395,7 @@ Dictionary ptProjectUsers threadDoc.Data.ExtraHeadingInfo ); // Find the corresponding comment thread - CommentThread? existingThread = commentManager.FindThread(threadDoc.Data.ThreadId); + commentThreadsById.TryGetValue(threadDoc.Data.ThreadId, out CommentThread? existingThread); if (existingThread is null) { // The thread has been removed @@ -1462,7 +1469,7 @@ Dictionary ptProjectUsers IEnumerable newThreadIds = ptThreadIds.Except(matchedThreadIds); foreach (string threadId in newThreadIds) { - CommentThread? thread = commentManager.FindThread(threadId); + commentThreadsById.TryGetValue(threadId, out CommentThread? thread); if (thread is null || thread.Comments.All(c => c.Deleted)) continue; Paratext.Data.ProjectComments.Comment info = thread.Comments[0]; @@ -2989,17 +2996,13 @@ private static IEnumerable GetCommentThreads(CommentManager manag // reallocated within the chapter, so for our query, we only need the first location. // A Biblical Term has a VerseRef, but it is usually not useful, so we exclude BT notes when getting a book's notes // The VerseRef will still be stored for a BT note, as this is a PT requirement. - return manager.FindThreads( - commentThread => - ( - bookNum != null - && commentThread - .Comments[0] - .VerseRefStr.StartsWith(verseRefBook, StringComparison.OrdinalIgnoreCase) - && !commentThread.IsBTNote - && !commentThread.Id.StartsWith("ANSWER_") - ) || (bookNum == null && commentThread.IsBTNote), - false + return manager.FindCompleteThreads(commentThread => + ( + bookNum != null + && commentThread.Comments[0].VerseRefStr.StartsWith(verseRefBook, StringComparison.OrdinalIgnoreCase) + && !commentThread.IsBTNote + && !commentThread.Id.StartsWith("ANSWER_") + ) || (bookNum == null && commentThread.IsBTNote) ); } diff --git a/test/SIL.XForge.Scripture.Tests/Services/CommentManagerExtensionsTests.cs b/test/SIL.XForge.Scripture.Tests/Services/CommentManagerExtensionsTests.cs new file mode 100644 index 00000000000..a36105f8cf7 --- /dev/null +++ b/test/SIL.XForge.Scripture.Tests/Services/CommentManagerExtensionsTests.cs @@ -0,0 +1,156 @@ +using System; +using System.Collections.Generic; +using System.IO; +using System.Linq; +using System.Xml; +using NSubstitute; +using NUnit.Framework; +using Paratext.Data; +using Paratext.Data.Languages; +using Paratext.Data.ProjectComments; +using Paratext.Data.ProjectFileAccess; +using Paratext.Data.Repository; +using SIL.WritingSystems; +using SIL.XForge.Scripture.Models; +using ParatextComment = Paratext.Data.ProjectComments.Comment; + +namespace SIL.XForge.Scripture.Services; + +[TestFixture] +public class CommentManagerExtensionsTests +{ + private const string ParatextUser01 = "ParatextUser01"; + + // A thread whose comments do not all share the same anchor makes Comment.CompareTo + // inconsistent: comments of the same thread compare to each other by date, but to other + // threads' comments by anchor. Biblical-term and spelling-note threads have this shape: + // their location-independent ids (BT_term, project_word) collect comments anchored at + // different verses. CommentManager.FindThreads sorts all + // comments and then groups only adjacent comments that share a thread id, so an unlucky sort + // order returns such a thread split into fragments. This arrangement makes the split + // deterministic: each comment compares greater than the one before it, so List.Sort's + // insertion sort (used for small lists) keeps this exact order, leaving thread-a's two + // comments separated by thread-b's. thread-a's second comment anchors after thread-b's + // comment but is dated before its own thread-mate, which is where the inconsistency bites. + private static readonly (string Thread, string VerseRef, DateTimeOffset Date)[] SplitThreadArrangement = + [ + ("thread-a", "RUT 1:1", new DateTimeOffset(2020, 1, 1, 0, 0, 0, TimeSpan.Zero)), + ("thread-b", "RUT 1:2", new DateTimeOffset(2020, 1, 2, 0, 0, 0, TimeSpan.Zero)), + ("thread-a", "RUT 1:3", new DateTimeOffset(2019, 1, 1, 0, 0, 0, TimeSpan.Zero)), + ("thread-c", "RUT 1:4", new DateTimeOffset(2020, 1, 3, 0, 0, 0, TimeSpan.Zero)), + ]; + + [Test] + public void FindThreads_SplitsAThreadWhoseCommentsHaveDifferentAnchors() + { + var env = new TestEnvironment(); + using MockScrText scrText = env.GetScrText(HexId.CreateNew().ToString()); + CommentManager manager = CommentManager.Get(scrText); + foreach ((string thread, string verseRef, DateTimeOffset date) in SplitThreadArrangement) + TestEnvironment.AddComment(scrText, thread, verseRef, date); + + // SUT + List threads = manager.FindThreads(); + + // This asserts the ParatextData bug that FindCompleteThreads exists to work around: + // thread-a comes back as two single-comment fragments sharing an id. If this test fails + // after a ParatextData (or .NET sort) update because thread-a comes back whole, the + // workaround (and this test pair) can likely be removed. + Assert.That(threads.Count(t => t.Id == "thread-a"), Is.EqualTo(2)); + Assert.That(threads.Where(t => t.Id == "thread-a").Select(t => t.Comments.Count), Is.All.EqualTo(1)); + } + + [Test] + public void FindCompleteThreads_ReturnsEveryThreadComplete() + { + var env = new TestEnvironment(); + using MockScrText scrText = env.GetScrText(HexId.CreateNew().ToString()); + CommentManager manager = CommentManager.Get(scrText); + foreach ((string thread, string verseRef, DateTimeOffset date) in SplitThreadArrangement) + TestEnvironment.AddComment(scrText, thread, verseRef, date); + + // SUT + List threads = manager.FindCompleteThreads(); + + Assert.That(threads.Select(t => t.Id), Is.EquivalentTo(new[] { "thread-a", "thread-b", "thread-c" })); + CommentThread threadA = threads.Single(t => t.Id == "thread-a"); + // Complete, and in FindThread's order: oldest comment first + Assert.That(threadA.Comments.Select(c => c.VerseRefStr), Is.EqualTo(["RUT 1:3", "RUT 1:1"])); + } + + [Test] + public void FindCompleteThreads_OrdersThreadsByOldestCommentAnchor() + { + var env = new TestEnvironment(); + using MockScrText scrText = env.GetScrText(HexId.CreateNew().ToString()); + CommentManager manager = CommentManager.Get(scrText); + foreach ((string thread, string verseRef, DateTimeOffset date) in SplitThreadArrangement) + TestEnvironment.AddComment(scrText, thread, verseRef, date); + + // SUT + List threads = manager.FindCompleteThreads(); + + // A thread is located at its oldest comment's anchor: thread-a's oldest comment (dated + // 2019) anchors at RUT 1:3, placing the thread between thread-b (RUT 1:2) and thread-c + // (RUT 1:4). + Assert.That(threads.Select(t => t.Id), Is.EqualTo(["thread-b", "thread-a", "thread-c"])); + } + + private class TestEnvironment + { + private readonly string _syncDir = Path.GetTempPath(); + + public TestEnvironment() + { + // Ensure that the SLDR is initialized for LanguageID.Code to be retrieved correctly + if (!Sldr.IsInitialized) + Sldr.Initialize(true); + + // Setup Mercurial for tests + Hg.DefaultRunnerCreationFunc = (_, _, _) => new MockHgRunner(); + Hg.Default = new MockHg(); + VersionedText.AllCommitsDisabled = true; + } + + public static void AddComment(ScrText scrText, string threadId, string verseRef, DateTimeOffset date) + { + XmlDocument doc = new XmlDocument(); + XmlElement root = doc.CreateElement("content"); + XmlElement paragraph = doc.CreateElement("p"); + paragraph.InnerText = "comment text"; + root.AppendChild(paragraph); + doc.AppendChild(root); + + var comment = new ParatextComment(scrText.User) + { + Thread = threadId, + VerseRefStr = verseRef, + Contents = root, + DateTime = date, + Status = NoteStatus.Todo, + SelectedText = string.Empty, + StartPosition = 0, + }; + CommentManager.Get(scrText).AddComment(comment); + } + + public MockScrText GetScrText(string paratextId) + { + string scrTextDir = Path.Join(_syncDir, paratextId, "target"); + ProjectName projectName = new ProjectName { ProjectPath = scrTextDir, ShortName = "Proj" }; + var scrText = new MockScrText(new SFParatextUser(ParatextUser01), projectName) + { + CachedGuid = HexId.FromStr(paratextId), + }; + scrText.Settings.LanguageID = LanguageId.English; + scrText.Settings.FileNamePostPart = ".SFM"; + + // Set up the file manager for the comment manager + ProjectFileManager fileManager = Substitute.For(scrText, null); + fileManager.IsWritable.Returns(true); + scrText.SetFileManager(fileManager); + + return scrText; + } + } +}