Re: ScPostIt revision

Hello Kohei,

I hoped very much that you could take a look at this and comment here. 
Thank you!

6/19/2015 11:42 AM, Kohei Yoshida пишет:
> On Wed, 2015-06-17 at 20:20 +1000, Mike Kaganski wrote:
> > 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. ;-)

Well, yes. :) However, I have some resources to spend on this topic; and 
I *suppose* that I see a little improvement that could make the usage of 
the class more intuitive. I suppose I understand *why* the bugs were 
introduced, and what changes would easily prevent that from happening. 
Probably I'm wrong; anyway, I ask you and Markus to allow me adding you 
to reviewers when I have something to review (on gerrit). And I'm sure 
that if it turns to be unworthy, it will be rejected.

> > 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.

I'm sorry, but you are wrong. That was the first approach that I 
described, and it is not one that I suggest. The second (preferred) one 
was to use smart pointers to allow keep sharing objects among owners, 
while ensuring correct memory management. I think that sharing pointers 
fits well to shared_ptr intended usage :)

> 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 see, and I understand that there is some inherent complexity. But I 
don't see how saying "it's too complicated; let it be as it is" helps. 
Actually, I wanted to get specific comments about my coverage of use 
cases, and their intended handling.

I want to describe them once again. I think that there are only three 
distinct types of notes out there:
1. *Ordinary* objects: they belong to one and only one cell in a 
document. They don't share anything with no other *ordinary* note, nor 
with *clipboard* notes. But they always share data with *undo* notes. 
They can be copied to any other type of notes:
- to other *ordinary* notes (copy to other cells of same or different 
document) - then no data should be shared, but fully copied - or maybe a 
"copy-on-change" logic could be implemented, but now there's no such 
logic, so *for now*, full copy is just as it is intended to work;
- to *undo* notes - always do shallow copy;
- to *clipboard* notes - then do deep copy; but the copy data should 
also be able to live without drawing layer. I believe that 
cut-to-clipboard should be treated uniformly, because there are *undo* 
objects that share data with original object, so transferring it to 
clipboard is prone to problems.
2. *Clipboard* notes. They should not share anything with any other 
note. They should be self-dependent, because otherwise (a) the clipboard 
contents could change by changing *ordinary* notes after 
copy-to-clipboard was made; and (b) clipboard should be able to live 
independently of original document and its objects. They can only be 
copied to *ordinary* notes, not to *undo* or other *clipboard* notes. 
When copying to *ordinary* note, deep copy is required, because paste 
from clipboard can be made multiple times, so independence is required.
3. *Undo* objects: they always represent some state of one specific 
*ordinary* note; all *undo* notes of one *ordinary* object should share 
its data. Their life is limited by original document - so no need to do 
expensive copies to manage memory; shared_ptr could be enough. They 
should only be copied to *ordinary* note, specifically - to the note of 
the cell they were originally created for. Shallow copy is required for 
that. Thus, they should at all times share data with exactly zero or one 
*ordinary* note (zero if original cell note was removed). Never copy 
them to *clipboard* or other *undo* notes.

That is what I see in code, and what I suppose is sane usage of the 
class. From this I draw the conclusion that smart memory management in 
this topic only depends on object *usage types*, and not on other 
circumstances. But I may well miss something (I'm sure I do), that's why 
I ask to point me to the usage I oversee. I want to build logic upon the 
stated usage modes, to make most optimal memory management automatically 
(ScPostIt client just need to specify the intended usage, and don't 
worry how not to mess things), not to make full copies at all times.

> 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.

I don't see it as "counter-proposal", just necessary seat belts. Thank 
you for your concerns!
I don't rush into breaking everything; actually you see, that I do spend 
time to understand what goes on with the notes *prior* to making deep 
changes, including consulting with experienced LO developers. So I do 
take your advise seriously.

P.S. And I do understand that there need to be a number of unit tests. 
(It's sad, because I don't like writing them, but they are necessary, so 
I'll just have to provide them, or you will ban the changes, won't you? :) )

--
Best regards,
Mike Kaganski