From 955117439454b0d0b8a8fbcc965b6d3c8f6063a9 Mon Sep 17 00:00:00 2001 From: Hatton Date: Fri, 4 Sep 2026 14:49:32 -0600 Subject: [PATCH 1/5] Spreadsheet: a hidden [details] column and a registration point for the things on a page that the ordinary columns cannot carry Some things on a page cannot survive a spreadsheet round trip through the text, image, video and widget columns: putting them back needs state that none of those columns holds. This adds the generic mechanism for them, so that a feature which needs it registers one object and gets export, import, placement and reporting for free. It adds no such feature itself. The parts: - InternalSpreadsheet gains a hidden [details] column. A row's cell in it holds a JSON object whose "kind" property says what the row is for, so a blob identifies itself even apart from its row. HiddenColumns now tolerates optional columns being absent instead of reporting -1 as a column to hide. The column's presence anywhere in a spreadsheet marks that spreadsheet as the authority on the objects whose rows use it; a spreadsheet without it (any made by an older Bloom) can say nothing about them, so import leaves whatever the book has alone and behaves exactly as before. - SpreadsheetIO keeps [details] out of the WYSIWYG formatting and out of XML escaping, so the JSON comes back byte for byte. Escaping an ampersand in it would corrupt what we are copying verbatim. - ISpreadsheetObjectKind (SpreadsheetObjectKind.cs) is the registration point: a selector saying which page elements the kind owns, so the generic collectors of translation groups, bloom-canvases, video containers and widget containers leave their innards alone; an export method producing the object's rows; and an import method consuming the row family. Registering a kind is all it takes. - SpreadsheetExporter writes each object's rows at the object's own position among the page's [page content] rows, worked out from document order, and makes the [details] column before it calls the kind. WriteTranslationGroup, WriteVideo, ImagePath and CopyImageFileToSpreadsheetFolder become internal so a kind can put its own text, video and pictures into its rows -- text then reaching a translator in the same language columns as any other text. - SpreadsheetImporter treats such an object as a page block in its own right, so a lead row advances onto the object's page and picks the object there just as a [page content] row picks its translation group. That is the only thing that could reach a page whose sole content is such an object. The importer collects the row family, and skips it with a row-numbered warning when the sheet has no [details] column or the page has nowhere to put the object; it never invents one, since that would be an object of a shape nothing in the spreadsheet asked for. A continuation row stranded from its lead row is reported too. Warn, CurrentRowIndexForMessages, PutRowInGroupAsync, PutRowInImageAsync, PutRowInVideo and DestinationDom become internal for the use of a kind. Tests: SpreadsheetDetailsTests registers a stub kind of its own and covers the [details] column being present and hidden, the lead row carrying the kind and its state, an object's rows landing at the object's position, the object's inner text not also being exported as page content, a round trip restoring both the text and the state only [details] carries, an import into a page with nowhere to put the object reporting and skipping while still importing the rest of the page, a stranded continuation row being reported, and a spreadsheet with no [details] column importing as it always did. 11 tests; BloomTests.Spreadsheet 405/405; full BloomTests 3357 passed, 13 skipped, 0 failed. Co-Authored-By: Claude Opus 5 --- .../Spreadsheet/InternalSpreadsheet.cs | 15 +- .../Spreadsheet/SpreadsheetExporter.cs | 170 ++++- src/BloomExe/Spreadsheet/SpreadsheetIO.cs | 7 +- .../Spreadsheet/SpreadsheetImporter.cs | 255 +++++++- .../Spreadsheet/SpreadsheetObjectKind.cs | 272 ++++++++ .../Spreadsheet/SpreadsheetDetailsTests.cs | 587 ++++++++++++++++++ 6 files changed, 1272 insertions(+), 34 deletions(-) create mode 100644 src/BloomExe/Spreadsheet/SpreadsheetObjectKind.cs create mode 100644 src/BloomTests/Spreadsheet/SpreadsheetDetailsTests.cs diff --git a/src/BloomExe/Spreadsheet/InternalSpreadsheet.cs b/src/BloomExe/Spreadsheet/InternalSpreadsheet.cs index 84525e0521c4..b60fe028bee8 100644 --- a/src/BloomExe/Spreadsheet/InternalSpreadsheet.cs +++ b/src/BloomExe/Spreadsheet/InternalSpreadsheet.cs @@ -39,6 +39,16 @@ public class InternalSpreadsheet public const string WidgetSourceColumnLabel = "[activities source]"; public const string PageTypeColumnLabel = "[page type]"; public const string AttributeColumnLabel = "[attribute]"; + + // A hidden column holding a JSON object with whatever non-textual state a row's + // object needs to be reconstructed on import. Which object a row is for is said by + // the "kind" property of that JSON, matching the Kind of a registered + // ISpreadsheetObjectKind. Its presence anywhere in a spreadsheet marks that + // spreadsheet as the authority on the objects whose rows use it; a spreadsheet + // without the column (e.g. made by an older Bloom) can say nothing about them, so + // import leaves whatever the book already has alone. + public const string DetailsColumnLabel = "[details]"; + public const string DetailsColumnFriendlyName = "Details"; public const string ImageSourceColumnFriendlyName = "Image File Path"; public const string BlankContentIndicator = "[blank]"; @@ -350,7 +360,10 @@ public int AddColumnForTag( { GetColumnForTag(PageNumberColumnLabel), GetColumnForTag(ImageSourceColumnLabel), - }; + GetColumnForTag(DetailsColumnLabel), + } + .Where(i => i >= 0) // optional columns may be absent + .ToList(); public void SortHiddenContentRowsToTheBottom() { diff --git a/src/BloomExe/Spreadsheet/SpreadsheetExporter.cs b/src/BloomExe/Spreadsheet/SpreadsheetExporter.cs index bc4587950e45..6b6bb1d9c6b3 100644 --- a/src/BloomExe/Spreadsheet/SpreadsheetExporter.cs +++ b/src/BloomExe/Spreadsheet/SpreadsheetExporter.cs @@ -183,42 +183,91 @@ private void AddContentRows( Color colorForPage ) { - var imageContainers = GetImageContainersAndBloomCanvases(page); + // Anything inside one of these objects belongs to the object, which gets rows + // of its own below; it must not also become a [page content] row, or a single + // object holding many text boxes would fill the sheet with positional rows that + // no importer could put back where they came from. + var objects = SpreadsheetObjectKinds.ObjectsOnPage(page); + var imageContainers = GetImageContainersAndBloomCanvases(page) + .Where(x => !SpreadsheetObjectKinds.IsInsideAnObject(x)) + .ToList(); var allGroups = TranslationGroupManager.SortedGroupsOnPage(page, true); var groups = allGroups - .Where(x => !x.GetAttribute("class").Contains("bloom-imageDescription")) + .Where(x => + !x.GetAttribute("class").Contains("bloom-imageDescription") + && !SpreadsheetObjectKinds.IsInsideAnObject(x) + ) .ToList(); var videoContainers = page.SafeSelectNodes( ".//*[contains(@class,'bloom-videoContainer')]" ) .Cast() + .Where(x => !SpreadsheetObjectKinds.IsInsideAnObject(x)) .ToList(); var widgetContainers = page.SafeSelectNodes( ".//*[contains(@class,'bloom-widgetContainer')]" ) .Cast() + .Where(x => !SpreadsheetObjectKinds.IsInsideAnObject(x)) .ToList(); var pageType = SpreadsheetImporter.GetLabelFromPage(page); + // Puts the page type on the first row we make for the page, whatever kind of row + // that is. If we make more rows for this page, we don't specify a type; that + // allows subsequent rows to go onto the same page if there is room. + void SetPageTypeIfNeeded(ContentRow rowNeedingType) + { + if (string.IsNullOrEmpty(pageType)) + return; + var pageTypeIndex = _spreadsheet.AddColumnForTag( + InternalSpreadsheet.PageTypeColumnLabel, + "Page Type" + ); + rowNeedingType.SetCell(pageTypeIndex, pageType); + pageType = null; + } + // Each of these will result in one row in the output. - var rowContentSources = Extensions.MapUnevenLists( + var rowContentSources = Extensions + .MapUnevenLists( + new[] { groups, imageContainers, videoContainers, widgetContainers } + ) + .ToList(); + + // Where each object's rows go among the page's [page content] rows: after as + // many of them as hold something that precedes the object in the document. + var pageContentRowsBeforeObject = RowsBeforeEachObject( + page, + objects, new[] { groups, imageContainers, videoContainers, widgetContainers } ); - foreach (var pageContent in rowContentSources) + var objectsWritten = 0; + void WriteObjectsDueBefore(int pageContentRowsWritten) { - var row = new ContentRow(_spreadsheet); - if (!string.IsNullOrEmpty(pageType)) + while ( + objectsWritten < objects.Count + && pageContentRowsBeforeObject[objectsWritten] <= pageContentRowsWritten + ) { - var pageTypeIndex = _spreadsheet.AddColumnForTag( - InternalSpreadsheet.PageTypeColumnLabel, - "Page Type" + ExportObjectRows( + objects[objectsWritten], + pageNumber, + colorForPage, + bookFolderPath, + SetPageTypeIfNeeded ); - row.SetCell(pageTypeIndex, pageType); - // If we make more rows for this page, don't specify a type. - // This allows subsequent rows to go onto the same page if there is room. - pageType = null; + objectsWritten++; } + } + + var pageContentRowsSoFar = 0; + foreach (var pageContent in rowContentSources) + { + WriteObjectsDueBefore(pageContentRowsSoFar); + pageContentRowsSoFar++; + var row = new ContentRow(_spreadsheet); + SetPageTypeIfNeeded(row); row.SetCell( InternalSpreadsheet.RowTypeColumnLabel, InternalSpreadsheet.PageContentRowLabel @@ -279,6 +328,72 @@ Color colorForPage row.BackgroundColor = colorForPage; } + // Any objects that come after everything else on the page, including the case of + // a page whose only content is such an object, where there are no [page content] + // rows at all. + WriteObjectsDueBefore(int.MaxValue); + } + + /// + /// For each object, how many [page content] rows must be written before it: the + /// largest number of items that precede it in document order in any one of the + /// lists those rows are made from. (Row k holds groups[k], imageContainers[k] and so + /// on, so everything before the object has been written once we have written that + /// many rows.) + /// + private static List RowsBeforeEachObject( + SafeXmlElement page, + List objects, + List[] rowContentLists + ) + { + if (objects.Count == 0) + return new List(); + var documentOrder = SpreadsheetObjectKinds.GetDocumentOrder(page); + int OrderOf(SafeXmlElement element) => + documentOrder.TryGetValue(element, out var order) ? order : int.MaxValue; + return objects + .Select(objectOnPage => + { + var objectOrder = OrderOf(objectOnPage.Element); + return rowContentLists + .Select(list => list.Count(item => OrderOf(item) < objectOrder)) + .Max(); + }) + .ToList(); + } + + /// + /// Writes the rows for one object of a registered ISpreadsheetObjectKind, at the + /// point in the page's rows where the object belongs. All this generic code does is + /// make sure the hidden [details] column exists -- its presence is what tells the + /// importer that this spreadsheet is the authority on such objects -- and then let + /// the kind write whatever rows it needs. + /// + private void ExportObjectRows( + SpreadsheetObjectOnPage objectOnPage, + string pageNumber, + Color colorForPage, + string bookFolderPath, + Action setPageTypeIfNeeded + ) + { + _spreadsheet.AddColumnForTag( + InternalSpreadsheet.DetailsColumnLabel, + InternalSpreadsheet.DetailsColumnFriendlyName + ); + objectOnPage.Kind.ExportObject( + objectOnPage.Element, + new SpreadsheetObjectExportContext + { + Exporter = this, + Spreadsheet = _spreadsheet, + PageNumber = pageNumber, + ColorForPage = colorForPage, + BookFolderPath = bookFolderPath, + SetPageTypeIfNeeded = setPageTypeIfNeeded, + } + ); } private void WriteWidget( @@ -341,7 +456,12 @@ string bookFolderPath } } - private void WriteVideo( + /// + /// Writes a bloom-videoContainer's video into the row's [video source] cell, + /// copying the file into the spreadsheet's video folder. internal so that an + /// ISpreadsheetObjectKind can put a video inside its object into a row of its own. + /// + internal void WriteVideo( SafeXmlElement videoContainer, ContentRow row, string bookFolderPath @@ -389,7 +509,13 @@ string bookFolderPath } } - private void WriteTranslationGroup( + /// + /// Writes each language's text of a bloom-translationGroup into that language's + /// column of the row (plus any audio columns). internal so that an + /// ISpreadsheetObjectKind can put text inside its object into a row of its own, and + /// so a translator sees that text in the same language columns as any other text. + /// + internal void WriteTranslationGroup( SafeXmlElement translationGroup, ContentRow row, string bookFolderPath @@ -634,7 +760,12 @@ private List GetImageContainersAndBloomCanvases(SafeXmlElement e .ToList(); } - private string ImagePath(string imagesFolderPath, string imageSrc) + /// + /// The full path of an image, given the folder it is in and the (url-encoded) src + /// attribute that referred to it. internal for the use of ISpreadsheetObjectKind + /// implementations that export images inside their objects. + /// + internal string ImagePath(string imagesFolderPath, string imageSrc) { return Path.Combine( imagesFolderPath, @@ -840,7 +971,12 @@ ref imageSrcAttribute } } - private void CopyImageFileToSpreadsheetFolder(string imageSourcePath) + /// + /// Copies an image the book uses into the spreadsheet's images folder, so that the + /// spreadsheet is self-contained. internal for the use of ISpreadsheetObjectKind + /// implementations that export images inside their objects. + /// + internal void CopyImageFileToSpreadsheetFolder(string imageSourcePath) { if (_outputImageFolder != null) { diff --git a/src/BloomExe/Spreadsheet/SpreadsheetIO.cs b/src/BloomExe/Spreadsheet/SpreadsheetIO.cs index a03483b42e7e..ba43e95cbdbd 100644 --- a/src/BloomExe/Spreadsheet/SpreadsheetIO.cs +++ b/src/BloomExe/Spreadsheet/SpreadsheetIO.cs @@ -379,8 +379,12 @@ private static bool WantXmlEscaping(SpreadsheetRow row, int index) // PageType column holds what is basically the InnerText of an element; // the InnerText property handles any XML escaping. // Image source column holds the actual path to the file; it will be url encoded when set as the src. + // The details column holds JSON, which we must get back byte for byte: escaping + // its ampersands (e.g. in a url inside an attribute value) would corrupt what we + // are copying verbatim. return key != InternalSpreadsheet.PageTypeColumnLabel - && key != InternalSpreadsheet.ImageSourceColumnLabel; + && key != InternalSpreadsheet.ImageSourceColumnLabel + && key != InternalSpreadsheet.DetailsColumnLabel; } private static bool IsWysiwygFormattedColumn(SpreadsheetRow row, int index) @@ -392,6 +396,7 @@ private static bool IsWysiwygFormattedColumn(SpreadsheetRow row, int index) || key == InternalSpreadsheet.WidgetSourceColumnLabel || key == InternalSpreadsheet.PageTypeColumnLabel || key == InternalSpreadsheet.AttributeColumnLabel + || key == InternalSpreadsheet.DetailsColumnLabel ) return false; return !nonWysiwygColumns.Contains(key); diff --git a/src/BloomExe/Spreadsheet/SpreadsheetImporter.cs b/src/BloomExe/Spreadsheet/SpreadsheetImporter.cs index a4640723188d..9705a4bd644f 100644 --- a/src/BloomExe/Spreadsheet/SpreadsheetImporter.cs +++ b/src/BloomExe/Spreadsheet/SpreadsheetImporter.cs @@ -39,17 +39,27 @@ public class SpreadsheetImporter private SafeXmlElement _currentPage; private List _pages; - // text, image, video, widget - const int blockTypeCount = 4; + // text, image, video, widget, registered object + const int blockTypeCount = 5; // lists of translation groups (other than image descriptions), - // bloom-canvases, video containers, and widget containers. + // bloom-canvases, video containers, widget containers, and the objects of + // registered ISpreadsheetObjectKinds. List[] _blocksOnPage = new List[blockTypeCount]; public const int translationGroupIndex = 0; public const int bloomCanvasIndex = 1; public const int videoContainerIndex = 2; public const int widgetContainerIndex = 3; + // An object of a registered ISpreadsheetObjectKind is a block in its own right, so + // that its lead row can advance the importer onto its page and pick the right + // object there just as a [page content] row does for text and pictures. A page + // whose only content is such an object has no [page content] row at all, so nothing + // else would get us there. All registered kinds share this one slot, which is + // enough while at most one kind puts objects on a page; a second such kind would + // need a slot of its own to keep its own place on the page. + public const int objectIndex = 4; + // for each kind of block, this gives the index in the corresponding list in blocksOnPage // of the next block we should import into. May be -1 or too large if there are no more // blocks of that type we can import into on this page. @@ -276,6 +286,7 @@ public async Task> ImportAsync( _currentPageIndex = -1; _blocksOnPage[translationGroupIndex] = new List(); _blocksOnPage[bloomCanvasIndex] = new List(); + _blocksOnPage[objectIndex] = new List(); _destLayout = Layout.FromDom(_destinationDom, Layout.A5Portrait); var pageTypeIndex = sheet.GetColumnForTag(InternalSpreadsheet.PageTypeColumnLabel); while (_currentRowIndex < _inputRows.Count) @@ -378,6 +389,26 @@ await PutRowInGroupAsync( typesInRow &= ~typesToPut; } } + else if (SpreadsheetObjectKinds.ForLeadRowLabel(rowTypeLabel) != null) + { + await ImportObjectAsync( + SpreadsheetObjectKinds.ForLeadRowLabel(rowTypeLabel), + pageType + ); + } + else if ( + rowTypeLabel != InternalSpreadsheet.ImageDescriptionRowLabel + && SpreadsheetObjectKinds.ThatWouldContinueWith(currentRow) + is ISpreadsheetObjectKind strandedKind + ) + { + // A row that means something only as part of the family led by its + // kind's lead row, but that we did not reach through that lead row. + // (Normally the branch above consumes such a row.) + Warn( + $"Row {CurrentRowIndexForMessages} is a {rowTypeLabel} row that does not follow a {strandedKind.LeadRowLabel} row, so Bloom could not use it." + ); + } else if (rowTypeLabel.StartsWith("[") && rowTypeLabel.EndsWith("]")) //This row is xmatter { var dataBookLabel = InternalSpreadsheet.MapRowLabelToDataBookLabel( @@ -650,7 +681,12 @@ private void ImportStylesheetsIfNeeded(string sourceBook) } } - private void PutRowInVideo(ContentRow currentRow, SafeXmlElement videoContainer) + /// + /// Puts a row's video into a bloom-videoContainer. internal so that an + /// ISpreadsheetObjectKind can fill a video container inside its object from one of + /// its own rows. + /// + internal void PutRowInVideo(ContentRow currentRow, SafeXmlElement videoContainer) { var source = currentRow.GetCell(InternalSpreadsheet.VideoSourceColumnLabel).Text; Debug.Assert( @@ -837,7 +873,12 @@ private void CleanupLeftOverPages() } } - private async Task PutRowInImageAsync( + /// + /// Puts a row's image (and any description row that followed it) into a + /// bloom-canvas. internal so that an ISpreadsheetObjectKind can fill a bloom-canvas + /// inside its object from one of its own rows. + /// + internal async Task PutRowInImageAsync( ContentRow currentRow, ContentRow descriptionRow, SafeXmlElement currentBloomCanvas @@ -963,7 +1004,11 @@ private void CopyImageFileToDestination( } } - void Warn(string message) + /// + /// Reports a problem with the import to the user. internal so that an + /// ISpreadsheetObjectKind can report a problem with one of its own rows. + /// + internal void Warn(string message) { _warnings.Add(message); _progress?.MessageWithoutLocalizing(message, ProgressKind.Warning); @@ -1655,6 +1700,25 @@ string pageTypeWeHave ); } + // None of Bloom's default pages holds an object of a registered kind, so there + // is no page we could generate that would satisfy one. Drop the flag and let + // the caller decide what to do with the page we have: either it can hold the + // rest of what the row needs, or ImportObjectAsync reports that the page has no + // object for the row and skips it. We must not invent one: it would be an object + // of a shape nothing in the spreadsheet asked for. + if ( + string.IsNullOrEmpty(guid) + && (blocksNeeded & BlockTypes.Object) == BlockTypes.Object + ) + { + return InsertDefaultPageIfNeeded( + blocksNeeded & ~BlockTypes.Object, + blocksWeHave, + pageTypeNeeded, + _pageTypeOfLastPage + ); + } + if (string.IsNullOrEmpty(guid)) { throw new ApplicationException("Failed to find a default page type"); @@ -1666,10 +1730,10 @@ string pageTypeWeHave // A good index to show for the current row in messages. This should be the actual // row number Excel displays next to the row. - private int CurrentRowIndexForMessages => + internal int CurrentRowIndexForMessages => _sheet.GetIndexOfRow(_inputRows[_currentRowIndex]) + 1; - private static IEnumerable SafeSelectNodesByClassName( + internal static IEnumerable SafeSelectNodesByClassName( SafeXmlNode ancestor, string elementXPath, string className, @@ -1687,7 +1751,7 @@ private static IEnumerable SafeSelectNodesByClassName( .Cast(); } - private static bool HasExactClassName(SafeXmlElement element, string className) + internal static bool HasExactClassName(SafeXmlElement element, string className) { return element .GetAttribute("class") @@ -1695,6 +1759,150 @@ private static bool HasExactClassName(SafeXmlElement element, string className) .Contains(className); } + /// + /// The HtmlDom being imported into. internal so that an ISpreadsheetObjectKind can + /// create the elements it needs to rebuild its object. + /// + internal HtmlDom DestinationDom => _destinationDom; + + /// + /// Imports the object whose lead row we are on, and every row that belongs to it, + /// by handing the whole family to the kind that claims the lead row's label. Leaves + /// _currentRowIndex on the last row of the family, since the main loop advances + /// past that. + /// + /// The object the rows are for is found the same way a [page content] row finds its + /// translation group: an object of a registered kind is one of the page's blocks, + /// so the lead row advances onto the page it belongs to (which for a page whose + /// only content is that object is the only thing that could) and takes that page's + /// next unused object. A page that has no object left for the row gets a warning + /// naming the row, and the whole family is skipped, leaving the page as it was. + /// + /// The kind whose LeadRowLabel this row carries. + /// The page type named in this row's [page type] cell, which + /// export writes on the first row it makes for a page. As for a [page content] row, + /// a page type means "start a new page of that type". + private async Task ImportObjectAsync(ISpreadsheetObjectKind kind, string pageType) + { + var rows = CollectRowFamily(kind); + var firstRowIndex = _currentRowIndex; + var lastRowIndex = _currentRowIndex + rows.Count - 1; + var leadRowLabel = kind.LeadRowLabel; + + if (_sheet.GetColumnForTag(InternalSpreadsheet.DetailsColumnLabel) < 0) + { + // Nothing but the [details] column can tell us what the object looks like, + // so there is nothing we can do with these rows but leave the book's own + // object alone. (Export always writes the column when it writes such a row, + // so this means the spreadsheet was edited into this state.) + Warn( + $"Row {CurrentRowIndexForMessages} is a {leadRowLabel} row, but this spreadsheet has no {InternalSpreadsheet.DetailsColumnLabel} column, so Bloom could not use it." + ); + _currentRowIndex = lastRowIndex; + return; + } + + // Check first that advancing could actually find an object of this kind. Bloom + // has no default page that holds one, and it must not invent one, so if there + // is none to be had we say so and skip rather than advancing off the end of the + // book and adding a page that still has no object for us. + if (!AnObjectOfKindIsStillAvailable(kind)) + { + WarnNoObjectFor(leadRowLabel); + _currentRowIndex = lastRowIndex; + return; + } + + // This is what gets us onto the page the object belongs to, and picks which of + // that page's objects this row is for. + var typesFound = AdvanceToNextSetOfBlocks(BlockTypes.Object, pageType); + var target = + (typesFound & BlockTypes.Object) == BlockTypes.Object + ? _blocksOnPage[objectIndex][_blockOnPageIndexes[objectIndex]] + : null; + // With more than one kind sharing the object slot we could land on an object + // belonging to another kind; better to say we found nothing than to hand a kind + // something it does not understand. + if (target != null && !kind.GetObjectsOnPage(_currentPage).Contains(target)) + target = null; + if (target == null) + { + WarnNoObjectFor(leadRowLabel); + _currentRowIndex = lastRowIndex; + return; + } + + await kind.ImportObjectAsync( + rows, + new SpreadsheetObjectImportContext + { + Importer = this, + Spreadsheet = _sheet, + TargetElement = target, + Warn = Warn, + // So that a message about one row of the family names that row. + SetRowInFamilyBeingProcessed = i => _currentRowIndex = firstRowIndex + i, + } + ); + _currentRowIndex = lastRowIndex; + } + + /// + /// Tells the user that the page had nowhere to put the object a lead row describes. + /// + private void WarnNoObjectFor(string leadRowLabel) + { + Warn( + $"Row {CurrentRowIndexForMessages} is a {leadRowLabel} row, but Bloom found nowhere on the page to put it, so it was skipped." + ); + } + + /// + /// The run of rows that belong to the lead row we are on: itself, then every + /// following row the kind claims as a continuation. + /// + private List CollectRowFamily(ISpreadsheetObjectKind kind) + { + var rows = new List { _inputRows[_currentRowIndex] }; + for (var i = _currentRowIndex + 1; i < _inputRows.Count; i++) + { + if (!kind.IsContinuationRow(_inputRows[i])) + break; + rows.Add(_inputRows[i]); + } + return rows; + } + + /// + /// Whether there is still an object of this kind somewhere for another of its lead + /// rows to fill: one on the current page that we have not used yet, one on a page we + /// have not reached, or one on the last content page, since a copy of that page is + /// what import adds when it runs out of pages. + /// + private bool AnObjectOfKindIsStillAvailable(ISpreadsheetObjectKind kind) + { + var objectsOnCurrentPage = _blocksOnPage[objectIndex]; + if (objectsOnCurrentPage != null && _currentPage != null) + { + var ofThisKind = kind.GetObjectsOnPage(_currentPage); + for ( + var i = Math.Max(_blockOnPageIndexes[objectIndex] + 1, 0); + i < objectsOnCurrentPage.Count; + i++ + ) + { + if (ofThisKind.Contains(objectsOnCurrentPage[i])) + return true; + } + } + for (var i = Math.Max(_currentPageIndex + 1, 0); i < _pages.Count; i++) + { + if (kind.GetObjectsOnPage(_pages[i]).Count > 0) + return true; + } + return _lastContentPage != null && kind.GetObjectsOnPage(_lastContentPage).Count > 0; + } + private List GetBloomCanvases(SafeXmlElement ancestor) { return SafeSelectNodesByClassName(ancestor, ".//div", "bloom-canvas").ToList(); @@ -1720,15 +1928,31 @@ private void CollectElementsFromPage( List[] blocksOnPageCollector ) { - blocksOnPageCollector[bloomCanvasIndex] = GetBloomCanvases(currentPage); + // Anything inside an object of a registered ISpreadsheetObjectKind belongs to + // that object, which is filled from the object's own rows, so it must not be a + // destination for the page's positionally-matched [page content] rows. + blocksOnPageCollector[bloomCanvasIndex] = GetBloomCanvases(currentPage) + .Where(x => !SpreadsheetObjectKinds.IsInsideAnObject(x)) + .ToList(); // We don't want image description slots as possible destinations for text. // They are handled by special extra rows inserted after the row that has the image. var allGroups = TranslationGroupManager.SortedGroupsOnPage(currentPage, true); blocksOnPageCollector[translationGroupIndex] = allGroups - .Where(x => !HasExactClassName(x, "bloom-imageDescription")) + .Where(x => + !HasExactClassName(x, "bloom-imageDescription") + && !SpreadsheetObjectKinds.IsInsideAnObject(x) + ) + .ToList(); + blocksOnPageCollector[videoContainerIndex] = GetVideoContainers(currentPage) + .Where(x => !SpreadsheetObjectKinds.IsInsideAnObject(x)) + .ToList(); + blocksOnPageCollector[widgetContainerIndex] = GetWidgetContainers(currentPage) + .Where(x => !SpreadsheetObjectKinds.IsInsideAnObject(x)) + .ToList(); + blocksOnPageCollector[objectIndex] = SpreadsheetObjectKinds + .ObjectsOnPage(currentPage) + .Select(o => o.Element) .ToList(); - blocksOnPageCollector[videoContainerIndex] = GetVideoContainers(currentPage); - blocksOnPageCollector[widgetContainerIndex] = GetWidgetContainers(currentPage); } // This helper method supports various tasks that have to be done for each block type @@ -1824,7 +2048,7 @@ public static bool IsEmptyCell(string content) /// /// /// - private async Task PutRowInGroupAsync(ContentRow row, SafeXmlElement group) + internal async Task PutRowInGroupAsync(ContentRow row, SafeXmlElement group) { if (HasExactClassName(group, "QuizAnswer-style")) { @@ -2378,7 +2602,8 @@ enum BlockTypes Image = 1 << SpreadsheetImporter.bloomCanvasIndex, Video = 1 << SpreadsheetImporter.videoContainerIndex, Widget = 1 << SpreadsheetImporter.widgetContainerIndex, - All = 15, // deliberately not including landscape! + Object = 1 << SpreadsheetImporter.objectIndex, + All = 31, // deliberately not including landscape! // This is special. A combination of the above flags may be used as an index // to look up a page guid that should be inserted when we need that combination diff --git a/src/BloomExe/Spreadsheet/SpreadsheetObjectKind.cs b/src/BloomExe/Spreadsheet/SpreadsheetObjectKind.cs new file mode 100644 index 000000000000..53b83f587903 --- /dev/null +++ b/src/BloomExe/Spreadsheet/SpreadsheetObjectKind.cs @@ -0,0 +1,272 @@ +using System; +using System.Collections.Generic; +using System.Drawing; +using System.Linq; +using System.Threading.Tasks; +using Bloom.SafeXml; + +namespace Bloom.Spreadsheet +{ + /// + /// A kind of thing on a page that the ordinary [page content] machinery cannot carry, + /// because putting it back needs more than the text, image, video and widget columns + /// hold. Such a thing gets rows of its own: a lead row with a row label of its own, + /// whose hidden [details] cell holds a JSON object whose "kind" property is this + /// kind's , optionally followed by more rows that belong to the + /// same thing. + /// + /// Registering a kind (see ) is all it + /// takes to make the exporter and importer handle those rows: export asks each + /// registered kind what the page holds and writes each object's rows at the object's + /// own position among the page's [page content] rows, and import collects each lead + /// row's family, finds the object on the target page, and hands both to the kind. + /// + public interface ISpreadsheetObjectKind + { + /// + /// The value the "kind" property of the [details] JSON carries for this kind's + /// rows. It identifies a [details] cell even apart from the row it sits in. + /// + string Kind { get; } + + /// + /// The row label of the lead row of one object of this kind, in the bracketed form + /// every row label takes. No two registered kinds may share one. + /// + string LeadRowLabel { get; } + + /// + /// True if this row continues the family of rows led by a + /// row of this kind, rather than starting something else. Asked of the rows after a + /// lead row, in order, and of no more rows once it has answered false. + /// + bool IsContinuationRow(ContentRow row); + + /// + /// The objects of this kind that the page holds in its own right, in document + /// order. Objects nested inside another object of this kind are not included: they + /// belong to the object that holds them, not to the page. A kind whose objects hang + /// off something else rather than sitting on the page returns an empty list, and + /// then it is up to the kind to get its rows written and read. + /// + List GetObjectsOnPage(SafeXmlElement page); + + /// + /// True if the element is inside one of this kind's objects, in which case it + /// belongs to that object rather than to the page: the exporter's and importer's + /// generic collectors of translation groups, bloom-canvases, video containers and + /// widget containers all leave such elements alone, since the object's own rows + /// carry them. + /// + bool IsInsideObject(SafeXmlElement element); + + /// + /// Writes the rows for one of the objects returned. + /// The first row written must have in its row-type cell + /// and a [details] cell whose JSON says "kind": . The [details] + /// column already exists by the time this is called. + /// + void ExportObject(SafeXmlElement obj, SpreadsheetObjectExportContext context); + + /// + /// Puts one family of rows (its lead row first, then whatever + /// claimed) back into the book, into or in place of + /// . + /// + Task ImportObjectAsync(List rows, SpreadsheetObjectImportContext context); + } + + /// + /// What needs beyond the object + /// itself. Anything else it wants is on the . + /// + public class SpreadsheetObjectExportContext + { + /// The exporter doing this export. + public SpreadsheetExporter Exporter { get; set; } + + /// The spreadsheet being built; make rows with new ContentRow(this). + public InternalSpreadsheet Spreadsheet { get; set; } + + /// The page number to put in the [page number] cell of each row made. + public string PageNumber { get; set; } + + /// + /// The background color of this page's rows. Rows made for an object should get it + /// too, since they are part of the export of the same chunk of the document. + /// + public Color ColorForPage { get; set; } + + /// The folder of the book being exported, for resolving image and video paths. + public string BookFolderPath { get; set; } + + /// + /// Call this with the first row made for the object. If the page's type has not + /// been written yet it goes on that row; otherwise this does nothing. Export puts + /// the page type on the first row it makes for a page, whatever kind of row it is, + /// so that later rows can go onto the same page if there is room. + /// + public Action SetPageTypeIfNeeded { get; set; } + } + + /// + /// What needs beyond the rows. + /// Anything else it wants is on the . + /// + public class SpreadsheetObjectImportContext + { + /// The importer doing this import. + public SpreadsheetImporter Importer { get; set; } + + /// The spreadsheet being imported. + public InternalSpreadsheet Spreadsheet { get; set; } + + /// + /// The object on the target page that this family of rows is for: the next object + /// of this kind on the page the rows belong to, found the same way a [page content] + /// row finds its translation group. Never null; import reports and skips a family + /// for which no object could be found rather than calling the kind. + /// + public SafeXmlElement TargetElement { get; set; } + + /// Reports a problem to the user. The message should name the row it is about. + public Action Warn { get; set; } + + /// + /// Say which row of the family is being worked on (0 for the lead row) so that + /// names that row. + /// + public Action SetRowInFamilyBeingProcessed { get; set; } + } + + /// + /// One object found on a page, paired with the kind that owns it, in the form the + /// exporter needs to place its rows. + /// + public class SpreadsheetObjectOnPage + { + /// The kind that reported this object. + public ISpreadsheetObjectKind Kind { get; set; } + + /// The element on the page. + public SafeXmlElement Element { get; set; } + } + + /// + /// The registered s, and the questions the exporter + /// and importer ask of all of them at once. + /// + /// Registration is static because the kinds are fixed features of Bloom rather than + /// per-book or per-export choices; a kind registers itself once at startup. Tests that + /// register a kind must it again, or they will change what + /// every later test in the process sees. + /// + public static class SpreadsheetObjectKinds + { + private static readonly List _kinds = + new List(); + + /// + /// Makes a kind known to the exporter and importer. Its + /// must not already be taken. + /// + public static void Register(ISpreadsheetObjectKind kind) + { + if (kind == null) + throw new ArgumentNullException(nameof(kind)); + if (ForLeadRowLabel(kind.LeadRowLabel) != null) + throw new ArgumentException( + $"A spreadsheet object kind using the row label {kind.LeadRowLabel} is already registered." + ); + _kinds.Add(kind); + } + + /// + /// Forgets a kind. Returns whether it was registered. Mainly for tests. + /// + public static bool Unregister(ISpreadsheetObjectKind kind) + { + return _kinds.Remove(kind); + } + + /// Every registered kind, in registration order. + public static IReadOnlyList All => _kinds; + + /// + /// The kind whose lead row carries this row label, or null if no registered kind + /// does (which is the case for every row label in a spreadsheet made before any + /// kind existed). + /// + public static ISpreadsheetObjectKind ForLeadRowLabel(string rowLabel) + { + return _kinds.FirstOrDefault(k => k.LeadRowLabel == rowLabel); + } + + /// + /// The kind that would have claimed this row as a continuation of one of its + /// families, or null. Used to tell the user that such a row was stranded away from + /// the lead row that gives it meaning. + /// + public static ISpreadsheetObjectKind ThatWouldContinueWith(ContentRow row) + { + return _kinds.FirstOrDefault(k => k.IsContinuationRow(row)); + } + + /// + /// True if the element belongs to an object of some registered kind, so that the + /// generic collectors of a page's translation groups, bloom-canvases, video + /// containers and widget containers must leave it alone. + /// + public static bool IsInsideAnObject(SafeXmlElement element) + { + // Fast path for the ordinary case of a book with none of these objects in it. + if (_kinds.Count == 0) + return false; + return _kinds.Any(k => k.IsInsideObject(element)); + } + + /// + /// Every registered kind's objects on the page, in document order, so that the + /// exporter can write each object's rows at the object's own position among the + /// page's [page content] rows. + /// + public static List ObjectsOnPage(SafeXmlElement page) + { + if (_kinds.Count == 0) + return new List(); + var found = _kinds + .SelectMany(k => + k.GetObjectsOnPage(page) + .Select(e => new SpreadsheetObjectOnPage { Kind = k, Element = e }) + ) + .ToList(); + if (found.Count < 2) + return found; + var documentOrder = GetDocumentOrder(page); + return found + .OrderBy(o => + documentOrder.TryGetValue(o.Element, out var order) ? order : int.MaxValue + ) + .ToList(); + } + + /// + /// Every element at or under the page, numbered in document order, so that two + /// elements found by different searches can be told which comes first. + /// + public static Dictionary GetDocumentOrder(SafeXmlElement page) + { + var result = new Dictionary(); + var next = 0; + void Walk(SafeXmlNode node) + { + if (node is SafeXmlElement element) + result[element] = next++; + foreach (var child in node.ChildNodes) + Walk(child); + } + Walk(page); + return result; + } + } +} diff --git a/src/BloomTests/Spreadsheet/SpreadsheetDetailsTests.cs b/src/BloomTests/Spreadsheet/SpreadsheetDetailsTests.cs new file mode 100644 index 000000000000..2b0b781832a1 --- /dev/null +++ b/src/BloomTests/Spreadsheet/SpreadsheetDetailsTests.cs @@ -0,0 +1,587 @@ +using System.Collections.Generic; +using System.Linq; +using System.Threading.Tasks; +using Bloom.Book; +using Bloom.SafeXml; +using Bloom.Spreadsheet; +using Moq; +using Newtonsoft.Json.Linq; +using NUnit.Framework; +using OfficeOpenXml; +using SIL.IO; +using SIL.TestUtilities; + +namespace BloomTests.Spreadsheet +{ + /// + /// Tests the generic mechanism that lets a thing on a page which the ordinary + /// [page content] machinery cannot carry get rows of its own in a spreadsheet: a lead + /// row with a row label of its own, carrying in the hidden [details] column the JSON + /// needed to put the thing back, optionally followed by more rows that belong to it. + /// + /// Nothing here knows what any real kind of such a thing is. The fixture registers a + /// stub kind of its own (see ) and checks what the exporter + /// and importer do around it: that the [details] column appears and is hidden, that an + /// object's rows land at the object's own position among the page's rows, that what is + /// inside the object is not also exported as page content, that a round trip restores + /// the object, that a page with nowhere to put the object gets a warning naming the row + /// and is otherwise imported normally, and that a spreadsheet with no [details] column + /// at all imports exactly as it did before any of this existed. + /// + public class SpreadsheetDetailsTests + { + static SpreadsheetDetailsTests() + { + // The package requires us to do this as a way of acknowledging that we + // accept the terms of the NonCommercial license. + ExcelPackage.LicenseContext = LicenseContext.NonCommercial; + } + + // What the stub kind's objects look like in a book: a div of its own class holding + // one translation group per part. The label is the bit of state that only [details] + // can carry, and the ampersand in it is deliberate: [details] holds JSON that must + // come back byte for byte, so it must not be XML-escaped on the way through a file. + private const string stubObjectLabel = "Wheels & Cogs"; + + // The same label as it has to be written in the book's XHTML. + private const string stubObjectLabelEscaped = "Wheels & Cogs"; + + private static string StubObject(string firstPart, string secondPart) + { + return $@"
+ {TranslationGroup(firstPart + "-es", firstPart + "-en")} + {TranslationGroup(secondPart + "-es", secondPart + "-en")} +
"; + } + + private static string TranslationGroup(string spanish, string english) + { + return $@"
+

