From 55b31d213d647cc3b643bdb473b185b8844ba84d Mon Sep 17 00:00:00 2001
From: Yegor Kozlov <yegor@apache.org>
Date: Thu, 29 Oct 2009 16:43:24 +0000
Subject: [PATCH] fixed logic for matching cells and comments in
 HSSFCell.getCellComment()

git-svn-id: https://svn.apache.org/repos/asf/poi/trunk@831025 13f79535-47bb-0310-9956-ffa450edef68
---
 src/documentation/content/xdocs/status.xml    |   1 +
 .../apache/poi/hssf/usermodel/HSSFCell.java   |  11 ++---
 .../poi/hssf/usermodel/TestHSSFComment.java   |  39 ++++++++++++++++++
 test-data/spreadsheet/47924.xls               | Bin 0 -> 18944 bytes
 4 files changed, 46 insertions(+), 5 deletions(-)
 create mode 100644 test-data/spreadsheet/47924.xls

diff --git a/src/documentation/content/xdocs/status.xml b/src/documentation/content/xdocs/status.xml
index 209d8ae8d8..0891579ed8 100644
--- a/src/documentation/content/xdocs/status.xml
+++ b/src/documentation/content/xdocs/status.xml
@@ -34,6 +34,7 @@
 
     <changes>
         <release version="3.6-beta1" date="2009-??-??">
+           <action dev="POI-DEVELOPERS" type="fix">47924 - fixed logic for matching cells and comments in HSSFCell.getCellComment()</action>
            <action dev="POI-DEVELOPERS" type="add">47942 - added implementation of protection features to XLSX and DOCX files</action>
            <action dev="POI-DEVELOPERS" type="fix">48070 - preserve leading and trailing white spaces in XSSFRichTextString</action>
            <action dev="POI-DEVELOPERS" type="add">48044 - added implementation for CountBlank function</action>
