<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/110589/">http://git.reviewboard.kde.org/r/110589/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On June 1st, 2013, 3:40 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;">Looks good for me besides one small thing, in editFormList and editFormCombo you are passing the "old" values while in editFormText and editFormButtons you are not (you get them from the forms themselves) is it possible not to pass the old values to editFormList and editFormCombo too?</pre>
</blockquote>
<p>On June 1st, 2013, 9:24 p.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;">This is the approach I originally attempted. However, I couldn't figure out how to reliably get the current text out of a FormFieldChoice object across different versions of Poppler. One complicating factor is that the PopplerFormFieldChoice::editChoice and setEditChoice methods are only meaningfully implemented for versions >0.22 of Poppler so getting this right would require some ifdefs.
That said, I'm open to implementation suggestions (and fine if you want to change anything along these lines after committing). Thanks!</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;">I don't understand why you need to interact with Poppler* at all? I mean the ui/* files don't so why do you need to? Isn't FormFieldChoice::currentChoices what you need?</pre>
<br />
<p>- Albert</p>
<br />
<p>On May 22nd, 2013, 3:41 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 May 22, 2013, 3:41 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;">Add undo / redo support for forms. Along with the previous annotation undo support I believe this completes the implementation of undo/redo for all document editing actions in Okular (Bug 177501). This review request corresponds to the "Undo/Redo support in PDF forms" feature in the 4.11 feature plan (http://techbase.kde.org/Schedules/KDE4/4.11_Feature_Plan)
Potential issue: If the last form or annotation that was modified is outside of the document viewport we are not currently moving the viewport to the form or annotation and so it can be unclear what has been undone. I would appreciate suggestions on whether we should add this viewport shifting and, if so, how best to go about implementing it. </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;">Manual testing on a variety of PDFs including forms. I've attached three such documents below.</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/document.h <span style="color: grey">(d443917)</span></li>
<li>core/document.cpp <span style="color: grey">(2732441)</span></li>
<li>core/documentcommands.cpp <span style="color: grey">(5fcc195)</span></li>
<li>core/documentcommands_p.h <span style="color: grey">(a9775a6)</span></li>
<li>ui/formwidgets.h <span style="color: grey">(24108b8)</span></li>
<li>ui/formwidgets.cpp <span style="color: grey">(57ecceb)</span></li>
<li>ui/pageview.h <span style="color: grey">(5e839f2)</span></li>
<li>ui/pageview.cpp <span style="color: grey">(6e093ef)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/110589/diff/" style="margin-left: 3em;">View Diff</a></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">File Attachments </h1>
<ul>
<li><a href="http://git.reviewboard.kde.org/media/uploaded/files/2013/05/22/forms-scribus.pdf">Mixed forms 1</a></li>
<li><a href="http://git.reviewboard.kde.org/media/uploaded/files/2013/05/22/formSamples.pdf">formSamples.pdf</a></li>
<li><a href="http://git.reviewboard.kde.org/media/uploaded/files/2013/05/22/stripped-doc.pdf">Exclusive checkboxes</a></li>
</ul>
</td>
</tr>
</table>
</div>
</body>
</html>