Bug 42474 - Patches for a couple of bugs in HSLF
Summary: Patches for a couple of bugs in HSLF
Status: RESOLVED FIXED
Alias: None
Product: POI
Classification: Unclassified
Component: HSLF (show other bugs)
Version: unspecified
Hardware: Other other
: P2 normal (vote)
Target Milestone: ---
Assignee: POI Developers List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2007-05-21 10:22 UTC by Yegor Kozlov
Modified: 2007-05-24 03:57 UTC (History)
0 users



Attachments
correct-matching-notes-to-slides.patch (1.94 KB, patch)
2007-05-21 10:24 UTC, Yegor Kozlov
Details | Diff
fix-NPE-with-textruns-from-notes.patch (2.33 KB, patch)
2007-05-21 10:24 UTC, Yegor Kozlov
Details | Diff
PPT file demonstrating the second bug (10.00 KB, application/vnd.ms-powerpoint)
2007-05-21 15:04 UTC, Ivan Todoroski
Details
PPT file demonstrating the first bug (Notes->Slides linkage) (558.00 KB, application/vnd.ms-powerpoint)
2007-05-21 22:05 UTC, Ivan Todoroski
Details
correct-matching-notes-to-slides.patch (1.70 KB, patch)
2007-05-21 22:16 UTC, Ivan Todoroski
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yegor Kozlov 2007-05-21 10:22:21 UTC
1. Incorrect matching of notes to slides

This is a long standing problem apparently, I noticed a TODO comment about it... 
anyway, after a bit of trial and error the problem seems to be that for some PPT 
files the SlidePersistAtom from the note's SlideAtomSet has a wrong 
slideIdentifier sometimes, so matching it to the associated slide is impossible. 
But if you look at the NotesAtom from the actual Notes record (retrieved via 
getCoreRecordForSAS()) it has the correct slideId, so that's what I use in my patch.

I don't know yet why SlideAtomSet.getSlidePersistAtom().getSlideIdentifier() is 
different from Notes.getNotesAtom().getSlideId(), maybe that's the real bug and 
my patch is merely working around it? Someone more knowledgeable could shed some 
light on whether these two values are supposed to be always equal, in the 
meantime this patch works perfectly for all my test cases which previously 
exhibited incorrect note->slide association.


2. NPE in RichTextRun.isBold() when the RichTextRun comes from a Notes model object

The Notes object wasn't doing setSheet(this) on the text runs it returned, 
causing a NPE inside RichTextRun.isCharFlagsTextPropVal() when any style 
accessor is called, because it tries to get the style info from the master sheet 
if not present. However, doing setSheet() in Notes was not enough, it merely 
moved the NPE a couple of lines down because NotesMaster objects are not yet 
implemented in HSLF, so I had to add a few null checks in RichTextRun too.
Comment 1 Yegor Kozlov 2007-05-21 10:24:19 UTC
Created attachment 20225 [details]
correct-matching-notes-to-slides.patch
Comment 2 Yegor Kozlov 2007-05-21 10:24:39 UTC
Created attachment 20226 [details]
fix-NPE-with-textruns-from-notes.patch
Comment 3 Ivan Todoroski 2007-05-21 15:04:30 UTC
Created attachment 20229 [details]
PPT file demonstrating the second bug

To reproduce the NPE:

new SlideShow(new
HSLFSlideShow("NPE-in-textrun-from-notes.ppt")).getNotes()[0].getTextRuns()[0].getRichTextRuns()[0].isBold();
Comment 4 Ivan Todoroski 2007-05-21 22:05:19 UTC
Created attachment 20235 [details]
PPT file demonstrating the first bug (Notes->Slides linkage)

From http://www.cse.lehigh.edu/~caar/marnold/presentations/vail3.ppt

Found a good example PPT file for the first bug. To reproduce:

Notes notes = new SlideShow(new
HSLFSlideShow("vail3.ppt")).getSlides()[7].getNotesSheet();

Without the patch, this returns null. With the patch it returns the correct
Notes object with the correct text.


