Re: [Libreoffice] [REVIEW][3-5][3-5-0] Fix for formula reference chain breakage during pivot table update

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

Attachment: pgpPB_rbSeSOj.pgp
Description: PGP signature