diff --git a/src/java/org/apache/poi/hssf/usermodel/HSSFCell.java b/src/java/org/apache/poi/hssf/usermodel/HSSFCell.java
index 999fa287dc..701b7ce746 100644
--- a/src/java/org/apache/poi/hssf/usermodel/HSSFCell.java
+++ b/src/java/org/apache/poi/hssf/usermodel/HSSFCell.java
@@ -1054,7 +1054,8 @@ public class HSSFCell implements Cell {
     protected static HSSFComment findCellComment(Sheet sheet, int row, int column) {
         // TODO - optimise this code by searching backwards, find NoteRecord first, quit if not found. Find one TXO by id
         HSSFComment comment = null;
-        ArrayList<TextObjectRecord> noteTxo = new ArrayList<TextObjectRecord>();
+        Map<Integer, TextObjectRecord> noteTxo =
+                               new HashMap<Integer, TextObjectRecord>();
         int i = 0;
         for (Iterator<RecordBase> it = sheet.getRecords().iterator(); it.hasNext();) {
             RecordBase rec = it.next();
@@ -1062,7 +1063,7 @@ public class HSSFCell implements Cell {
                 NoteRecord note = (NoteRecord) rec;
                 if (note.getRow() == row && note.getColumn() == column) {
                     if(i < noteTxo.size()) {
-                        TextObjectRecord txo = noteTxo.get(i);
+                        TextObjectRecord txo = noteTxo.get(note.getShapeId());
                         comment = new HSSFComment(note, txo);
                         comment.setRow(note.getRow());
                         comment.setColumn((short) note.getColumn());
@@ -1081,12 +1082,12 @@ public class HSSFCell implements Cell {
                 if (sub instanceof CommonObjectDataSubRecord) {
                     CommonObjectDataSubRecord cmo = (CommonObjectDataSubRecord) sub;
                     if (cmo.getObjectType() == CommonObjectDataSubRecord.OBJECT_TYPE_COMMENT) {
-                        //find the next TextObjectRecord which holds the comment's text
-                        //the order of TXO matches the order of NoteRecords: i-th TXO record corresponds to the i-th NoteRecord
+                        //map ObjectId and corresponding TextObjectRecord,
+                        //it will be used to match NoteRecord and TextObjectRecord
                         while (it.hasNext()) {
                             rec = it.next();
                             if (rec instanceof TextObjectRecord) {
-                                noteTxo.add((TextObjectRecord) rec);
+                                noteTxo.put(cmo.getObjectId(), (TextObjectRecord) rec);
                                 break;
                             }
                         }
diff --git a/src/testcases/org/apache/poi/hssf/usermodel/TestHSSFComment.java b/src/testcases/org/apache/poi/hssf/usermodel/TestHSSFComment.java
index 7014cb5149..bb4707b321 100644
--- a/src/testcases/org/apache/poi/hssf/usermodel/TestHSSFComment.java
+++ b/src/testcases/org/apache/poi/hssf/usermodel/TestHSSFComment.java
@@ -23,6 +23,10 @@ import java.io.IOException;
 import junit.framework.TestCase;
 
 import org.apache.poi.hssf.HSSFTestDataSamples;
+import org.apache.poi.ss.usermodel.Row;
+import org.apache.poi.ss.usermodel.Cell;
+import org.apache.poi.ss.usermodel.Comment;
+import org.apache.poi.ss.util.CellReference;
 
 /**
  * Tests TestHSSFCellComment.
@@ -215,4 +219,39 @@ public final class TestHSSFComment extends TestCase {
 //        wb.write(fout);
 //        fout.close();
     }
+
+    /**
+     *  HSSFCell#findCellComment should NOT rely on the order of records
+     * when matching cells and their cell comments. The correct algorithm is to map
+     */
+    public static void test47924() {
+        HSSFWorkbook wb = HSSFTestDataSamples.openSampleWorkbook("47924.xls");
+        HSSFSheet sheet = wb.getSheetAt(0);
+        HSSFCell cell;
+        HSSFComment comment;
+
+        cell = sheet.getRow(0).getCell(0);
+        comment = cell.getCellComment();
+        assertEquals("a1", comment.getString().getString());
+
+        cell = sheet.getRow(1).getCell(0);
+        comment = cell.getCellComment();
+        assertEquals("a2", comment.getString().getString());
+
+        cell = sheet.getRow(2).getCell(0);
+        comment = cell.getCellComment();
+        assertEquals("a3", comment.getString().getString());
+
+        cell = sheet.getRow(2).getCell(2);
+        comment = cell.getCellComment();
+        assertEquals("c3", comment.getString().getString());
+
+        cell = sheet.getRow(4).getCell(1);
+        comment = cell.getCellComment();
+        assertEquals("b5", comment.getString().getString());
+
+        cell = sheet.getRow(5).getCell(2);
+        comment = cell.getCellComment();
+        assertEquals("c6", comment.getString().getString());
+    }
 }
diff --git a/test-data/spreadsheet/47924.xls b/test-data/spreadsheet/47924.xls
new file mode 100644
index 0000000000000000000000000000000000000000..287f17b6e85a037fe4564bb9fe48a74c7381d970
GIT binary patch
literal 18944
zcmeHP4RBml6~6E7W_R<`Hc8V^N@=(0Uz(;N&8DHXg-u(qAZZ(+4poaKNjGh5NNU((
zQ7WmxjAM~04VJM)1(g}pinXnXBif*jj!sZUMMS|VN=2ljX~j_-VEw*(_wBxS_q~^0
zrH*xYnfrduyZ1Zio^$TGKksgy{AtCB2fjA#d7&Mbihy_{RV+d_x`BHpd6Wwg#SNd|
zNTpJeh7)ezI?f>tybf7s!)^hjAW{($Z5~F7AQdB(AeADOAx%J<h%^ZaE+8t9Dv>55
zRUu75szy2wX)4nBNYjumK$?zpA<_(_nMm{-rW)J*Ii$M(3sJ8Y{rF3wEnhDBa2*gg
z!y5S?)$kZg5hjX7BRmT+BG2OOybK<Ee*Te-KQ^9R{o==u;W`Vn5tU6`3+@AAyXcg1
zY*(cUI9CeOpx;_D<WrQauUoEi7Kv-bPH_YNI%M6igciD`Or#`55AOSXKx@IDF_7(t
zGWcY5ygHG7DiW80Lzk2xAuUjtJWIjfs(ZftsjqShjiWe%Rvao4Uz+(u_(zE5A-sHE
znBs8oah!$t9nvPTgKhuaiLLzu(X0AD)Z2ek^lTriqzoMfWf+01mlz>PXF|`zIF}=?
z@C~ew^JIDcs?7VCJYRx0yTp4$`8%$dwG=tnPi-_?Wxh|ybB#Pt#MuzvkY#=?DT@)E
zo<;tM*hdYSVp`Z(4NvHWCv?I?dc=P#$8>Q&1bC;CLm1OS#s<i-4c<b}%wLM)acfdF
zP7Xsfi^r&FPi+|JC;W4m=n=-#XoYW%dbG2pzFdOi=6Dv?_Q#;@FZGp!Ihx1Kv0|JY
zhKT#tWt&S~Zb*BeWqku)>aTV7-CkOs5uA?LjefdjPR;E4*|QrrFIc{*)_T~qU`B8j
zMgr)wEfU_p_v*x!>ztWtFjPRNZe?m{bmHKdQ1c;bjh;(GOJ}`U)QKh`HdB<ks3lqY
zaZ{}%Y+A59O{iH^1GzTWwr&QO2C)EtC~7ZP7qu6vi#kfgMZI{NlbNc4lb;mL#xfqy
zGHnqH(HgT%t@0VZR$<9HTk?b)FA4FtR1_X%9V1cFIRcaPSKgw(8G4`=4H31JiAbjD
z`B?f@@Uu?+XXsz?pnu7O{;&uAJ`eg=J?Mu#=%4nW@AaU2$>W88RN{o87ODS?9zNqi
zSMucMSMuc2zu<xMH4pkR5BhgJ=pXl>f763rs61bnIGiCl^`rcRvnENur=sHC(tArq
z@PN$EnUtb;cORd5e5MF~L()TtA(H+NWRQpDp9?(b^&a#(S2|~J5?_laoY6^o#=gp*
zIm?suUx;?xVzQ>>;mprQ2dtuVHkhVIaf?TduJPr}QRdI+NBKWzlWCmmaf^wg!r=^4
z(lc^?Ijy%eeYm2cy0jX4J}Bv&jav9h&r1Ixj8i0C({q)TUz3xw*|a<z7#C#pz!|Qj
z+w$KMsS>x)Au`9MV(N=8y}CQHNbGjdnZd+Nx=PL(ILu@xiSDItB6?&qw}2Xn7EnM9
z)(R-V=}SQ(aSl;X0WK^QRDgY8K?TBv7NAiI;=pEIPyx30f(o#q7F2*OxS#@T+65J0
zn=hyUgMn{>m0EjXGzfUNhoEdIiEg@_8tFx&yCXUXLq{GUbVF|t#+f`o=$hUj3_f{)
z&|SSj7?JV-p$mJ1Fnr|!Lbvt?VXPY$WVFrSI$RFS1H|7tQ}YAyw+<I1^N_^fI$Y7r
z1H|7tToTO##NRrYpV>h0;Ot9O+i8mVTZfCXSxG!W{H?=P-8?}2t;1#EJV5-d6U`6A
z-#WAM1M#;Gro7`<M=iX2H$b&KZ*}HCbThG9x%UQ{lb<C1)|s0hh`)8_<p<(#o%#8J
z_*<to1H$prun46b<1n@zzDWJ>C#kGr@76l61;`PWofq@|bGL8@IdkSr1|*yYzV+5y
zQ_=JhaiJC4Xuy_7i3@FaL>H<Kd6iHc*zs_!*@F*0m?Km?9?ucVPP2p}O+j;!NomSX
z%N@loM0VeCfj&;IFSNj-LJKq$THvBg0a}{|bSO4=Skj1WDxk3d7H*vdEFb&9U+g}n
zk8QBLWD6rSse>zhheZD%k2SUNK~iylkXSw-dUvDSwHY8;N64v8t~)P5fovO#$8U01
zVrukLIqv-CwK3f&Eg4m~Hhil=rv{CTP`U1Lu>)i{;o%;smxpa2dTU>rQ4e*QGoQ44
zER`C~fLI~u$dMyCt=&$uT03dLmWS2a?X+B*Ekj<r&EoDhbBl}iaAT>_PP2rvhZ{>M
zJ1sW^VX4>Vc{!TOEx@mY-mYrwj=W3j4pyK*(0eXJz3WZY-h8d@AXk!0GLq2UU<{`|
zhs;WR6~pANsc~*}QFLWjSE4Vucz<NA*w3CceHg(RsR>wJZxnMyR4hVTiEocxLVbBu
zVaC9>zHl(n{hO1gQ$@U|n!s)@*CxoOLTA&I%cjPMjS57PO;Muz%yZAWv#HeC#B$lp
z^I@Yxm1Gl2bpPq}bM9;=>uj2H+0^;4VK57z(Lqt+MEA)Pr`*|8>1>wdvT5*P!w{He
z6M=31{8x82Q*<^hxonzz*r=c;Yg3%)e*L9q+}Tu<O|!qJ%><j~JUnf*?a|Q}+}WI`
zvvKjX**<LSo;KR{hi^XW&St94#>La-`>?Tl+GyL;FFfYX=6s!vi>EF0VPp5S(YDc%
zU%RuJrn7PJw2OS$*gb8u?UCpH=+5Q>osEm9#eCS<J#Dn@wWp4`vzbmdE&iSs1)G*U
zJgq7A$)iszHVkMuGBNE!osEm9&GBJl_q3+iGxvVdoy`oLjf<z%`mnKkT2t)1zdY#9
zW~R=@#nbA2*w{U-DR$(6AG))N>TFy*t<i^#-P4+4Lr<h5v~HVOIvW>HYxZGd_q3+i
zKS%bt)us%er!jBM^qVWSe)Gaq*t4$`V*Q?PyMxZog2JP8&{-O&-HY()qkXMA=$tGl
z{6`0!tAW~mMu_{q^=^02xmi$niw?R#1GReyHh69MoIB{eEGT?K2VJCr+WkR@s}B9Z
z9dv#c6rP}iF4jQp))(UM2j1=m8WFV|maXd8vOluSX?f#<R6wl4#BEEa2QK313yT6A
z9r2zk2kA&sj{OH%!(ZLe+qJW|BiYm6w?DGS320WQN`=}|#BE=_fX;3+3(JuNK{+tC
z7;sfVG6-SsC0}w@V?Yh|r-<XJ*zq)EPQtDVe2rMWEzz|-x;oL@yFYTPCf9kX63Dd;
zgTXFr$cy4W0qJ^C?W|N>F_42odn2`^;@CTskxD&TQmH3PD)lsE2F1d`Kx%&^>J+6Y
z04A&DPABel;=#Q2gCXRt)$)e0OF5f2D&C8p;JQqiMT0?<nWvQrrXnb_6Z^Ra<k+_>
zz3Y)&8V18C(V&$Gr^<3lIC)JG7jZ}I<-2zDcJy^5`v-20Zb<A(?vJc-LYYfa<=AYw
z3!Ca_`#x!ZZi2iSI^O_scEKMw>#(d(W2fD#7<I!0eNBHq9Cf)9*O-<HiZxJLzvil3
zTd=Ue%)*+8T`O20yCTuCrKfLe6mtbl&_Fpfdj$ld@Ap8^tq@Sn5u^*C!OXmJw}PW9
zh9Q{>ghS>;OfqcXh<!SEg$uZfe;(ke_(LPVMMVf6vx6fB>fn_w;3^J!fUBW_DZ!iV
z;E07<KnP5B0cSi^77N?eGGvyBB`~D8vTw&uxWZ1S$;>L$hdWMpz!z!sY(wK(hGjG^
zc>IUWzZ?T3#HF^LzU_%E>Gpq>Cd^dCr8a5k?J)WlM+69DyfG7ttmwl9jt~@ACN75j
zVqIcqa-aj@MGMTvbOh#g0A@`G;8P4NYDOdDDONbF`VM0mB<=8UsX>9}7<0;M2(VlS
zPalw*Aqkb%zJc59WVIN({Q$A9KbhDc>2zY4C1@|iNJ1zspL5nDMZr3W71FrIkqxx!
z$qH%e$qH%eX~--U%LG=@*Y8Y9i+@mKIt9?{r4Dj!ZV&m4kPe%Zkd;I|S(2zHOA__O
zQ1-ULBIs_8Q-q=-7@{3@9>`4msWQ6ia=Pk<o@6hywb)rIFbU?_0Pje`aMV|>w;P05
zAtCyc^Z;$`A#lEgpu&~)gVL!S>kHJo6-QKUttYxzg}4MZ64!PN^f6?q@EUgtYL+5W
zUkeTcXjfL$ato{M#HEJsifj~j*%nFmp>`Gc<96FlS=(zM^3C#YE$Yr-$Svc`pv4|>
zlk}i$4z{4J=tYwVl`Z0WxwZHX)as{PYldfBvn`QGE+*xjT(`3*O{6@{MKg;MI~bWN
z4%*~Wu1(Dg@p)|9{=DrS%iRfAH}Lnk5kbOFEnYUn%h&{s@9|?-GlK|?@e?XuOALOd
zAtn>=W1@7d64&F{s~Hh{jFs30Jz})t+$v_A=0|!=#Ujvu1YdI_f5#s3*Tg*fx=fI&
zV=+%nJjHP8T!K?q3s12W1|Jb|C3JxDqAOC3gq!qHoxYml0P1zX_GZ;dL)-`2eKs29
zy%V%MZ8QRKvxVBuUhde=UTRkHwLB>8Ni5=j9qaNFpar&h6BeB$?<=HU$|31%Up)P@
zwVmzp&5~Xx=?mrgqmnX+Jk~K6Elr4eV-Mau9we_RNVmz{x6AW9<nEToAL85zYu(po
zQ2rQSV7q^Sdu4%0YZ|aBwGflX&*Rp$#T>@4W;iPbN;r=d?D6FBE=Yll=s0;$2MoP1
zqDiarzMe{jD@>9!KIk-<F^9%_>-i8t9JJS^UDOYTgy0j=C&@FPh)eCOcP8#8F@(sR
z^yf)HQ%_ia#1nZwjkfYAuI;!^#1`d=;*>Z+l(AnSGwe4;wa};ea|R%cjew4_W3`}R
zv-+5-Hy&(<DN1BHvY-UazYo{Xl_|RXoe7-m=V&MEKpzzC2Y(dWD`i)0W~0>N9{eEV
zd2yRO-(kE0fsPyHkm`H#UIqjaFiDS_0o3kzfVIY>Y<=t!$B+i`W(;!Bh!tZvVNlx)
z&#J^v9}H2^O5P``ING2mSBr~3)O&2*>DyZmzp(YD_D`)q39F8$q-RZ#7>^j0qLR$y
z!5>oaSV0|k#8${h_QL#KggQ=%eL0Gd{_ar3lV?Q{CKM2WB7(SvT@_(M5#-%EC98<G
zqwhO(=&z^N-0|CoOKJ~a*Qo*;Ym=uUI+r*VF+lC0aIPW(@D#dt)+1`i&W1<O6M|4g
z5v~zeMFgM-^1k1uh&>CNC#?N$$@)(pcxmeBQ0r$3Rzz2erHE4L5d&1Bt0ICqig@U(
zD1uFc4Nob=t!V_I2=W%yS&!(uJNku_FRr=#uF%8N4u1c@u7VZO(PSw?H4VUfD55Aw
z5#r3*@Cc=dX;KjZ@dTvNV?+@YLEhKf6frWV<@>+deS7QuA1l58<VWtoFIgD3R9$!-
zKSnIhR}l=n&ZaSVRun-`V6bG<a1WLtD1y94Y>Ft``Oz<iBV}uk{PKp|4&VLZ)`At$
zl?j%rX|%JmK~Kc-6%Phc#}DS@JJrbsUEisI%kfBVIn_UeT;H*e<ao}l9D7Xm#`?QU
z@fGh8>BFsf&!Zo0doRXRdzCvVqImznh3q|xB@BP#g*$UAhJRaa*)WLWQhMMrzY&<#
z7b)N_jC0SO(HARdjk>IB+1lZMbM%!~EiTdu^075bbg_bo-&d(o5V9#360pF3Y=xKb
zHT>MR0+wF?r>$T!>;LMl3LXp+>i75@_%bpup0Sbh_xYSy@%q;B|EGaBunbb9<|MFk
z_Ml&!xZ|^DUR}Sf{L6Po#KQSUpJ@c`FcN)(?^}`B&0m2;U%4KM3kVosvCF}LnE{61
zN7Vm^cnDW^NWVuR?iOaJVE<7$9>=Ax?dclmzp=kN8NGa0SE5(SAVv?B@c#?6qWs`%
zGxr|FJ^eYK!>|74Q%^iK+;Bnp(7mXC{i_eN{wI*Iur2G~jw^rcun~zH+<TGu1CAsT
z>vjv$WTe}XxWsWM67_Kp67}<KB)dLHyR4_L|AuH=+iKjaI+RIFJbGSdfEn}BE^2ux
zd)t(~jfpa3Btt5nf?nG{u>D4^N8KpP=c_Z6%p3+HLjVDSEkpm`5A%>KYp?a)-923i
zWwvNjV`D6uVJ{=js-EudrI=^%6ACQbAZyaFRJ^k6$Okg)-nw~{HDIK_o$bJ?bf<WK
i!RY)C4ECs)4*=VbnoH)Bd;C0w@*f|uupH+y|9=4>nT-zs

literal 0
HcmV?d00001