{spanish}

+

+

{english}

+
"; + } + + /// + /// The test book: a heading group, then whatever markup the caller wants in the + /// middle, then a trailing group. Putting the object between two ordinary groups is + /// what lets us see whether its rows land in the right place. + /// + private static string MakeBook(string middleMarkup) + { + return @" + + + + + + + +
+
+

Details round trip

+
+
+
+
+ Just Text +
+ +
+ +
+
+" + + TranslationGroup("Encabezado", "Heading") + + middleMarkup + + TranslationGroup("Pie", "Footing") + + @" +
+
+
+ + +"; + } + + private StubObjectKind _kind; + private InternalSpreadsheet _sheetFromExport; + private HtmlDom _emptyObjectDom; // imported into a book whose object has no content + private List _warningsForBookWithoutObject; + private HtmlDom _domWithoutObject; + + private static InternalSpreadsheet ExportBook(string bookHtml) + { + var mockLangDisplayNameResolver = new Mock(); + mockLangDisplayNameResolver + .Setup(x => x.GetLanguageDisplayName("en")) + .Returns("English"); + mockLangDisplayNameResolver + .Setup(x => x.GetLanguageDisplayName("es")) + .Returns("Spanish"); + var exporter = new SpreadsheetExporter(mockLangDisplayNameResolver.Object); + exporter.Params = new SpreadsheetExportParams(); + return exporter.Export(new HtmlDom(bookHtml, true), "fakeImagesFolderpath"); + } + + /// + /// Writes the sheet to a real .xlsx and reads it back before importing: only the + /// written file has the language cells flattened to text the way a real import + /// sees them, and only it can show us whether the [details] cell survived + /// unescaped. + /// + private static async Task> RoundTripThroughFileAndImportAsync( + InternalSpreadsheet sheetFromExport, + params HtmlDom[] targets + ) + { + using (var tempFile = TempFile.WithExtension("xlsx")) + { + sheetFromExport.WriteToFile(tempFile.Path); + var sheet = InternalSpreadsheet.ReadFromFile(tempFile.Path); + var warnings = new List(); + foreach (var target in targets) + warnings.AddRange( + await new TestSpreadsheetImporter(null, target).ImportAsync(sheet) + ); + return warnings; + } + } + + [OneTimeSetUp] + public async Task OneTimeSetUp() + { + // Registration is process-wide, so OneTimeTearDown must undo it or every later + // spreadsheet test would see this stub kind. + _kind = new StubObjectKind(); + SpreadsheetObjectKinds.Register(_kind); + + var bookWithObject = MakeBook(StubObject("Uno", "Dos")); + var origDom = new HtmlDom(bookWithObject, true); + _emptyObjectDom = new HtmlDom(MakeBook(StubObject("", "")), true); + _domWithoutObject = new HtmlDom(MakeBook(""), true); + + // Sanity: the test books are what we think they are before the round trip. + AssertThatXmlIn + .Dom(origDom.RawDom) + .HasSpecifiedNumberOfMatchesForXpath("//div[contains(@class,'stub-object')]", 1); + Assert.That( + origDom.RawDom.InnerXml, + Does.Contain("Uno-es"), + "sanity: the exported book has the object's text" + ); + Assert.That( + _emptyObjectDom.RawDom.InnerXml, + Does.Not.Contain("Uno-es"), + "sanity: the empty-object target has none of the object's text of its own" + ); + AssertThatXmlIn + .Dom(_domWithoutObject.RawDom) + .HasNoMatchForXpath("//div[contains(@class,'stub-object')]"); + + _sheetFromExport = ExportBook(bookWithObject); + await RoundTripThroughFileAndImportAsync(_sheetFromExport, _emptyObjectDom); + _warningsForBookWithoutObject = await RoundTripThroughFileAndImportAsync( + _sheetFromExport, + _domWithoutObject + ); + } + + [OneTimeTearDown] + public void OneTimeTearDown() + { + if (_kind != null) + Assert.That( + SpreadsheetObjectKinds.Unregister(_kind), + Is.True, + "the stub kind should still have been registered at teardown" + ); + } + + [Test] + public void Export_MakesTheDetailsColumn_AndHidesIt() + { + var detailsColumn = _sheetFromExport.GetColumnForTag( + InternalSpreadsheet.DetailsColumnLabel + ); + Assert.That( + detailsColumn, + Is.GreaterThanOrEqualTo(0), + $"the export should have made a {InternalSpreadsheet.DetailsColumnLabel} column" + ); + Assert.That( + _sheetFromExport.HiddenColumns, + Does.Contain(detailsColumn), + $"the {InternalSpreadsheet.DetailsColumnLabel} column holds machine-made state and should be hidden" + ); + } + + [Test] + public void Export_PutsTheObjectsRowsAtTheObjectsPosition() + { + var rowLabels = _sheetFromExport.ContentRows.Select(r => r.MetadataKey).ToList(); + Assert.That( + rowLabels, + Is.EqualTo( + new[] + { + "[book title]", // from the data div, before any page + InternalSpreadsheet.PageContentRowLabel, // the heading group + StubObjectKind.LeadLabel, + StubObjectKind.PartLabel, + StubObjectKind.PartLabel, + InternalSpreadsheet.PageContentRowLabel, // the trailing group + } + ), + "the object's rows belong between the rows of the groups it sits between" + ); + } + + [Test] + public void Export_PutsTheKindAndItsStateInTheLeadRowsDetailsCell() + { + var leadRow = _sheetFromExport.ContentRows.First(r => + r.MetadataKey == StubObjectKind.LeadLabel + ); + var details = JObject.Parse( + leadRow.GetCell(InternalSpreadsheet.DetailsColumnLabel).Content + ); + Assert.That( + details["kind"]?.ToString(), + Is.EqualTo(StubObjectKind.KindName), + "a [details] cell should say what kind of thing it is for" + ); + Assert.That(details["label"]?.ToString(), Is.EqualTo(stubObjectLabel)); + } + + [Test] + public void Export_DoesNotAlsoWriteTheObjectsTextAsPageContent() + { + var spanishColumn = _sheetFromExport.GetRequiredColumnForLang("es"); + var pageContentRows = _sheetFromExport + .ContentRows.Where(r => r.MetadataKey == InternalSpreadsheet.PageContentRowLabel) + .ToList(); + Assert.That( + pageContentRows.Count, + Is.EqualTo(2), + "only the two groups outside the object are page content" + ); + Assert.That( + pageContentRows.Select(r => r.GetCell(spanishColumn).Content), + Is.EquivalentTo(new[] { "

