pjfanning commented on code in PR #1524:
URL: https://github.com/apache/poi/pull/1524#discussion_r4453442583
##########
poi-scratchpad/src/main/java/org/apache/poi/hslf/usermodel/HSLFSlideShowImpl.java:
##########
@@ -989,6 +991,38 @@ public int appendRootLevelRecord(Record newRecord) {
return addedAt;
}
+ /**
+ * Removes persist objects, e.g. the slide and notes of a removed slide:
their entries in
+ * the persist directories and the root level records these point to,
including older
+ * versions of the records kept by incremental saves.
+ *
+ * @param persistIds the persist ids of the objects to remove
+ */
+ void removePersistObjects(Collection<Integer> persistIds) {
+ Set<Integer> removedOffsets = new HashSet<>();
+ Set<Integer> keptOffsets = new HashSet<>();
+ for (Record record : _records) {
+ if (!(record instanceof PersistPtrHolder pph)) {
+ continue;
+ }
+ Map<Integer, Integer> locations = new
HashMap<>(pph.getSlideLocationsLookup());
+ for (Integer persistId : persistIds) {
+ Integer offset = locations.remove(persistId);
+ if (offset != null) {
+ removedOffsets.add(offset);
+ }
+ }
+ keptOffsets.addAll(locations.values());
+ pph.clear();
Review Comment:
Minor: this copies, clears and rebuilds every `PersistPtrHolder`, including
ones that contain none of the removed ids. A
`PersistPtrHolder.removeSlideLookup(int)` that returns the removed offset would
be simpler and touch only the affected entries.
##########
poi-scratchpad/src/main/java/org/apache/poi/hslf/usermodel/HSLFSlideShow.java:
##########
@@ -772,6 +777,10 @@ public HSLFSlide removeSlide(int index) {
}
}
+ // without their slide list entries the records are no longer
reachable, but they would
+ // still be written out with the content of the removed slide
Review Comment:
Only the slide and notes persist objects are removed. Objects that were
reachable only from the removed slide stay in the stream: embedded OLE data
(`ExOleObjStg`), ActiveX controls, and the `ExObjList` entries for its
hyperlinks. Fine to leave for a follow-up, but it would be worth noting in the
PR that the removed slide's embedded data is still written out.
##########
poi-scratchpad/src/main/java/org/apache/poi/hslf/usermodel/HSLFSlideShow.java:
##########
@@ -753,6 +757,7 @@ public HSLFSlide removeSlide(int index) {
if (nslwt != null) {
for (SlideAtomsSet ns : nslwt.getSlideAtomsSets()) {
if (ns.getSlidePersistAtom().getSlideIdentifier() ==
notesId) {
+
removedPersistIds.add(ns.getSlidePersistAtom().getRefID());
Review Comment:
Notes are matched only by `notesId`. If a (malformed) file has another
remaining slide pointing at the same notes id, that notes record is now removed
from the stream while the other slide still holds a live `HSLFNotes` for it, so
its notes vanish on save. It would be safer to only remove the notes persist
object if no remaining slide refers to it.
##########
poi-scratchpad/src/main/java/org/apache/poi/hslf/usermodel/HSLFSlideShow.java:
##########
@@ -772,6 +777,10 @@ public HSLFSlide removeSlide(int index) {
}
}
+ // without their slide list entries the records are no longer
reachable, but they would
+ // still be written out with the content of the removed slide
+ _hslfSlideShow.removePersistObjects(removedPersistIds);
Review Comment:
After this, `_mostRecentCoreRecords` and `_sheetIdToCoreRecordsLookup` still
reference the removed Slide/Notes records, so
`getCoreRecordForRefID(removedRefId)` and `getMostRecentCoreRecords()` (used by
e.g. `SlideIdListing`) still return records that are no longer in the document.
Those caches should be updated too.
##########
poi-scratchpad/src/test/java/org/apache/poi/hslf/usermodel/TestAddingSlides.java:
##########
@@ -248,6 +251,51 @@ void testRemoving() throws IOException {
}
}
+ /**
+ * The records of a removed slide and its notes are not written out again
+ */
+ @Test
+ void removedSlideIsNotWrittenOut() throws IOException {
+ ss_empty.createSlide().createTextBox().setText("Text on the removed
slide");
+ ss_empty.createSlide().createTextBox().setText("Text on the kept
slide");
+ try (HSLFSlideShow ppt =
HSLFTestDataSamples.writeOutAndReadBack(ss_empty)) {
+ ppt.removeSlide(0);
+ String text = getRawText(ppt);
+ assertTrue(text.contains("Text on the kept slide"));
Review Comment:
Please use Hamcrest here (and at the three similar asserts below):
`assertThat(text, containsString("..."))` / `assertThat(text,
not(containsString("...")))`. Then a failure shows the extracted text, not just
`expected: <true>`.
##########
poi-scratchpad/src/main/java/org/apache/poi/hslf/usermodel/HSLFSlideShowImpl.java:
##########
@@ -989,6 +991,38 @@ public int appendRootLevelRecord(Record newRecord) {
return addedAt;
}
+ /**
+ * Removes persist objects, e.g. the slide and notes of a removed slide:
their entries in
+ * the persist directories and the root level records these point to,
including older
+ * versions of the records kept by incremental saves.
+ *
+ * @param persistIds the persist ids of the objects to remove
+ */
+ void removePersistObjects(Collection<Integer> persistIds) {
+ Set<Integer> removedOffsets = new HashSet<>();
+ Set<Integer> keptOffsets = new HashSet<>();
+ for (Record record : _records) {
+ if (!(record instanceof PersistPtrHolder pph)) {
+ continue;
+ }
+ Map<Integer, Integer> locations = new
HashMap<>(pph.getSlideLocationsLookup());
+ for (Integer persistId : persistIds) {
+ Integer offset = locations.remove(persistId);
+ if (offset != null) {
+ removedOffsets.add(offset);
+ }
+ }
+ keptOffsets.addAll(locations.values());
+ pph.clear();
+ locations.forEach(pph::addSlideLookup);
+ }
+ removedOffsets.removeAll(keptOffsets);
+
+ _records = Arrays.stream(_records)
+ .filter(r -> !(r instanceof PositionDependentRecord pdr &&
removedOffsets.contains(pdr.getLastOnDiskOffset())))
Review Comment:
This drops any root record whose offset matches a removed persist id,
whatever type it is. In a corrupt or crafted file, a persist directory entry
could point at the offset of the `UserEditAtom`, a `PersistPtrHolder` or the
`Document` record. That record would then be removed from `_records`, and the
next `write()` fails in `updateAndWriteDependantRecords` ("UserEditAtom or
PersistPtr can't be determined") or produces a file with no `Document`. Could
this be restricted to `Slide` / `Notes` records?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
