ScPostIt revision

[Mike Kaganski]

Hello Markus,

thank you very much for your comments!

6/19/2015 3:17 AM, Markus Mohrhard пишет:
> On Wed, Jun 17, 2015 at 12:20 PM, Mike Kaganski 
> <mike.kaganski at collabora.com <mailto:mike.kaganski at collabora.com>> 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.
>
>
> Notes are not a sim[ple object as they combine a drawinglayer object + 
> a calc core object. So the lifecycle of these will always be a bit 
> more complicated than that of a normal cell, ...

I totally agree. But *if* some aspect of that could be made simpler, 
without sacrificing, isn't it worth doing that?

>
>     Depending on the its document kind, the ScPostIt behaviour differ:
>     e.g. when in Undo document, it doesn't try to delete caption
>     object it points to, because the caption is "shared" with an
>     "ordinary" note.
>
>
> That is the correct and useful behavior. You don't want to create new 
> drawinglayer objects in copy and undo documents if possible as they 
> are quite expensive and are causing quite a lot of pain.

No doubt. I just describe what I see.

>
>     One possible approach is to avoid multiple pointers to same
>     ScPostIt/SdrCaptionObj, and always do deep copies. While somewhat
>     more expensive, taking into account small number of notes that are
>     typically in a document, only a small overhead should be expected.
>     However, it may lead to much more widespread changes: e.g.,
>     undoing any modification of the note itself (say, typing a text)
>     should not expect to find the object it was created for, but
>     instead always re-create new notes. I suppose that this route is
>     not optimal.
>
>
> This assumption will not hold. There are some users, notably Laurent, 
> who have a huge number of notes and who spend quite some optimizing 
> the notes storage. As Laurent just recently spend a few days/weeks on 
> optimizing the notes storage and handling of documents with many notes 
> it would be good not to just dismiss his use case.

Well, that's why I asked for opinions. Laurent has already posted his 
concerns, and I see that there's no way to assume that the overhead is 
justified. However, I never told I like this approach. Actually, I said 
that I don't like this way in the last sentence of cited paragraph 
(based on other grounds, but still).

>
>     Another one is to keep as much of current approach as possible. I
>     suppose that the following policy should be established for the
>     objects (and documented in code): when copying a "ordinary" note
>     to another cell of ordinary document - regardless if it's the same
>     document or another - always do deep copy. If copying "ordinary"
>     note to/from undo, then only do "shallow" copy, and the shared
>     reference to SdrCaptionObj must be made using std::shared_ptr.
>     This will allow to keep the object while there's at least one
>     client is alive. Passing the pointer of SdrCaptionObj to other
>     **owners** should be done via shared_ptr, too. Copying to/from
>     clipboard must be a deep copy.
>
>
>
> IMHO we should use a shared_ptr for the drawing layer object but it 
> will not solve everything (it still means you need to use the correct 
> method in the callers). The general architecture is well known and 
> worked for a long time. The problem lies in the details and large 
> refactorings like the calc core change. In general the problem with 
> the notes lifecycle are mostly in the callers and not in the 
> implementation itself.

Automatic memory management never solves all problems. When I will say 
"Let's do foo and fix all calc problems at once!", then please call 
psychiatrist. But the shared owning of one resource by several others 
(ordinary and its undo objects) is naturally fits in the purpose of std 
class shared_ptr, isn't it? Please don't put other words that I hadn't 
spoke into my mouth ;)

> My suggestion would be to start small with something simple as moving 
> to shared_ptr and adding asserts that make sure that the assumptions 
> that you have are valid. If after that task and realizing that at 
> least 2 calc developers only managed to fix some of the problems 
> around notes and note captions I recommend starting adding some tests 
> before implementing something new. You'd be surprised to discover how 
> many corner cases there are that you never thought about but are 
> currently handled in the code.

And what I actually intended was described as "to keep as much of 
current approach as possible". That should sound coherent with what you 
suggest. I don't want to drastically change the logic; I just want to 
adjust interface in the way that programmers could understand well when 
to call what ctor/method, and move some resource-management logic from 
caller to the class itself (when it is doable and meaningful).

Thank you very much!

-- 
Best regards,
Mike Kaganski
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/libreoffice/attachments/20150619/4f3e968a/attachment.html>