Encabezado

", "

Pie

" }), + "the groups inside the object belong to it, not to the page" + ); + + // Sanity: the object's text really was exported, just not as page content. + var partRows = _sheetFromExport + .ContentRows.Where(r => r.MetadataKey == StubObjectKind.PartLabel) + .ToList(); + Assert.That( + partRows.Select(r => r.GetCell(spanishColumn).Content), + Is.EqualTo(new[] { "

Uno-es

", "

Dos-es

" }) + ); + } + + [Test] + public void Import_RestoresTheObjectsTextIntoAnEmptyObject() + { + var editables = _emptyObjectDom + .SafeSelectNodes( + "//div[contains(@class,'stub-object')]//div[contains(@class,'bloom-editable') and @lang='es']" + ) + .Cast() + .Select(e => e.InnerText.Trim()) + .ToList(); + Assert.That( + editables, + Is.EqualTo(new[] { "Uno-es", "Dos-es" }), + "the spreadsheet is the authority on what is inside the object" + ); + } + + [Test] + public void Import_RestoresTheStateThatOnlyTheDetailsCellCarries() + { + var stubObject = _emptyObjectDom + .SafeSelectNodes("//div[contains(@class,'stub-object')]") + .Cast() + .Single(); + Assert.That( + stubObject.GetAttribute("data-label"), + Is.EqualTo(stubObjectLabel), + "an ampersand in a [details] cell must survive the file unescaped" + ); + } + + [Test] + public void Import_StillPutsThePagesOwnContentOnThePage() + { + Assert.That( + _emptyObjectDom.RawDom.InnerXml, + Does.Contain("Encabezado").And.Contain("Pie"), + "the rows around the object should import as they always did" + ); + } + + [Test] + public void Import_WhenThePageHasNowhereToPutTheObject_ReportsAndSkips() + { + Assert.That( + _warningsForBookWithoutObject, + Has.Exactly(1).Contains(StubObjectKind.LeadLabel), + "the user should be told that the object's rows could not be used" + ); + var warning = _warningsForBookWithoutObject.First(w => + w.Contains(StubObjectKind.LeadLabel) + ); + var leadRowNumber = + _sheetFromExport.GetIndexOfRow( + _sheetFromExport.ContentRows.First(r => + r.MetadataKey == StubObjectKind.LeadLabel + ) + ) + 1; + Assert.That( + leadRowNumber, + Is.GreaterThan(1), + "sanity: the lead row is not the first row of the sheet" + ); + Assert.That( + warning, + Does.StartWith($"Row {leadRowNumber} is"), + "the warning should name the spreadsheet row the user can go look at" + ); + } + + [Test] + public void Import_WhenThePageHasNowhereToPutTheObject_ImportsTheRestOfThePage() + { + AssertThatXmlIn + .Dom(_domWithoutObject.RawDom) + .HasNoMatchForXpath("//div[contains(@class,'stub-object')]"); + Assert.That( + _domWithoutObject.RawDom.InnerXml, + Does.Contain("Encabezado").And.Contain("Pie"), + "skipping the object must not cost the page its own content" + ); + Assert.That( + _domWithoutObject.RawDom.InnerXml, + Does.Not.Contain("Uno-es"), + "there was nowhere to put the object, so its text must not have leaked onto the page" + ); + } + + [Test] + public async Task Import_OfASheetWithNoDetailsColumn_WorksAsItAlwaysDid() + { + var sheet = ExportBook(MakeBook("")); + + // Sanity: a book with none of these objects gets no [details] column at all, + // which is exactly the shape of every spreadsheet made before the column existed. + Assert.That( + sheet.GetColumnForTag(InternalSpreadsheet.DetailsColumnLabel), + Is.LessThan(0), + "nothing needed the column, so it should not have been made" + ); + Assert.That( + sheet.ContentRows.Select(r => r.MetadataKey).ToList(), + Is.EqualTo( + new[] + { + "[book title]", + InternalSpreadsheet.PageContentRowLabel, + InternalSpreadsheet.PageContentRowLabel, + } + ) + ); + + var target = new HtmlDom(MakeBook(""), true); + var warnings = await RoundTripThroughFileAndImportAsync(sheet, target); + Assert.That(warnings, Is.Empty, string.Join("; ", warnings)); + Assert.That(target.RawDom.InnerXml, Does.Contain("Encabezado").And.Contain("Pie")); + } + + [Test] + public async Task Import_OfAContinuationRowWithNoLeadRow_ReportsIt() + { + // A hand-made sheet holding just one part row, as a user would leave behind by + // deleting or moving the lead row that gave it meaning. + var sheet = new InternalSpreadsheet(); + var spanishColumn = sheet.AddColumnForLang("es", "Spanish"); + var strandedRow = new ContentRow(sheet); + strandedRow.SetCell(InternalSpreadsheet.RowTypeColumnLabel, StubObjectKind.PartLabel); + strandedRow.SetCell(spanishColumn, "

