poi-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From bugzi...@apache.org
Subject DO NOT REPLY [Bug 48018] Defect in - org.apache.poi.hwpf.model.ListLevel
Date Wed, 21 Oct 2009 21:44:40 GMT
https://issues.apache.org/bugzilla/show_bug.cgi?id=48018

Josh Micich <josh@gildedtree.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |NEEDINFO

--- Comment #5 from Josh Micich <josh@gildedtree.com> 2009-10-21 14:44:36 UTC ---
Rather than just making arbitrary changes to the code, it would be nice to have
some additional justification - junits are usually a good start.  Can you give
details as to how the current code causes errors?

I understand very little about the code that you are changing, but there seems
to be a few outstanding issues in your patch that either need fixing or
explaining.

ListLevel.java

Problems with the order of field serialisation.

the current order is:
<init>()      { .., cbGrpprlChpx, cbGrpprlPapx, .., grpprlPapx, grpprlChpx, ..
}
toByteArray() { .., cbGrpprlChpx, cbGrpprlPapx, .., grpprlChpx, grpprlPapx, ..
}

but you suggest:
<init>()      { .., cbGrpprlPapx, cbGrpprlChpx, .., grpprlPapx, grpprlChpx, ..
}
toByteArray() { .., cbGrpprlPapx, cbGrpprlChpx, .., grpprlPapx, grpprlChpx, .. 
}

So the patch does the exact opposite of what you describe in (comment 0) which
suggests the correct code should be 'Ch' before 'Pa'.  Note - the ordering in
the current code is internally inconsistent at lines 106-109 (the only place
that currently has 'Pa' before 'Ch').  Perhaps this is the one place you wanted
to change.

I am pretty sure that the proposed change at line 122 makes no difference:
-    _numberText[x] = (char)LittleEndian.getShort(buf, offset);
+    _numberText[x] = (char)LittleEndian.getUShort(buf, offset);


ListTables.java

You've made a new field _listArr which seems to store the same data as
_listMap.   Around line 106 should there be an extra line added, similar to
what you have done at line 62?
     _listMap.put(new Integer(lsid), lst);
+    _listArr.add(lst);

It would also be good to add a comment explaining why you're forcing the
ListData objects to be serialised in the same order they were read in.



Lastly, can you make your changes with respect to the latest code from trunk? 
Your original change seems to be from prior to svn r805492 .  Try composing the
patch using "svn diff" or "git diff".  Patch/diff files are easier to manage.

-- 
Configure bugmail: https://issues.apache.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org


Mime
View raw message