Re: Patches for a couple of bugs in HSLF

2007-05-21 Thread Ivan Todoroski


Ah sorry, somehow I missed the bugzilla link on the main page.

Unfortunately, the sample PPTs that demonstrated incorrect Notes-Slides linkage 
are internal company files which I'm not sure I'm allowed to send out. There was 
already a comment about this problem in the code, so you or some other HSLF 
developer must have had an example PPT file for this at some point? If not, I 
can try some random PPT files from the web...


As for the second bug, it should be easy to reproduce, I will attach a sample 
PPT later.


On 21.05.2007 19:34, Yegor Kozlov wrote:

Ivan,

Thanks for the patches. I will find time to review them.

P.S. We submit patches via Bugzilla. Please use it next time. Brief
instructions for contributors are here: 
http://jakarta.apache.org/poi/getinvolved/index.html

I opened a new bug and attached your patches to it:
http://issues.apache.org/bugzilla/show_bug.cgi?id=42474

Could you attach sample ppts that were causing problems? One for Notes
- Slides linkage problem and another for NPE in
RichTextRun.isBold()?

Regards,
Yegor

Forgot the attachments...




On 20.05.2007 11:58, Ivan Todoroski wrote:

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 sometimes has a wrong slideIdentifier, 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 simply working around it? Someone more knowledgeable could shed 
some light whether these two values are even supposed to be equal, in 
the mean time this patch works perfectly for all my test cases which 
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.



P.S. Not subscribed, please CC.





-
To unsubscribe, e-mail: [EMAIL PROTECTED]
Mailing List:http://jakarta.apache.org/site/mail2.html#poi
The Apache Jakarta POI Project: http://jakarta.apache.org/poi/




-
To unsubscribe, e-mail: [EMAIL PROTECTED]
Mailing List:http://jakarta.apache.org/site/mail2.html#poi
The Apache Jakarta POI Project: http://jakarta.apache.org/poi/



Re: Patches for a couple of bugs in HSLF

2007-05-20 Thread Ivan Todoroski

Forgot the attachments...


On 20.05.2007 11:58, Ivan Todoroski wrote:

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 sometimes has a wrong slideIdentifier, 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 simply working around it? Someone more knowledgeable could shed 
some light whether these two values are even supposed to be equal, in 
the mean time this patch works perfectly for all my test cases which 
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.



P.S. Not subscribed, please CC.



Index: src/scratchpad/src/org/apache/poi/hslf/usermodel/SlideShow.java
===
--- src/scratchpad/src/org/apache/poi/hslf/usermodel/SlideShow.java 
(revision 539839)
+++ src/scratchpad/src/org/apache/poi/hslf/usermodel/SlideShow.java 
(working copy)
@@ -32,6 +32,7 @@
 import org.apache.poi.hslf.model.*;
 import org.apache.poi.hslf.record.Document;
 import org.apache.poi.hslf.record.DocumentAtom;
+import org.apache.poi.hslf.record.NotesAtom;
 import org.apache.poi.hslf.record.FontCollection;
 import org.apache.poi.hslf.record.ParentAwareRecord;
 import org.apache.poi.hslf.record.PositionDependentRecordContainer;
@@ -54,8 +55,6 @@
  * This class is a friendly wrapper on top of the more scary HSLFSlideShow.
  *
  * TODO:
- *  - figure out how to match notes to their correct sheet
- *(will involve understanding DocSlideList and DocNotesList)
  *  - handle Slide creation cleaner
  * 
  * @author Nick Burch
@@ -363,12 +362,13 @@
Record r = getCoreRecordForSAS(notesSets[i]);

// Ensure it really is a notes record
-   if(r != null  r instanceof 
org.apache.poi.hslf.record.Notes) {
-   notesRecordsL.add( 
(org.apache.poi.hslf.record.Notes)r );
+   if(r instanceof org.apache.poi.hslf.record.Notes) {
+   org.apache.poi.hslf.record.Notes notesRecord = 
(org.apache.poi.hslf.record.Notes)r;
+   notesRecordsL.add(notesRecord);

// Record the match between slide id and these 
notes
-   SlidePersistAtom spa = 
notesSets[i].getSlidePersistAtom();
-   Integer slideId = new 
Integer(spa.getSlideIdentifier());
+   NotesAtom na = notesRecord.getNotesAtom();
+   Integer slideId = new Integer(na.getSlideID());
slideIdToNotes.put(slideId, new Integer(i));
} else {
logger.log(POILogger.ERROR, A Notes 
SlideAtomSet at  + i +  said its record was at refID  + 
notesSets[i].getSlidePersistAtom().getRefID() + , but that was actually a  + 
r);
Index: src/scratchpad/src/org/apache/poi/hslf/usermodel/RichTextRun.java
===
--- src/scratchpad/src/org/apache/poi/hslf/usermodel/RichTextRun.java   
(revision 539839)
+++ src/scratchpad/src/org/apache/poi/hslf/usermodel/RichTextRun.java   
(working copy)
@@ -170,7 +170,8 @@
 Sheet sheet = parentRun.getSheet();
 int txtype = parentRun.getRunType();
 SlideMaster master = (SlideMaster)sheet.getMasterSheet();
-cftp = (CharFlagsTextProp)master.getStyleAttribute(txtype, 
getIndentLevel(), char_flags, true);
+if (master != null)
+cftp = (CharFlagsTextProp)master.getStyleAttribute(txtype, 
getIndentLevel(), char_flags, true);
 }
 
return cftp == null ? false : cftp.getSubValue(index);
@@ -223,7 +224,8 @@
 Sheet sheet = parentRun.getSheet();
 int txtype = parentRun.getRunType();
 SlideMaster master =