ScPostIt revision

[Eike Rathke]

Hi Mike,

On Friday, 2015-06-19 20:19:10 +1000, Mike Kaganski wrote:

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

Please start by writing unit tests for ScPostIt usage. Create, delete,
edit, replace, copy, paste, cut, move, undo, redo in as many
combinations and orders as possible. Think of corner cases. Implement
a test for each bug that was fixed around ScPostIt code if there isn't
one already.

Then do not refactor the code, but instead fix the pending bugs you
mentioned. Write tests for these fixes. If all is fixed and you have
tests for as many cases as possible then and only then start to think
about refactoring.

Really. It doesn't help much to refactor and add some reviewers. You
will still miss the cases no one thought of while browsing the changes.

> 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 :)

Introducing shared_ptr doesn't automatically make code fail safe. It is
still possible to delete an object and later have a shared_ptr fail on
a dangling pointer. Or have an object deleted at an "unfortunate" time
while something holds the raw pointer and accesses it later.


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

That's not what Kohei said. Please re-read.


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

There *are* or there *should be*?

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

You'll need copy-on-write for that, otherwise modifying a note would
also modify the Undo note.

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

See above.

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

As long as they *are* the same you don't need deep copies.

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

Which is why copy-on-write would be good to have.

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

If you cut and paste once, no deep copy is required. Further pastes at
that stage or copy&paste also don't require deep copies, only if one of
the copies is modified it needs copy-on-write.

> 3. *Undo* objects: they always represent some state of one specific
> *ordinary* note;

Exactly.

> all *undo* notes of one *ordinary* object should share its
> data.

If the state of the ordinary note changes it needs copy-on-write.

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

What is the ScPostIt client and how would it know what the "copy" will
be used for?

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

And that "two instances claim ownership" needs to be taken into account.
You can solve it with shared_ptr, if a proper copy-on-write is
implemented and the copy is triggered only from Calc core and not by the
drawing layer.

> 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? :) )

Yes :-)

  Eike

-- 
LibreOffice Calc developer. Number formatter stricken i18n transpositionizer.
GPG key "ID" 0x65632D3A - 2265 D7F3 A7B0 95CC 3918  630B 6A6C D5B7 6563 2D3A
Better use 64-bit 0x6A6CD5B765632D3A here is why: https://evil32.com/
Care about Free Software, support the FSFE https://fsfe.org/support/?erack
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/libreoffice/attachments/20150619/c5de16d5/attachment.sig>