naruto-lgtm commented on code in PR #1524:
URL: https://github.com/apache/poi/pull/1524#discussion_r4465357019
##########
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:
Done, the filter now only drops Slide and Notes records. Anything else a
directory entry points at stays in _records.
##########
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:
Added PersistPtrHolder.removeSlideLookup(int), which returns the removed
offset or null. The copy, clear and rebuild is gone, only holders that know the
id are touched.
##########
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:
Good catch. removeSlide now rebuilds _mostRecentCoreRecords and
_sheetIdToCoreRecordsLookup without the removed sheets, so
getCoreRecordForRefID returns null for them and getMostRecentCoreRecords no
longer lists them. The test asserts both, and that the kept slide still
resolves.
##########
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:
Done. If a remaining slide has the same notes id, removeSlide leaves the
notes alone entirely: the HSLFNotes stays in the notes list, the SlideAtomsSet
stays in the notes SLWT and the record is kept. Keeping just the persist object
while dropping the SLWT entry would still lose the notes for the other slide on
reload, since findSlides matches them through the notes SLWT. Added
removedSlideKeepsSharedNotes for this, using 42474-1.ppt with the second slide
pointed at the first slide's notes.
##########
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:
Agreed, left for a follow-up. The PR description now says that the removed
slide's ExOleObjStg, ActiveX controls and ExObjList entries are still written
out.
--
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]
