<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="http://git.reviewboard.kde.org/r/106923/">http://git.reviewboard.kde.org/r/106923/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On October 17th, 2012, 5:41 p.m., <b>Albert Astals Cid</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">To be honest i don't think this is a good idea. This is poors man solution for "i don't know what do to with my memory". Personally i'd like to have a much more defined ownership model for annotations, e.g. annotations that *are* in the page belong to the page and the ones that are not in the page (i.e. they are in the "undone" actions) belong to whoever holds them (i.e. the actions).
This is something similar to what i do in the KTuberling undo/redo actions http://quickgit.kde.org/index.php?p=ktuberling.git&a=blob&f=action.cpp
What do you think of that?</pre>
</blockquote>
<p>On October 17th, 2012, 6:38 p.m., <b>Jon Mease</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Thanks for the feedback.
I started with the idea of a more defined memory management model. But once I realized that multiple undo command objects will need to reference the same annotation I had trouble envisioning what this model might be. But your implementation in KTuberling makes sense.
So the idea is that an annotation would be owned by the createAnnotation command only if it in the 'undone state'. An annotation belongs to the removeAnotation command only if it is in the 'redone state'. Annotations being displayed on the page are owned by the page. And all of the other undo commands that don't create or remove an annotation never have ownership.
Does this sound like what you're thinking?
</pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Exactly :-)</pre>
<br />
<p>- Albert</p>
<br />
<p>On October 17th, 2012, 2:24 a.m., Jon Mease wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://git.reviewboard.kde.org/media/rb/images/review_request_box_top_bg.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Okular.</div>
<div>By Jon Mease.</div>
<p style="color: grey;"><i>Updated Oct. 17, 2012, 2:24 a.m.</i></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">The purpose of this patch is to shift the responsibility for explicitly deleting annotations away from the Page class. Prior to this patch the Page class would explicitly delete an annotations object that was removed from the page. However, in order to eventually support Undoing and Redoing the creation and removal of annotations from a page it is important that the annotation objects are not automatically deleted upon removal.
The Page class now stores QSharedPointers to annotations rather than standard pointers. Page::addAnnotation and Document::addPageAnnotation now require a QSharedPointer rather than a standard pointer. The implication is that the deletion of annotation objects is now managed by the QSharedPointer class rather than the Page class.
Currently the only QSharedPointer referencing a particular annotation will be the one help by the page class and so the removal of the annotaion from a page will automatically result in the deletion of the annotation object. However, in the future several QUndoCommands may also have QSharedPointer references to the annotation and in this case the annotation will not be deleted until all of those QUndoCommands have been been destroyed.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Added a kDebug() statement to the destructor of the Annotation class. For both annotations loaded from disk and annotations created during the current session the removal of the annotation from a page still results in the destruction of the annotation object (no annotation memory leak). This is the same behavior as before, only now it is accomplished through the use of QSharedPointers rather than an explicit "delete" call.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>core/annotations.cpp <span style="color: grey">(21114af)</span></li>
<li>core/document.h <span style="color: grey">(d47acec)</span></li>
<li>core/document.cpp <span style="color: grey">(bddef39)</span></li>
<li>core/page.h <span style="color: grey">(a8f2761)</span></li>
<li>core/page.cpp <span style="color: grey">(d746382)</span></li>
<li>core/textdocumentgenerator.cpp <span style="color: grey">(f370ded)</span></li>
<li>generators/djvu/generator_djvu.cpp <span style="color: grey">(bc83ed7)</span></li>
<li>generators/poppler/generator_pdf.cpp <span style="color: grey">(fcc8dc4)</span></li>
<li>ui/annotationtools.h <span style="color: grey">(fdcae1b)</span></li>
<li>ui/annotationtools.cpp <span style="color: grey">(f2db355)</span></li>
<li>ui/pagepainter.cpp <span style="color: grey">(916a0ab)</span></li>
<li>ui/pageviewannotator.cpp <span style="color: grey">(4488639)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/106923/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>