P.S. You will notice that with or without the patch there are some superflous
Notes being returned for Slides which have no actual Notes in PowerPoint, but
atleast with the patch Notes don't seem to get lost.
Comment 5 Ivan Todoroski 2007-05-21 22:16:29 UTC
Created attachment 20236 [details]
correct-matching-notes-to-slides.patch

Revised the patch for the first bug to not remove the TODO comment about 
notes->slides linkage as there is obviously more work to be done.
Comment 6 Yegor Kozlov 2007-05-24 01:09:56 UTC
I applied the patches.
Unit tests are in
src/scratchpad/testcases/org/apache/poi/hslf/usermodel/TestBugs.java

1. Incorrect matching of notes to slides
My fix is a bit different from the suggested patch. To find Notes for a Slide we
need to look at Slide.SlideAtom.notesId. It references the corresponding notes
slide ( 0 if slide has no notes). This logic works OK to me.   

2. NPE in RichTextRun.isBold() when the RichTextRun comes from a Notes model object

Applied. It's possible to have null master sheet for Notes. No NPE in this case.

>P.S. You will notice that with or without the patch there are some superflous
>Notes being returned for Slides which have no actual Notes in PowerPoint, but
>atleast with the patch Notes don't seem to get lost.

I confirmed that. For the ppt from
http://www.cse.lehigh.edu/~caar/marnold/presentations/vail3.ppt slides 2 and 3
don't have notes in PowerPoint but have them in HSLF. This is quite unexpected
to me. Slide.SlideAtom.notesId indicates that the slide has notes. It looks like
there is something else to check when linking Slides and Notes. But what? I'm
leaving this riddle undiscovered.  

Yegor
Comment 7 Ivan Todoroski 2007-05-24 01:17:08 UTC
Hi Yegor, looks like the second patch was not applied properly. The SVN commit 
message contains this chunk:


 				// Record the match between slide id and these 
notes
-				SlidePersistAtom spa = 
notesSets[i].getSlidePersistAtom();
-				Integer slideId = new 
Integer(spa.getSlideIdentifier());
-				slideIdToNotes.put(slideId, new Integer(i));
+                SlidePersistAtom spa = notesSets[i].getSlidePersistAtom();
+                Integer slideId = new Integer(spa.getSlideIdentifier());
+                slideIdToNotes.put(slideId, new Integer(i));


but the newly added code in this chunk should be:


NotesAtom na = notesRecord.getNotesAtom();
Integer slideId = new Integer(na.getSlideID());
slideIdToNotes.put(slideId, new Integer(i));
Comment 8 Ivan Todoroski 2007-05-24 01:22:28 UTC
(In reply to comment #7)
> Hi Yegor, looks like the second patch was not applied properly. The SVN 
commit 
> message contains this chunk:

Ah, nevermind, I missed your comment that you solved it differently... I will 
test your fix with other PPT files I have around, and report any issues.
Comment 9 Yegor Kozlov 2007-05-24 01:28:44 UTC
Hi Ivan,

this is intentionally.
See how I link Slides and Notes:
		// Do we have a notes for this?
		Notes notes = null;
        //Slide.SlideAtom.notesId references the corresponding notes slide. 0 if
slide has no notes.
        int noteId = slidesRecords[i].getSlideAtom().getNotesID();
        if (noteId != 0){
            Integer notesPos = (Integer)slideIdToNotes.get(new Integer(noteId));
            if (notesPos != null) notes = _notes[notesPos.intValue()];
            else logger.log(POILogger.ERROR, "Notes not found for noteId=" +
noteId);
        }

Slide.SlideAtom.notesId points to the Note sheet and id of this sheet is takes
from the corresponding SlidePersistAtom.slideId. (Don't be misleaded.
SlidePersistAtom.slideId is actually sheet identifier, it can point to Slide,
Notes, Master and other objects).

Yegor
Comment 10 Yegor Kozlov 2007-05-24 01:31:57 UTC
>I will test your fix with other PPT files I have around, and report any issues.
OK. Shout if there are still problems with Slide-->Notes linkage.

P.S. You posted more bugs. It's time to look at them :)

Yegor
Comment 11 Ivan Todoroski 2007-05-24 03:57:28 UTC
Your fix works great with all my reference PPTs, thank you.