Re: ScPostIt revision

On Wed, 2015-06-17 at 20:20 +1000, Mike Kaganski wrote:
> Hello! 
>
> While working on tdf#89226, I found out that there are a number of
> places in calc where ScPostIt's, as well as the SdrCaptionObj's
> (controlled by ScPostIt) are used after delete. 
>
> Turns out that quite a few regressions issued in bugzilla are rooted
> to that. To number a few: 
> tdf#89226 
> tdf#83192 
> tdf#91995 
> tdf#90741 
> tdf#83192 
>
> Another crash that I haven't issued to bugzilla (it would only add up
> to those already mentioned; don't know if it is already reported):
> Copy a cell with comment to clipboard → close original document →
> paste to another document. 

This is a corner case that is known to create all sorts of issues due to
the way Calc interacts with the system clipboard.  This itself alone is
not good enough evidence that there is a need for redesign with the note
handling.
> So, I thought it's  manifestation of obscure interface/protocol of the
> objects that leads to broken conditions when implementers use them.
> Currently I'm evaluating possible changes to (mainly) ScPostIt that
> would make its usage more safe and robust. 

Not in my opinion.  Bugs happen, and they need to be fixed.  But I think
you are jumping to a conclusion a bit too quickly.  Seeing a few bugs
(to me 6 bugs is a "few") and thinking that "hey, we need a total
redesign of foo" is what I'm seeing here.  A little premature move.

If you think the way notes are handled is too obscure, you haven't seen
nothing yet. ;-)

[...]

> That's a summary of what I'm trying to do. I'm very thankful for
> anything you'd like to share on this topic. Thank you. 

So, to me, a summary of what you are proposing is that "trying to be
clever with manual memory management is too complicated, let's just
always make copies of objects so that we don't have to think about
this." which is the wrong mindset to have especially when you work on a
very performance sensitive area such as Calc core.

The unfortunate reality is that in Calc core, you *do* need to be smart
about object life cycles to avoid poor run-time performance.  This is
because 1) object creation and deletion is so darn expensive especially
when millions of instances are involved, which the note objects
certainly are, and 2) we've optimized the hell out of the current note
handling to make Calc scale enough to handle hundreds of thousands of
note instances.  This unfortunately means that we do have to take every
opportunities to avoid unnecessary coping of objects, and also means
that (as you see) it has caused unfortunate crasher bugs needing to be
squashed, but that's usually the way the games are played around Calc
core.  I'm not necessarily fond of it, but that's the way it is.

I'm not saying that there is not a better way to handle note's life
cycles.  As Markus pointed out, there is an unfortunate complexity in
how the notes are currently handled in large part because two separate
entities - Calc core and the drawing layer - claim ownership and memory
management responsibility to the note objects.

Anyway, I'm very sorry to have to counter your proposal this way.  You
are obviously very brave to want to dive into Calc core and unwind its
huge mess, which should be commended. :-)  But my advice to you is to
spend a bit more time and effort in trying to deepen your understanding
of how Calc interacts with the drawing layer to handle notes, then get
back to the subject to improving the design of notes.  Either way, as
Markus pointed out, incremental approaches would help, not a big-bang
re-design.

Best,

Kohei