Hi Kohei, On Thursday, 2012-01-26 16:03:56 -0500, Kohei Yoshida wrote:
> http://cgit.freedesktop.org/libreoffice/core/commit/?id=af70bc00c6714eb8695babdf5af07416552f7034 > > cherry-picked to 3-5 and preferably to 3-5-0 as well. IMO this is a > safe change. > > The change consists of two parts. The real bug is in the first part > (ScColumn::DeleteRange), where a note cell with live broadcaster would > silently get deleted, hence losing link to its listeners. if (eCellType != CELLTYPE_NOTE) { // do not rescue note if it has to be deleted according to passed flags ScPostIt* pNote = bDeleteNote ? 0 : pOldCell->ReleaseNote(); // #i99844# do not release broadcaster from old cell, it still has to notify deleted content SvtBroadcaster* pBC = pOldCell->GetBroadcaster(); if( pNote || pBC ) pNoteCell = new ScNoteCell( pNote, pBC ); } + else + { + SvtBroadcaster* pBC = pOldCell->GetBroadcaster(); + if (pBC && pBC->HasListeners()) + pNoteCell = new ScNoteCell(pOldCell->ReleaseNote(), pBC); + } Isn't now the case missing where notes themself shall be deleted? It seems that ScPostIt* pNote = bDeleteNote ? 0 : pOldCell->ReleaseNote(); should also be done for the new code, so effectively the if/else wouldn't be needed anymore. It's late now and I didn't try and maybe I'm tired and wrong.. And ... while otherwise the patch seems to do what it should it occurred to me that * it creates an ScNoteCell with a broadcaster obtained from pOldCell * where pOldCell is an ScNoteCell, at least because it's in the else of if (eCellType != CELLTYPE_NOTE) it should be one.. * the following if (pNoteCell) sets maItems[nIdx].pCell = pNoteCell; Wouldn't it be equivalent to not create another ScNoteCell and replace the existing ScNoteCell to delete it, but leave the existing ScNoteCell in place instead? i.e. if (eCellType != CELLTYPE_NOTE) { ... } else { if (bDeleteNote) pOldCell->ReleaseNote(); pNoteCell = pOldCell; } Further down the pOldCell->ReleaseBroadcaster() and pOldCell->Delete() would only have to be executed if (pOldCell != pNoteCell) Maybe worth a thought. > Then the second part is in ScDPOutput::Output(), where the old content > gets removed before writing out a new one. The thing is, this method > only gets called from ScDPObject::Output(), and that method already > deletes the old content which makes the removal code in ScDPOutput > unnecessary. So, when updating or refreshing the pivot table output, > calc was essentially deleting the range twice. That's indeed unnecessary. Eike -- LibreOffice Calc developer. Number formatter stricken i18n transpositionizer. GnuPG key 0x293C05FD : 997A 4C60 CE41 0149 0DB3 9E96 2F1A D073 293C 05FD
pgpEUrds2Iy8D.pgp
Description: PGP signature
_______________________________________________ LibreOffice mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/libreoffice