Orphan

"); + + // Sanity: the sheet is the shape we meant to make. + Assert.That( + sheet.ContentRows.Select(r => r.MetadataKey).ToList(), + Is.EqualTo(new[] { StubObjectKind.PartLabel }) + ); + + var target = new HtmlDom(MakeBook(StubObject("", "")), true); + var warnings = await new TestSpreadsheetImporter(null, target).ImportAsync(sheet); + Assert.That( + warnings, + Has.Exactly(1).Contains(StubObjectKind.LeadLabel), + "the warning should say which lead row the stranded row needed" + ); + Assert.That( + target.RawDom.InnerXml, + Does.Not.Contain("Orphan"), + "a stranded row's text has no place we could know of, so it must be dropped" + ); + } + + /// + /// A minimal ISpreadsheetObjectKind, existing only to exercise the generic + /// mechanism. Its objects are divs of class "stub-object" holding one translation + /// group per part; its lead row carries the object's data-label attribute (the bit + /// of state no ordinary column could hold) in [details], and each part gets a row + /// whose language columns hold that part's text. + /// + private class StubObjectKind : ISpreadsheetObjectKind + { + /// The class marking one of this kind's objects in a book. + public const string ObjectClass = "stub-object"; + + /// The "kind" this kind's [details] cells claim. + public const string KindName = "stub"; + + /// The row label of an object's lead row. + public const string LeadLabel = "[stub object]"; + + /// The row label of one part of an object. + public const string PartLabel = "[stub part]"; + + /// The "kind" of a part row's [details] cell. + public const string PartKindName = "stub-part"; + + /// See . + public string Kind => KindName; + + /// See . + public string LeadRowLabel => LeadLabel; + + /// A part row, and nothing else, continues an object's family. + public bool IsContinuationRow(ContentRow row) + { + return row.MetadataKey == PartLabel; + } + + /// + /// The stub objects the page holds in its own right. (Nesting is not something + /// this stub does, so every one it finds is one of the page's own.) + /// + public List GetObjectsOnPage(SafeXmlElement page) + { + return page.SafeSelectNodes($".//div[contains(@class,'{ObjectClass}')]") + .Cast() + .ToList(); + } + + /// + /// True if the element has a stub object among its ancestors, so that it belongs + /// to that object rather than to the page. + /// + public bool IsInsideObject(SafeXmlElement element) + { + for ( + var ancestor = element.ParentNode as SafeXmlElement; + ancestor != null; + ancestor = ancestor.ParentNode as SafeXmlElement + ) + { + if (ancestor.HasClass(ObjectClass)) + return true; + } + return false; + } + + /// + /// Writes the object's lead row, whose [details] cell holds the label, then one + /// part row per translation group inside it, each carrying that group's text in + /// the ordinary language columns. + /// + public void ExportObject(SafeXmlElement obj, SpreadsheetObjectExportContext context) + { + var leadRow = new ContentRow(context.Spreadsheet); + context.SetPageTypeIfNeeded(leadRow); + leadRow.SetCell(InternalSpreadsheet.RowTypeColumnLabel, LeadLabel); + leadRow.SetCell(InternalSpreadsheet.PageNumberColumnLabel, context.PageNumber); + leadRow.SetCell( + InternalSpreadsheet.DetailsColumnLabel, + new JObject + { + ["kind"] = KindName, + ["label"] = obj.GetAttribute("data-label"), + }.ToString(Newtonsoft.Json.Formatting.None) + ); + leadRow.BackgroundColor = context.ColorForPage; + + var groups = GroupsOf(obj); + for (var i = 0; i < groups.Count; i++) + { + var partRow = new ContentRow(context.Spreadsheet); + partRow.SetCell(InternalSpreadsheet.RowTypeColumnLabel, PartLabel); + partRow.SetCell(InternalSpreadsheet.PageNumberColumnLabel, context.PageNumber); + partRow.SetCell( + InternalSpreadsheet.DetailsColumnLabel, + new JObject { ["kind"] = PartKindName, ["index"] = i }.ToString( + Newtonsoft.Json.Formatting.None + ) + ); + partRow.BackgroundColor = context.ColorForPage; + context.Exporter.WriteTranslationGroup( + groups[i], + partRow, + context.BookFolderPath + ); + } + } + + /// + /// Puts the label back on the target object and each part row's text into the + /// group at that part's index, warning about a part row that has no group. + /// + public async Task ImportObjectAsync( + List rows, + SpreadsheetObjectImportContext context + ) + { + var details = JObject.Parse( + rows[0].GetCell(InternalSpreadsheet.DetailsColumnLabel).Content + ); + context.TargetElement.SetAttribute( + "data-label", + details["label"]?.ToString() ?? "" + ); + var groups = GroupsOf(context.TargetElement); + for (var i = 1; i < rows.Count; i++) + { + context.SetRowInFamilyBeingProcessed(i); + var index = + (int?) + JObject.Parse( + rows[i].GetCell(InternalSpreadsheet.DetailsColumnLabel).Content + )["index"] ?? -1; + if (index < 0 || index >= groups.Count) + { + context.Warn( + $"Row {context.Importer.CurrentRowIndexForMessages} is a {PartLabel} row for a part that is not there, so it was skipped." + ); + continue; + } + await context.Importer.PutRowInGroupAsync(rows[i], groups[index]); + } + } + + /// The translation groups that are the object's parts. + private static List GroupsOf(SafeXmlElement obj) + { + return obj.SafeSelectNodes(".//div[contains(@class,'bloom-translationGroup')]") + .Cast() + .ToList(); + } + } + } +} From c1b68549800dd84a3317de9faa24cdf2ce59f744 Mon Sep 17 00:00:00 2001 From: Hatton Date: Fri, 4 Sep 2026 15:15:06 -0600 Subject: [PATCH 2/5] Spreadsheet import: warn once, not twice, when a lead row's page type has no object When an object's lead row names a page type whose template page holds no object of the kind, InsertDefaultPageIfNeeded warned that the page type "contains no data suitable for that page type", found no default page for the object, and then recursed with nothing left needed. The recursive call ran the same page-type branch again and repeated the warning for the same row. Now, when the object was all the row needed, it returns false at once and uses the page it has; ImportObjectAsync then reports and skips the row as before. Co-Authored-By: Claude Fable 5.1 --- src/BloomExe/Spreadsheet/SpreadsheetImporter.cs | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/BloomExe/Spreadsheet/SpreadsheetImporter.cs b/src/BloomExe/Spreadsheet/SpreadsheetImporter.cs index 9705a4bd644f..d4cefda0355e 100644 --- a/src/BloomExe/Spreadsheet/SpreadsheetImporter.cs +++ b/src/BloomExe/Spreadsheet/SpreadsheetImporter.cs @@ -1711,8 +1711,14 @@ string pageTypeWeHave && (blocksNeeded & BlockTypes.Object) == BlockTypes.Object ) { + var blocksNeededBesidesObject = blocksNeeded & ~BlockTypes.Object; + // If the object was all the row needed, there is nothing left to find a page + // for, so we use the page we have. (Recursing with nothing needed would only + // repeat the "requested page type" warning above for the same row.) + if (blocksNeededBesidesObject == BlockTypes.None) + return false; return InsertDefaultPageIfNeeded( - blocksNeeded & ~BlockTypes.Object, + blocksNeededBesidesObject, blocksWeHave, pageTypeNeeded, _pageTypeOfLastPage From ce98bf982c8b58ea1c55c536796ee2c789477ac7 Mon Sep 17 00:00:00 2001 From: Hatton Date: Fri, 4 Sep 2026 15:28:01 -0600 Subject: [PATCH 3/5] Spreadsheet: read the [details] cell back without decoding Excel escapes The [details] column holds JSON that must come back byte for byte, and the read path already skipped XML escaping for it. But it still ran the cell through the decoding of Excel's _xNNNN_ character escapes, so text in the JSON that merely looked like such an escape ("_x0041_" in a label, say) came back as the character it named. Excel needs those escapes only for characters XML cannot hold, and JSON never holds one raw, so the details cell now gets neither treatment. A test writes such a cell to a real .xlsx and reads it back unchanged. Co-Authored-By: Claude Fable 5.1 --- src/BloomExe/Spreadsheet/SpreadsheetIO.cs | 32 ++++++++++++---- .../Spreadsheet/SpreadsheetDetailsTests.cs | 38 +++++++++++++++++++ 2 files changed, 63 insertions(+), 7 deletions(-) diff --git a/src/BloomExe/Spreadsheet/SpreadsheetIO.cs b/src/BloomExe/Spreadsheet/SpreadsheetIO.cs index ba43e95cbdbd..5e86eef3d899 100644 --- a/src/BloomExe/Spreadsheet/SpreadsheetIO.cs +++ b/src/BloomExe/Spreadsheet/SpreadsheetIO.cs @@ -366,6 +366,21 @@ private static bool IsWysiwygFormattedPair(SpreadsheetRow row, int index) return IsWysiwygFormattedColumn(row, index) && IsWysiwygFormattedRow(row); } + /// + /// True for a cell of the [details] column, whose JSON we must get back byte for + /// byte. Such a cell gets neither XML escaping nor the decoding of Excel's _xNNNN_ + /// character escapes: text that merely looks like such an escape (say, "_x0041_" in + /// a label) must stay as it is, not become the character it would name. Excel needs + /// those escapes only for characters XML cannot hold, and JSON never holds one raw. + /// + private static bool IsCopiedVerbatim(SpreadsheetRow row, int index) + { + if (row.Spreadsheet.AllRows().Count() <= 1) + return false; // the header row itself is being read; see WantXmlEscaping + return row.Spreadsheet.Header.GetRow(0).GetCell(index).Content + == InternalSpreadsheet.DetailsColumnLabel; + } + private static bool WantXmlEscaping(SpreadsheetRow row, int index) { if (row.Spreadsheet.AllRows().Count() <= 1) @@ -443,13 +458,16 @@ SpreadsheetRow row } else { - var cellContent = worksheet.Cells[rowIndex + 1, c + 1].Value ?? ""; - row.AddCell( - ReplaceExcelEscapedCharsAndEscapeXmlOnes( - cellContent.ToString(), - WantXmlEscaping(row, c) - ) - ); + var cellContent = (worksheet.Cells[rowIndex + 1, c + 1].Value ?? "").ToString(); + if (IsCopiedVerbatim(row, c)) + row.AddCell(cellContent); + else + row.AddCell( + ReplaceExcelEscapedCharsAndEscapeXmlOnes( + cellContent, + WantXmlEscaping(row, c) + ) + ); } } } diff --git a/src/BloomTests/Spreadsheet/SpreadsheetDetailsTests.cs b/src/BloomTests/Spreadsheet/SpreadsheetDetailsTests.cs index 2b0b781832a1..870fbe17f3d3 100644 --- a/src/BloomTests/Spreadsheet/SpreadsheetDetailsTests.cs +++ b/src/BloomTests/Spreadsheet/SpreadsheetDetailsTests.cs @@ -401,6 +401,44 @@ public async Task Import_OfASheetWithNoDetailsColumn_WorksAsItAlwaysDid() Assert.That(target.RawDom.InnerXml, Does.Contain("Encabezado").And.Contain("Pie")); } + [Test] + public void ReadFromFile_LeavesTheDetailsCellExactlyAsWritten() + { + // The JSON in a [details] cell is copied verbatim, so text in it that happens to + // look like one of Excel's _xNNNN_ character escapes must come back as that + // text, not as the character it would name; and its markup characters must not + // be XML-escaped. (EPPlus itself still rewrites the doubly-escaped form + // _x005F_xNNNN_, which Bloom does not try to protect.) + const string details = "{\"kind\":\"stub\",\"label\":\"_x0041_ & \"}"; + var sheet = new InternalSpreadsheet(); + sheet.AddColumnForTag( + InternalSpreadsheet.DetailsColumnLabel, + InternalSpreadsheet.DetailsColumnFriendlyName + ); + var row = new ContentRow(sheet); + row.SetCell(InternalSpreadsheet.RowTypeColumnLabel, StubObjectKind.LeadLabel); + row.SetCell(InternalSpreadsheet.DetailsColumnLabel, details); + + // Sanity: the decoding that ordinary cells get would indeed change this text. + Assert.That( + SpreadsheetIO.ReplaceExcelEscapedCharsAndEscapeXmlOnes(details, false), + Is.Not.EqualTo(details) + ); + + using (var tempFile = TempFile.WithExtension("xlsx")) + { + sheet.WriteToFile(tempFile.Path); + var readBack = InternalSpreadsheet.ReadFromFile(tempFile.Path); + Assert.That( + readBack + .ContentRows.Single() + .GetCell(InternalSpreadsheet.DetailsColumnLabel) + .Content, + Is.EqualTo(details) + ); + } + } + [Test] public async Task Import_OfAContinuationRowWithNoLeadRow_ReportsIt() { From be8f773e1f0a309a424a92d5524eb803e815febe Mon Sep 17 00:00:00 2001 From: Hatton Date: Fri, 4 Sep 2026 15:47:40 -0600 Subject: [PATCH 4/5] Keep a skipped object family's place in the page order When a lead row's object cannot be found in the book, the importer used to skip the family without moving, so every row after it landed a page early and the page the family was meant for was thrown away as unused at the end of the import. Now the importer looks for the object the way it looks for any other block, before deciding to skip: a lead row that starts a page (it carries a page type, as export writes on the first row of every page) moves onto a new page whether or not an object is found there, so later rows still land on their own pages and no page is lost. A lead row in the middle of a page is still skipped in place, so that page keeps its layout. This removes the look-ahead that decided whether any object of the kind was still available; it is no longer needed. Tests cover an object-only page followed by a text page, an object-only spreadsheet, and the mid-page case. Co-Authored-By: Claude Fable 5.1 --- .../Spreadsheet/SpreadsheetImporter.cs | 97 +++++------- .../Spreadsheet/SpreadsheetDetailsTests.cs | 140 +++++++++++++++--- 2 files changed, 157 insertions(+), 80 deletions(-) diff --git a/src/BloomExe/Spreadsheet/SpreadsheetImporter.cs b/src/BloomExe/Spreadsheet/SpreadsheetImporter.cs index d4cefda0355e..3d00addad9f3 100644 --- a/src/BloomExe/Spreadsheet/SpreadsheetImporter.cs +++ b/src/BloomExe/Spreadsheet/SpreadsheetImporter.cs @@ -1781,8 +1781,18 @@ internal static bool HasExactClassName(SafeXmlElement element, string className) /// translation group: an object of a registered kind is one of the page's blocks, /// so the lead row advances onto the page it belongs to (which for a page whose /// only content is that object is the only thing that could) and takes that page's - /// next unused object. A page that has no object left for the row gets a warning - /// naming the row, and the whole family is skipped, leaving the page as it was. + /// next unused object. + /// + /// A family that has nowhere to go gets a warning naming its lead row and is + /// skipped. Where that leaves the importer depends on whether the lead row starts a + /// page. Export writes a page type on the first row it makes for a page, so a lead + /// row carrying one is the first row of its page, and it moves onto a new page + /// whether or not an object is found there: if it did not, every row after it would + /// land a page early, and the page it was meant for would be thrown away as unused + /// at the end of the import. (Past the end of the book this adds a copy of the last + /// page with nothing to put on it, as it does for any other row.) A lead row with no + /// page type belongs to the page we are on, so if that page has no object for it we + /// skip it in place and the page stays as it was. /// /// The kind whose LeadRowLabel this row carries. /// The page type named in this row's [page type] cell, which @@ -1795,6 +1805,22 @@ private async Task ImportObjectAsync(ISpreadsheetObjectKind kind, string pageTyp var lastRowIndex = _currentRowIndex + rows.Count - 1; var leadRowLabel = kind.LeadRowLabel; + // Find the object first, so that a family we end up skipping still holds its + // place in the page order (see the summary). Only a lead row that starts a page, + // or one whose page still has an unused object, moves the importer at all. + SafeXmlElement target = null; + if (!string.IsNullOrEmpty(pageType) || CurrentPageHasAnUnusedObject()) + { + var typesFound = AdvanceToNextSetOfBlocks(BlockTypes.Object, pageType); + if ((typesFound & BlockTypes.Object) == BlockTypes.Object) + target = _blocksOnPage[objectIndex][_blockOnPageIndexes[objectIndex]]; + // With more than one kind sharing the object slot we could land on an + // object belonging to another kind; better to say we found nothing than to + // hand a kind something it does not understand. + if (target != null && !kind.GetObjectsOnPage(_currentPage).Contains(target)) + target = null; + } + if (_sheet.GetColumnForTag(InternalSpreadsheet.DetailsColumnLabel) < 0) { // Nothing but the [details] column can tell us what the object looks like, @@ -1808,32 +1834,11 @@ private async Task ImportObjectAsync(ISpreadsheetObjectKind kind, string pageTyp return; } - // Check first that advancing could actually find an object of this kind. Bloom - // has no default page that holds one, and it must not invent one, so if there - // is none to be had we say so and skip rather than advancing off the end of the - // book and adding a page that still has no object for us. - if (!AnObjectOfKindIsStillAvailable(kind)) - { - WarnNoObjectFor(leadRowLabel); - _currentRowIndex = lastRowIndex; - return; - } - - // This is what gets us onto the page the object belongs to, and picks which of - // that page's objects this row is for. - var typesFound = AdvanceToNextSetOfBlocks(BlockTypes.Object, pageType); - var target = - (typesFound & BlockTypes.Object) == BlockTypes.Object - ? _blocksOnPage[objectIndex][_blockOnPageIndexes[objectIndex]] - : null; - // With more than one kind sharing the object slot we could land on an object - // belonging to another kind; better to say we found nothing than to hand a kind - // something it does not understand. - if (target != null && !kind.GetObjectsOnPage(_currentPage).Contains(target)) - target = null; if (target == null) { - WarnNoObjectFor(leadRowLabel); + Warn( + $"Row {CurrentRowIndexForMessages} is a {leadRowLabel} row, but Bloom found nowhere on the page to put it, so it was skipped." + ); _currentRowIndex = lastRowIndex; return; } @@ -1854,13 +1859,13 @@ await kind.ImportObjectAsync( } /// - /// Tells the user that the page had nowhere to put the object a lead row describes. + /// True if the page we are on has an object (of any registered kind) that no lead + /// row has used yet, so that the next lead row can stay on this page. /// - private void WarnNoObjectFor(string leadRowLabel) + private bool CurrentPageHasAnUnusedObject() { - Warn( - $"Row {CurrentRowIndexForMessages} is a {leadRowLabel} row, but Bloom found nowhere on the page to put it, so it was skipped." - ); + var objects = _blocksOnPage[objectIndex]; + return objects != null && _blockOnPageIndexes[objectIndex] + 1 < objects.Count; } /// @@ -1879,36 +1884,6 @@ private List CollectRowFamily(ISpreadsheetObjectKind kind) return rows; } - /// - /// Whether there is still an object of this kind somewhere for another of its lead - /// rows to fill: one on the current page that we have not used yet, one on a page we - /// have not reached, or one on the last content page, since a copy of that page is - /// what import adds when it runs out of pages. - /// - private bool AnObjectOfKindIsStillAvailable(ISpreadsheetObjectKind kind) - { - var objectsOnCurrentPage = _blocksOnPage[objectIndex]; - if (objectsOnCurrentPage != null && _currentPage != null) - { - var ofThisKind = kind.GetObjectsOnPage(_currentPage); - for ( - var i = Math.Max(_blockOnPageIndexes[objectIndex] + 1, 0); - i < objectsOnCurrentPage.Count; - i++ - ) - { - if (ofThisKind.Contains(objectsOnCurrentPage[i])) - return true; - } - } - for (var i = Math.Max(_currentPageIndex + 1, 0); i < _pages.Count; i++) - { - if (kind.GetObjectsOnPage(_pages[i]).Count > 0) - return true; - } - return _lastContentPage != null && kind.GetObjectsOnPage(_lastContentPage).Count > 0; - } - private List GetBloomCanvases(SafeXmlElement ancestor) { return SafeSelectNodesByClassName(ancestor, ".//div", "bloom-canvas").ToList(); diff --git a/src/BloomTests/Spreadsheet/SpreadsheetDetailsTests.cs b/src/BloomTests/Spreadsheet/SpreadsheetDetailsTests.cs index 870fbe17f3d3..34ccaa8b72ec 100644 --- a/src/BloomTests/Spreadsheet/SpreadsheetDetailsTests.cs +++ b/src/BloomTests/Spreadsheet/SpreadsheetDetailsTests.cs @@ -1,5 +1,6 @@ using System.Collections.Generic; using System.Linq; +using System.Text; using System.Threading.Tasks; using Bloom.Book; using Bloom.SafeXml; @@ -65,25 +66,30 @@ private static string TranslationGroup(string spanish, string english) /// /// The test book: a heading group, then whatever markup the caller wants in the - /// middle, then a trailing group. Putting the object between two ordinary groups is - /// what lets us see whether its rows land in the right place. + /// middle, then a trailing group, all on one page. Putting the object between two + /// ordinary groups is what lets us see whether its rows land in the right place. /// private static string MakeBook(string middleMarkup) { - return @" - - - - - + return MakeBookWithPages( + TranslationGroup("Encabezado", "Heading") + + middleMarkup + + TranslationGroup("Pie", "Footing") + ); + } - -
-
-

Details round trip

-
-
-
+ /// + /// A book with one "Just Text" page per argument, each page holding just the + /// markup given for it. + /// + private static string MakeBookWithPages(params string[] pageMarkups) + { + var pages = new StringBuilder(); + for (var i = 0; i < pageMarkups.Length; i++) + { + pages.Append( + $@" +
Just Text
@@ -93,18 +99,43 @@ Just Text
" - + TranslationGroup("Encabezado", "Heading") - + middleMarkup - + TranslationGroup("Pie", "Footing") - + @" + + pageMarkups[i] + + @"
+" + ); + } + return @" + + + + + + + +
+
+

Details round trip

+
+
" + + pages + + @" "; } + /// The book's pages, in order. + private static List PagesOf(HtmlDom dom) + { + return dom + .RawDom.SafeSelectNodes("//div[contains(@class,'bloom-page')]") + .Cast() + .ToList(); + } + private StubObjectKind _kind; private InternalSpreadsheet _sheetFromExport; private HtmlDom _emptyObjectDom; // imported into a book whose object has no content @@ -369,6 +400,77 @@ public void Import_WhenThePageHasNowhereToPutTheObject_ImportsTheRestOfThePage() Does.Not.Contain("Uno-es"), "there was nowhere to put the object, so its text must not have leaked onto the page" ); + Assert.That( + PagesOf(_domWithoutObject).Count, + Is.EqualTo(1), + "a lead row in the middle of a page is skipped in place: it must not start a new page and push the footing onto it" + ); + } + + [Test] + public async Task Import_WhenTheBookHasNoObject_LaterRowsStillLandOnTheirOwnPages() + { + // A spreadsheet whose first page holds nothing but an object, then a page of + // text, imported into a book that has no such object anywhere. The object's + // rows have nowhere to go, but they must still hold their place in the page + // order: the text that follows belongs on the second page, not the first, and + // the first page must not be thrown away as unused. + var sheet = ExportBook( + MakeBookWithPages(StubObject("Uno", "Dos"), TranslationGroup("Segundo", "Second")) + ); + Assert.That( + sheet.ContentRows.Select(r => r.MetadataKey).ToList(), + Is.EqualTo( + new[] + { + "[book title]", + StubObjectKind.LeadLabel, + StubObjectKind.PartLabel, + StubObjectKind.PartLabel, + InternalSpreadsheet.PageContentRowLabel, + } + ), + "sanity: an object-only page followed by a text page" + ); + + var target = new HtmlDom( + MakeBookWithPages( + TranslationGroup("Primero", "First"), + TranslationGroup("Viejo", "Old") + ), + true + ); + var warnings = await RoundTripThroughFileAndImportAsync(sheet, target); + + Assert.That(warnings, Has.Exactly(1).Contains(StubObjectKind.LeadLabel)); + var pages = PagesOf(target); + Assert.That(pages.Count, Is.EqualTo(2), "neither page should have been thrown away"); + Assert.That( + pages[0].InnerXml, + Does.Contain("Primero").And.Not.Contain("Segundo"), + "the first page had nowhere to put the object, so it must be left as it was" + ); + Assert.That( + pages[1].InnerXml, + Does.Contain("Segundo").And.Not.Contain("Viejo"), + "the text row belongs on the second page" + ); + } + + [Test] + public async Task Import_OfAnObjectOnlyPage_IntoABookWithNoObject_KeepsThatPage() + { + // The object's rows are the whole spreadsheet. Skipping them must still count + // as reaching the page they were meant for, or the cleanup at the end of the + // import would throw that page away as one the spreadsheet never got to. + var sheet = ExportBook(MakeBookWithPages(StubObject("Uno", "Dos"))); + var target = new HtmlDom(MakeBookWithPages(TranslationGroup("Primero", "First")), true); + var warnings = await RoundTripThroughFileAndImportAsync(sheet, target); + + Assert.That(warnings, Has.Exactly(1).Contains(StubObjectKind.LeadLabel)); + var pages = PagesOf(target); + Assert.That(pages.Count, Is.EqualTo(1), "the page must not be thrown away"); + Assert.That(pages[0].InnerXml, Does.Contain("Primero")); } [Test] From 543e2ffe75b0c2656cf8de4cf30053f5af6355f2 Mon Sep 17 00:00:00 2001 From: Hatton Date: Fri, 4 Sep 2026 15:55:26 -0600 Subject: [PATCH 5/5] Move on from a skipped object family the way any row would The previous commit moved the importer past a skipped object family only when its lead row carried a page type. A page with no label gets no page type on export, so an object-only page of that shape was still skipped in place and thrown away at the end of the import. Now the lead row always looks for its object the way a [page content] row looks for its group, before deciding to skip; the importer therefore moves the same way for every row. A lead row in the middle of a page whose book has no object now moves on too, so the rows after it land on a page of their own, as they would after any row the page had no room for. Also make SpreadsheetObjectKinds.Register reject a kind whose Kind name is already taken, as it already did for the lead row label, so that two kinds can never claim the same [details] kind. Co-Authored-By: Claude Fable 5.1 --- .../Spreadsheet/SpreadsheetImporter.cs | 51 +++++-------- .../Spreadsheet/SpreadsheetObjectKind.cs | 11 ++- .../Spreadsheet/SpreadsheetDetailsTests.cs | 72 +++++++++++++++++-- 3 files changed, 93 insertions(+), 41 deletions(-) diff --git a/src/BloomExe/Spreadsheet/SpreadsheetImporter.cs b/src/BloomExe/Spreadsheet/SpreadsheetImporter.cs index 3d00addad9f3..ae6c3fba336e 100644 --- a/src/BloomExe/Spreadsheet/SpreadsheetImporter.cs +++ b/src/BloomExe/Spreadsheet/SpreadsheetImporter.cs @@ -1784,15 +1784,13 @@ internal static bool HasExactClassName(SafeXmlElement element, string className) /// next unused object. /// /// A family that has nowhere to go gets a warning naming its lead row and is - /// skipped. Where that leaves the importer depends on whether the lead row starts a - /// page. Export writes a page type on the first row it makes for a page, so a lead - /// row carrying one is the first row of its page, and it moves onto a new page - /// whether or not an object is found there: if it did not, every row after it would - /// land a page early, and the page it was meant for would be thrown away as unused - /// at the end of the import. (Past the end of the book this adds a copy of the last - /// page with nothing to put on it, as it does for any other row.) A lead row with no - /// page type belongs to the page we are on, so if that page has no object for it we - /// skip it in place and the page stays as it was. + /// skipped, but only after the importer has moved the way it would for any row: + /// onto a new page if the lead row names a page type; otherwise it stays on the + /// page if that page still has an unused object, and else moves to the next page. + /// If it did not move, every row after a skipped family would land a page early, + /// and the page the family was meant for would be thrown away as unused at the end + /// of the import. (Past the end of the book this adds a copy of the last page with + /// nothing to put on it, as it does for any other row.) ///
/// The kind whose LeadRowLabel this row carries. /// The page type named in this row's [page type] cell, which @@ -1805,21 +1803,18 @@ private async Task ImportObjectAsync(ISpreadsheetObjectKind kind, string pageTyp var lastRowIndex = _currentRowIndex + rows.Count - 1; var leadRowLabel = kind.LeadRowLabel; - // Find the object first, so that a family we end up skipping still holds its - // place in the page order (see the summary). Only a lead row that starts a page, - // or one whose page still has an unused object, moves the importer at all. + // Find the object first, the way a [page content] row finds its group, so that + // a family we end up skipping still holds its place in the page order (see the + // summary). + var typesFound = AdvanceToNextSetOfBlocks(BlockTypes.Object, pageType); SafeXmlElement target = null; - if (!string.IsNullOrEmpty(pageType) || CurrentPageHasAnUnusedObject()) - { - var typesFound = AdvanceToNextSetOfBlocks(BlockTypes.Object, pageType); - if ((typesFound & BlockTypes.Object) == BlockTypes.Object) - target = _blocksOnPage[objectIndex][_blockOnPageIndexes[objectIndex]]; - // With more than one kind sharing the object slot we could land on an - // object belonging to another kind; better to say we found nothing than to - // hand a kind something it does not understand. - if (target != null && !kind.GetObjectsOnPage(_currentPage).Contains(target)) - target = null; - } + if ((typesFound & BlockTypes.Object) == BlockTypes.Object) + target = _blocksOnPage[objectIndex][_blockOnPageIndexes[objectIndex]]; + // With more than one kind sharing the object slot we could land on an object + // belonging to another kind; better to say we found nothing than to hand a kind + // something it does not understand. + if (target != null && !kind.GetObjectsOnPage(_currentPage).Contains(target)) + target = null; if (_sheet.GetColumnForTag(InternalSpreadsheet.DetailsColumnLabel) < 0) { @@ -1858,16 +1853,6 @@ await kind.ImportObjectAsync( _currentRowIndex = lastRowIndex; } - /// - /// True if the page we are on has an object (of any registered kind) that no lead - /// row has used yet, so that the next lead row can stay on this page. - /// - private bool CurrentPageHasAnUnusedObject() - { - var objects = _blocksOnPage[objectIndex]; - return objects != null && _blockOnPageIndexes[objectIndex] + 1 < objects.Count; - } - /// /// The run of rows that belong to the lead row we are on: itself, then every /// following row the kind claims as a continuation. diff --git a/src/BloomExe/Spreadsheet/SpreadsheetObjectKind.cs b/src/BloomExe/Spreadsheet/SpreadsheetObjectKind.cs index 53b83f587903..1e9f38c845dc 100644 --- a/src/BloomExe/Spreadsheet/SpreadsheetObjectKind.cs +++ b/src/BloomExe/Spreadsheet/SpreadsheetObjectKind.cs @@ -167,8 +167,11 @@ public static class SpreadsheetObjectKinds new List(); /// - /// Makes a kind known to the exporter and importer. Its - /// must not already be taken. + /// Makes a kind known to the exporter and importer. Neither its + /// nor its + /// may already be taken: the row label + /// is what picks the kind for a row, and the kind name is what the row's [details] + /// cell claims, so two kinds sharing either would be indistinguishable. /// public static void Register(ISpreadsheetObjectKind kind) { @@ -178,6 +181,10 @@ public static void Register(ISpreadsheetObjectKind kind) throw new ArgumentException( $"A spreadsheet object kind using the row label {kind.LeadRowLabel} is already registered." ); + if (_kinds.Any(k => k.Kind == kind.Kind)) + throw new ArgumentException( + $"A spreadsheet object kind named {kind.Kind} is already registered." + ); _kinds.Add(kind); } diff --git a/src/BloomTests/Spreadsheet/SpreadsheetDetailsTests.cs b/src/BloomTests/Spreadsheet/SpreadsheetDetailsTests.cs index 34ccaa8b72ec..baf0eba61934 100644 --- a/src/BloomTests/Spreadsheet/SpreadsheetDetailsTests.cs +++ b/src/BloomTests/Spreadsheet/SpreadsheetDetailsTests.cs @@ -127,6 +127,19 @@ Just Text "; } + /// + /// The same book with no page labels, as an older or hand-made page can have; export + /// then writes no page type for the page. + /// + private static string WithoutPageLabels(string bookHtml) + { + return System.Text.RegularExpressions.Regex.Replace( + bookHtml, + @"
]*>\s*Just Text\s*
", + "" + ); + } + /// The book's pages, in order. private static List PagesOf(HtmlDom dom) { @@ -393,18 +406,19 @@ public void Import_WhenThePageHasNowhereToPutTheObject_ImportsTheRestOfThePage() Assert.That( _domWithoutObject.RawDom.InnerXml, Does.Contain("Encabezado").And.Contain("Pie"), - "skipping the object must not cost the page its own content" + "skipping the object must not cost the book any of its own content" ); Assert.That( _domWithoutObject.RawDom.InnerXml, Does.Not.Contain("Uno-es"), "there was nowhere to put the object, so its text must not have leaked onto the page" ); - Assert.That( - PagesOf(_domWithoutObject).Count, - Is.EqualTo(1), - "a lead row in the middle of a page is skipped in place: it must not start a new page and push the footing onto it" - ); + // The lead row moved on the way any row the page had no room for would, so the + // footing that followed it went onto a page of its own. + var pages = PagesOf(_domWithoutObject); + Assert.That(pages.Count, Is.EqualTo(2)); + Assert.That(pages[0].InnerXml, Does.Contain("Encabezado")); + Assert.That(pages[1].InnerXml, Does.Contain("Pie")); } [Test] @@ -457,6 +471,52 @@ public async Task Import_WhenTheBookHasNoObject_LaterRowsStillLandOnTheirOwnPage ); } + [Test] + public async Task Import_OfAnUnlabeledObjectOnlyPage_IntoABookWithNoObject_KeepsThatPage() + { + // As above, but the page has no label, so its lead row carries no page type. + // The lead row must still count as reaching the page. + var sheet = ExportBook(WithoutPageLabels(MakeBookWithPages(StubObject("Uno", "Dos")))); + Assert.That( + sheet.GetColumnForTag(InternalSpreadsheet.PageTypeColumnLabel), + Is.LessThan(0), + "sanity: no row got a page type" + ); + var target = new HtmlDom(MakeBookWithPages(TranslationGroup("Primero", "First")), true); + var warnings = await RoundTripThroughFileAndImportAsync(sheet, target); + + Assert.That(warnings, Has.Exactly(1).Contains(StubObjectKind.LeadLabel)); + var pages = PagesOf(target); + Assert.That(pages.Count, Is.EqualTo(1), "the page must not be thrown away"); + Assert.That(pages[0].InnerXml, Does.Contain("Primero")); + } + + [Test] + public void Register_OfAKindWhoseNameIsTaken_Throws() + { + var other = new Mock(); + other.Setup(k => k.Kind).Returns(StubObjectKind.KindName); + other.Setup(k => k.LeadRowLabel).Returns("[some other label]"); + Assert.That( + () => SpreadsheetObjectKinds.Register(other.Object), + Throws.ArgumentException + ); + Assert.That(SpreadsheetObjectKinds.All, Has.Count.EqualTo(1)); + } + + [Test] + public void Register_OfAKindWhoseLeadRowLabelIsTaken_Throws() + { + var other = new Mock(); + other.Setup(k => k.Kind).Returns("some-other-kind"); + other.Setup(k => k.LeadRowLabel).Returns(StubObjectKind.LeadLabel); + Assert.That( + () => SpreadsheetObjectKinds.Register(other.Object), + Throws.ArgumentException + ); + Assert.That(SpreadsheetObjectKinds.All, Has.Count.EqualTo(1)); + } + [Test] public async Task Import_OfAnObjectOnlyPage_IntoABookWithNoObject_KeepsThatPage() {