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
Attachment:
pgpJVXAbX76Sx.pgp
Description: PGP signature