<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/107442/">http://git.reviewboard.kde.org/r/107442/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On March 14th, 2013, 7:43 p.m. UTC, <b>Albert Astals Cid</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="http://git.reviewboard.kde.org/r/107442/diff/6/?file=119425#file119425line3282" style="color: black; font-weight: bold; text-decoration: underline;">core/document.cpp</a>
<span style="font-weight: normal;">
(Diff revision 6)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">bool Document::canModifyPageAnnotation( const Annotation * annotation ) const</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">2819</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">if</span> <span class="p">(</span> <span class="n">d</span><span class="o">-></span><span class="n">m_annotationBeingMoved</span> <span class="p">)</span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">3241</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">QString</span> <span class="n">prevContents</span> <span class="o">=</span> <span class="n">d</span><span class="o">-></span><span class="n">m_prevAnnotContents</span><span class="p">[</span><span class="n">annotation</span><span class="p">];</span></pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I am wondering why do we need this map for the prev contents, as far as i can see annotwindow does not modify the annotation anymore, so the annotation * you get here should have the old values still no?</pre>
</blockquote>
<p>On March 16th, 2013, 1:55 a.m. UTC, <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;">Yes, we could assume that the old contents are still in the annotation but this makes me a little nervous because there's nothing stopping a user of Okular core from modifying the annotation's contents as well which would then break the undo stack. But we could do it this way and just add API documentation specifying that the annotation's contents should not be updated.
If we do go this route I have a question. The AnnotationWindow class uses the function GuiUtils::contents rather than Annotation::contents when retrieving the contents of an annotation. Would the Document class need to use this GuiUtils version as well in order to grab the old contents (rather than Annotation::contents?). I'm not really clear from the source what conditions the GuiUtils version is handling. If so we'll need to move this function out of GuiUtils (because of the GUI dependency) and into some other core class. Let me know if you have a preference of where to move it to.</pre>
</blockquote>
<p>On March 16th, 2013, 11:52 p.m. UTC, <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;">"there's nothing stopping a user of Okular core from modifying the annotation's contents"
Sure, there's nothing stopping people from doing int *a = 0; *a = 33; either :D We have to document stuff properly and do the best we can to review new stuff so that it does not break
About the second part, yes, it seems we'd need that logic somewhere, as you are already doing some special casing in DocumentPrivate::performSetAnnotationContents, honestly i can't really say what the GuiUtils code does either :D About a name, I guess we could always go with Document::annotationContents(Annotation *);</pre>
</blockquote>
<p>On March 17th, 2013, 1:45 a.m. UTC, <b>Fabio D'Urso</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;">About the first check in GuiUtils::contents ( m_annot->window().text() ): it is correct in principle, because under some circumstances the popup windows's text overrides the actual annotation contents. However, such circumstances are currently not even detectable outside Poppler (*). Furthermore, Poppler-qt4 never sets popup windows' text and always returns an empty string. So I say let's keep it simple and remove it. We can solve this issue at a lower level in Poppler itself in the future.
* = [unimportant detail] this happens if the /Popup annotation has *no* optional /Parent field (see PDF 32000-1:2008, table 183)
About the second check: in Poppler, inplaceText is a synonym for contents. It used to be a separate thing but now it only exists for source compatibility purposes. So I think we can kill it in Okular too and always use annotation->contents(). Of course it needs to be done in a backward-compatible way (we don't want users losing their saved annotations)
tldr: You can remove TextAnnotation's inplaceText/setInplaceText, ignore all conditions listed in GuiUtils::contents, kill GuiUtils::contents and always use plain Annotation::contents/setContents :D</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Ok, I removed GuiUtils::contents and replaced all usages with plain Annotation::contents. I didn't remove the inplaceText methods and member because these are used in the QDomNode code to store and retrieve annotation properties and I'm not comfortable enough to change these in a backwards compatible way.
With this change I was able to get rid of the previous contents map and instead grab the previous contents from the annotation itself.
Feel free to re-open this issue if you have any other concerns</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On March 14th, 2013, 7:43 p.m. UTC, <b>Albert Astals Cid</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="http://git.reviewboard.kde.org/r/107442/diff/6/?file=119429#file119429line209" style="color: black; font-weight: bold; text-decoration: underline;">ui/annotationpropertiesdialog.cpp</a>
<span style="font-weight: normal;">
(Diff revision 6)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">void AnnotsPropertiesDialog::slotapply()</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">209</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">m_document</span><span class="o">-></span><span class="n">modifyPageAnnotation</span><span class="p">(</span> <span class="n">m_page</span><span class="p">,</span> <span class="n">m_annot</span> <span class="p">);</span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">209</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">m_document</span><span class="o">-></span><span class="n">modifyPageAnnotation<span class="hl">Properties</span></span><span class="p">(</span> <span class="n">m_page</span><span class="p">,</span> <span class="n">m_annot</span><span class="p"><span class="hl">,</span></span><span class="hl"> </span><span class="nb"><span class="hl">true</span></span> <span class="p">);</span></pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Maybe we could not modify the annot in ::slotapply but modify it in Document::modifyPageAnnotationProperties? And then you wouldn't need the map in there either because you would get the "old" annotation as parameter?</pre>
</blockquote>
<p>On March 16th, 2013, 1:55 a.m. UTC, <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;">The difficulty here is that most of the work in modifying the annotation's properties is actually performed in the applyChanges() method of the AnnotationWidget (m_annotWidget). And we can't pass the AnnotationWidget into the Document::modifyPageAnnotationProperties method because of the GUI dependency. So I'm not really sure how this could be done (but I'm open to suggestions).</pre>
</blockquote>
<p>On March 16th, 2013, 11:52 p.m. UTC, <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;">Hmmm, i see, maybe we could go to a two step process?
Like
m_document->startAnnotModification();
annot->setStuff();
m_document->endAnnotModification();
It's not ultra nice, but I don't really like maintainging caches of stuff, they will eventually get out of sync, so if it helps, i'm all for it.</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I got rid of the previous annotations map by adding a Document::prepareToModifyAnnotationProperties method. Document::modifyPageAnnotationProperties now checks to make sure that prepareToModifyAnnotationProperties was called previously.
Feel free to re-open this issue if you have other concerns</pre>
<br />
<p>- Jon</p>
<br />
<p>On March 24th, 2013, 12:40 a.m. UTC, Jon Mease wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://git.reviewboard.kde.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.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 March 24, 2013, 12:40 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;">This patch adds undo/redo support to Okular's annotation manipulation commands.
Functionality:
The following actions can be undone and redone: creation and removal of annotations, editing arbitrary annotation properties, relocating annotations with Ctrl+drag, and editing the text contents of an annotation.
This patch does not include support for undoing and redoing editing actions on forms.
</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;">I have tested the undoing and redoing of the specified annotation actions using .dvi and .pdf documents.</pre>
</td>
</tr>
</table>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="http://bugs.kde.org/show_bug.cgi?id=177501">177501</a>
</div>
<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.h <span style="color: grey">(72abdff)</span></li>
<li>core/annotations.cpp <span style="color: grey">(49ab5bd)</span></li>
<li>core/annotations_p.h <span style="color: grey">(221572d)</span></li>
<li>core/document.h <span style="color: grey">(6ff6536)</span></li>
<li>core/document.cpp <span style="color: grey">(5ab759e)</span></li>
<li>core/document_p.h <span style="color: grey">(fb3aec6)</span></li>
<li>core/page.cpp <span style="color: grey">(1db2763)</span></li>
<li>part.rc <span style="color: grey">(39c1571)</span></li>
<li>ui/annotationpropertiesdialog.cpp <span style="color: grey">(4b02258)</span></li>
<li>ui/annotwindow.h <span style="color: grey">(f7df9f6)</span></li>
<li>ui/annotwindow.cpp <span style="color: grey">(c1bafb9)</span></li>
<li>ui/guiutils.h <span style="color: grey">(2ae4ab3)</span></li>
<li>ui/guiutils.cpp <span style="color: grey">(1d67d3a)</span></li>
<li>ui/pageview.cpp <span style="color: grey">(b018dfe)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/107442/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>