<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="12" style="border: 1px #c9c399 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/127366/">https://git.reviewboard.kde.org/r/127366/</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, 2016, 12:46 a.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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Looks very nice for a start, congratulations.</p>
<blockquote style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid #bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Reconsider UI, esp. the Ctrl-way to get focus. Is there something in the KDE HIG about it?</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">It's not too bad, i think it would be ok, but if you awnt to try asking in the kde-usability mailing list it may help (sometimes it does not though, so be prepared :D)</p>
<blockquote style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid #bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Improve handling when annotation gets so small that resize handles overlap.</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I guess in that case one could try to always default to one of the corner ones, so you're forced to make it bigger by 2 dimensions?</p>
<blockquote style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid #bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Find approach how to handle "resize to negative geometry".</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I think not letting people go negative is what makes more sense</p>
<blockquote style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid #bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Known Bug: In the thumbnail list, resize handles are drawn too big, and not refreshed correctly.</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I'd say it makes more sense to not draw the handles at all</p></pre>
</blockquote>
<p>On March 18th, 2016, 8:33 p.m. UTC, <b>Tobias Deiminger</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Fixed with Revision 2:
-set correct resize mode if page is rotated
-don't allow resize to smaller than 0 x 0
-don't draw resize handles in thumbnail list</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Changed:
-resize handles are filled slightly transparent</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Drawing the handles is now optional, and only enabled in PageView::drawDocumentOnPainter.
Should it be done somewhere else, too? E.g., there is active/components/pageitem.cpp, but sadly I don't have a clue when/by whom this is used.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Regarding kde-usability mailing list, I think before posting there I'll try to find some good KDE reference application and check what they do. I'd consider Okular as a good reference app, but it won't help me in this case :-) Some ideas come already from gwenview.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Is there something that you think should be done soon, or as next change? If not I'll just go on with what comes to my mind, and will probably update here in 1..2 weeks.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Btw., sorry, I don't know how inline quote here (using the reviewboard webinterface).</p></pre>
</blockquote>
<p>On March 21st, 2016, 7:20 a.m. UTC, <b>Jonathan Schultz</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I like it a lot in terms what it lets the user do, but agree that the use of the Control key is a bit unusual. I also have an interest in this because it overlaps with stuff I am working on to do more with selections.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">My sense is that annotations should be select-able, by left-clicking on some obvious part of them, and once selected can then be moved/resized, (and eventually also cut/copied/deleted/properties/etc). This seems more consistent with UIs that people are used to.</p></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;"><blockquote style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid #bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Should it be done somewhere else, too? E.g., there is active/components/pageitem.cpp, but sadly I don't have a clue when/by whom this is used.</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">It's for the "mobile" version of the app, it doesn't have the complex interaction modes (i think) so ignore for now.</p>
<blockquote style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid #bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Btw., sorry, I don't know how inline quote here (using the reviewboard webinterface).</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">It's using markdown, see the "Markdown Reference" link, but it's basically prefixing the line by > and then having an empty line for adding your answer</p></pre>
<br />
<p>- Albert</p>
<br />
<p>On March 18th, 2016, 8:06 p.m. UTC, Tobias Deiminger wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="12" style="border: 1px #888a85 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
<div>Review request for Okular.</div>
<div>By Tobias Deiminger.</div>
<p style="color: grey;"><i>Updated March 18, 2016, 8:06 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
okular
</div>
<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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">This diff adds a first version of an annotation resize feature to okular (see Bug 177778). Do you think the approach is worth carrying on? If so, I'm looking forward to read your comments on everything below, esp. the TODO items.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Usage:
If you press Ctrl while mouse cursor is over an annotation, you'll get focus on that annotation. Now keep Ctrl pressed, and move mouse cursor over one of the 8 resize handles (each 10x10 pixels) on the corners/edges. The cursor will change shape to indicate resize mode. Clicking somewhere else on the annotation means "move", just as it was before resize feature was added.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Notable changes:
It works by eventually changing AnnotationPrivate::m_boundary and notifying generator (i.e. poppler) about that change, similar to the existing move functionality.
-Separated annotation state handling out of PageView into a new class MouseAnnotation (ui/pageviewmouseannotation.cpp).
-Added method Document::resizePageAnnotation, backed by a QUndoCommand class Okular::ResizeAnnotationCommand.
-Added method Annotation::resize functions and new Annotation flags BeingResized and BeingFocused.
-Draw resize handles in PagePainter::paintCroppedPageOnPainter
-Draw only a bounding rectangle during resize, if annotation is rendered externally</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">TODO:
-Reconsider UI, esp. the Ctrl-way to get focus. Is there something in the KDE HIG about it?
-Feature is currently limited to annotation of type AText and AStamp => extend to all types where applicable.
-Improve handling when annotation gets so small that resize handles overlap.
-Reconsider name of class MouseAnnotation. What does it do, and what's a good name for that?
-Reconsider interface between PageView and MouseAnnotation (currently it's just forwarding UI events together with PageViewItem and event positions).
-Add test cases once requirements are fixed.</p></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>CMakeLists.txt <span style="color: grey">(97e8db6e4a704fd34331fad7b7628ca2248b62d8)</span></li>
<li>core/annotations.h <span style="color: grey">(4f107440dc824fd9049a30082befd18642e63895)</span></li>
<li>core/annotations.cpp <span style="color: grey">(e02994688414bdf485b308d6ef122ee2eff3fbaf)</span></li>
<li>core/annotations_p.h <span style="color: grey">(07b124a4fae40b7a983aa382ae824125e6d25746)</span></li>
<li>core/document.h <span style="color: grey">(63f58741bd6680a673945a7b7c05a10130968beb)</span></li>
<li>core/document.cpp <span style="color: grey">(6953b1fb0dc29a375be7ff331a2a2bccce975366)</span></li>
<li>core/document_p.h <span style="color: grey">(fda23275033645ea67f5ad9d27341fc4635ede34)</span></li>
<li>core/documentcommands.cpp <span style="color: grey">(95aded51d73a3d3b98ff26284c9c46fc5c9cf0ca)</span></li>
<li>core/documentcommands_p.h <span style="color: grey">(17394f2a25b187cf4aff66b3a7f891b81be5acdd)</span></li>
<li>generators/poppler/annots.cpp <span style="color: grey">(8cde64833831ec833b3be552608cff99d38f8e63)</span></li>
<li>ui/pagepainter.h <span style="color: grey">(68b241658162d9bd6eb187efc594ef17ea99d899)</span></li>
<li>ui/pagepainter.cpp <span style="color: grey">(57e8620cdefc36e40660fce242d7ea8197c25339)</span></li>
<li>ui/pageview.cpp <span style="color: grey">(3ebf7dcb04aa1942e02f49133d98081e2bbb565c)</span></li>
<li>ui/pageviewmouseannotation.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>ui/pageviewmouseannotation.cpp <span style="color: grey">(PRE-CREATION)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/127366/diff/" style="margin-left: 3em;">View Diff</a></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">File Attachments </h1>
<li><a href="https://git.reviewboard.kde.org/media/uploaded/files/2016/03/18/c9141599-8eb1-4054-8eb3-d81eec44fb94__annotationresize.diff">annotationresize.diff</a></li>
<li><a href="https://git.reviewboard.kde.org/media/uploaded/files/2016/03/18/9d63276d-4765-406f-ad1d-6d4176b88c53__annotationresize.diff">annotationresize.diff</a></li>
</ul>
</td>
</tr>
</table>
</div>
</body>
</html>