<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 April 8th, 2016, 7:52 a.m. AEST, <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;">Sorry I've been meaning to test this a bit more and only got to it today.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I have a couple of minor comments:</p>
<ol style="padding: 0;text-rendering: inherit;margin: 0 0 0 2em;line-height: inherit;white-space: normal;">
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The corner boxes do not seem to work - even though the mouse icon changes to indicate resizing, left-clicking causes the annotation to become unselected. That looks like such an obvious thing that perhaps it is a bug that has crept in, or is strangely dependent on my particular build?</p>
</li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The mouse icon changes could be made much better. Could the cursor not first (ie before an annotation has been selected) change to a pointer when over a point that left-clicking would select an annotation? When an annotation is selected, the "move annotation" (ie the vertical cross) is currently maintained even when the mouse cursor is no longer over the annotation itself, ie that left-clicking will not produce movement of the annotation.</p>
</li>
</ol>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">More generally in terms of UI I really like the "select then operate on" model, and would suggest that, once an annotation has been selected, a range of functions (most obviously delete/edit/properties of annotation but also cut/copy and others) then become accessible through a context-dependent right-click menu?</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;">Sounds interesting... who would eventually care about merging?</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I think you are way ahead of me, and I am looking forward to going over your patch to learn how to do a few things. That said, I have contributed one tiny review request: https://git.reviewboard.kde.org/r/127496/ but haven't had any response yet.</p></pre>
</blockquote>
<p>On April 8th, 2016, 4:17 p.m. AEST, <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;">Thanks a lot for testing!</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;">left-clicking causes the annotation to become unselected</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I can't reproduce it yet. The last diff is a few commits behind now. But even after rebase and clean build it correctly starts resizing after left click for me.</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;">Could the cursor not first (ie before an annotation has been selected) change to a pointer when over a point that left-clicking would select an annotation?</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Yes, I'd like that too, will try it. It means to check for what's under the mouse more frequently (with every move event, not just every click event), but that's not too bad I guess. Finally it'll be up to Albert and his team whether they like the UI.</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;">(ie the vertical cross) is currently maintained even when the mouse cursor is no longer over the annotation</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Again, I could not reproduce that. There seems to be some difference between your build and mine. I'll try to merge the diff into a clean clone and see what happens then.</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;">I am looking forward to going over your patch to learn how to do a few things.</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I'm also at learning, don't trust too much in my changes ;-)</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;">https://git.reviewboard.kde.org/r/127496/ but haven't had any response yet.</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Can't promise when, but will try to test yours too, thanks again!</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Great to see progress on this issue!</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Can I put in a plea for a slight extension to functionality? Rather than making resize available only after an annotation has been created, it would be better if it were also available immediately after the selection is first made, that is before the selection has been turned into an annotation.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">What are the advantages?</p>
<ol style="padding: 0;text-rendering: inherit;margin: 0 0 0 2em;line-height: inherit;white-space: normal;">
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Increased functionality. If the user is selecting not to create an annotation but to copy, perform text-to-speech or whatever else is or will become available, they could make an adjustment instead of having to go and start again from scratch if they found that the selection wasn't quite right.</p>
</li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">More intuitive. Select-adjust-create annotation feels much more straightfoward to me than select-create annotation-adjust annotation.</p>
</li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">More consistent. Currently, when doing regular (ie not text) selection, the pop-up menu appears immediately on releasing the mouse button. This behaviour is jarring and inconsistent with comparable GUIs. This inconsistency would be removed by this proposed change.</p>
</li>
</ol>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Disadvantages?</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I apologise that I still haven't looked over the code of this change, so perhaps I am underestimating how difficult it would be to implement.</p></pre>
<br />
<p>- Jonathan</p>
<br />
<p>On January 27th, 2017, 8:47 a.m. AEDT, 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 Jan. 27, 2017, 8:47 a.m.</i></p>
<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=177778">177778</a>
</div>
<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 an annotation resize feature to okular (see Bug 177778).</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Usage:
If you left-click at an annotation, it gets selected and 8 resize handles appear on the corners/edges of the selection rectangle. When cursor is moved over one of the handles, the cursor shape indicates resize mode (everywhere else on the annotation means "move", just as it was before resize feature was added). Press ESC, or click an area outside the annotation to cancel selection. Feature is only applicable for annotation types AText, AStamp and AGeom.</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::adjustPageAnnotation, backed by a QUndoCommand class Okular::AdjustAnnotationCommand
-Added method Annotation::adjust
-Draw resize handles and selection boundary in MouseAnnotation::routePaint
-Draw only a bounding rectangle during resize, if annotation is rendered externally
Some functionality unrelated to the resize feature is also shifted to class MouseAnnotation, to establish a single place of responsibility for annotation interactions.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Known Bugs:
-A comment annotation (subtype 1) with embedded appearance can be selected for resize. But the resize changes only the selection rectangle, while the annotation is rendered unchanged.
-AWidget (subtype 13) can be selected for move and resize, but nothing happens when performing those actions.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">TODO:
-Consider z-Order for overlapping annotations? (currently, selection rectangle is always drawn on top, while the related annotation may be in background).
-Provide a PDF document with suitable content, where supported annotation types and and possible regressions can be tested.
-Add test cases once requirements are fixed.</p></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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Resize and move work
-for types AText, AStamp and AGeom
-on all pages of document
-when viewport position changes
-when zoom level changes
-for all page rotations (0°, 90°, 180°, 270°)</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Selection is canceled
-when currently selected annotation is deleted
-on mouse click outside of currently selected annotation
-ESC is pressed</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Viewport is shifted when mouse cursor during move/resize comes close to viewport border.
Resize to negative is prevented.
Tiny annotations are still selectable.
If mouse is moved over an annotation type that we can focus, and the annotation is not yet focused, mouse cursor shape changes to arrow.
If mouse cursor rests over an annotation A, while annotation B is focused, a tooltip for annotation A is shown.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Test for regressions:
-Annotation interaction (focus, move, resize, start playback, ...) are only done in mode EnumMouseMode::Browse.
-If mouse is moved over an annotation type where we can start an action, mouse cursor shape changes to pointing hand.
-If mouse is moved over an annotation type that we can't interact with, mouse cursor shape stays a open hand.
-If mouse cursor rests over an annotation of any type, a tooltip for that annotation is shown.
-Grab/move scroll area (on left click + mouse move) is prevented, if mouse is over focused annotation, or over AMovie/AScreen/AFileAttachment annotation.
-A double click on a annotation starts the "annotator".</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">(78a2e855c22ebb31a9195c679cf0adf981d5443f)</span></li>
<li>core/annotations.h <span style="color: grey">(5653097fe892ce57c8e81615a1c20217e538e1de)</span></li>
<li>core/annotations.cpp <span style="color: grey">(c95fe877316398a5341e29146379dd231dfa40c7)</span></li>
<li>core/annotations_p.h <span style="color: grey">(07b124a4fae40b7a983aa382ae824125e6d25746)</span></li>
<li>core/document.h <span style="color: grey">(bac38f89f85980d478e6252ecc8dc823cbe4359a)</span></li>
<li>core/document.cpp <span style="color: grey">(468a8a9fbadaef5ca8540cf113d5397b989f2aa5)</span></li>
<li>core/document_p.h <span style="color: grey">(8b20c5586f641f6f9ec3a6a6f0d978848a2cb7c8)</span></li>
<li>core/documentcommands.cpp <span style="color: grey">(aafc45a1a989a7240520f2168e253d51d6744f7c)</span></li>
<li>core/documentcommands_p.h <span style="color: grey">(616999dd68406590b304cf648878fa8acb3ec6e0)</span></li>
<li>generators/poppler/annots.cpp <span style="color: grey">(df67986adc076f722e601c3bea187200ecf9df31)</span></li>
<li>ui/pagepainter.cpp <span style="color: grey">(c5eb1ef90e4af48c644a7890a60fa3892cdcf08a)</span></li>
<li>ui/pageview.cpp <span style="color: grey">(8992539c7c282e865a3e5d20e119c8790d79e925)</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>
</td>
</tr>
</table>
</div>
</body>
</html>