> On April 7, 2016, 9:52 p.m., Jonathan Schultz wrote: > > Sorry I've been meaning to test this a bit more and only got to it today. > > > > I have a couple of minor comments: > > > > 1. 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? > > > > 2. 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. > > > > 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? > > > > > Sounds interesting... who would eventually care about merging? > > > > 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. > > Tobias Deiminger wrote: > Thanks a lot for testing! > > > left-clicking causes the annotation to become unselected > > 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. > > > 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? > > 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. > > > (ie the vertical cross) is currently maintained even when the mouse > cursor is no longer over the annotation > > 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. > > > I am looking forward to going over your patch to learn how to do a few > things. > > I'm also at learning, don't trust too much in my changes ;-) > > > https://git.reviewboard.kde.org/r/127496/ but haven't had any response > yet. > > Can't promise when, but will try to test yours too, thanks again! > > Jonathan Schultz wrote: > Great to see progress on this issue! > > 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. > > What are the advantages? > > 1. 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. > > 2. More intuitive. Select-adjust-create annotation feels much more > straightfoward to me than select-create annotation-adjust annotation. > > 3. 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. > > Disadvantages? > > 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. > > Jonathan Schultz wrote: > Bah I realise that those advantages don't make sense. They relate to my > fantasy of being able to turn a selection directly into an annotation. But > even without that I still like the idea that it should be possible to tweak > the size and location of an annotation at the moment of creation.
> my fantasy of being able to turn a selection directly into an annotation You mean without the intermediate step of entering text and settings to a popup window? I guess this could be nice user experience, but would imply that you can edit the annotations in WYSIWIG fashion, is it? When looking at the code, such a feature seems to be a not so easy task, at least out of scope of this review request... - Tobias ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/127366/#review94355 ----------------------------------------------------------- On Jan. 26, 2017, 9:47 p.m., Tobias Deiminger wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/127366/ > ----------------------------------------------------------- > > (Updated Jan. 26, 2017, 9:47 p.m.) > > > Review request for Okular. > > > Bugs: 177778 > http://bugs.kde.org/show_bug.cgi?id=177778 > > > Repository: okular > > > Description > ------- > > This diff adds an annotation resize feature to okular (see Bug 177778). > > 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. > > 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. > > 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. > > 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. > > > Diffs > ----- > > CMakeLists.txt 78a2e855c22ebb31a9195c679cf0adf981d5443f > core/annotations.h 5653097fe892ce57c8e81615a1c20217e538e1de > core/annotations.cpp c95fe877316398a5341e29146379dd231dfa40c7 > core/annotations_p.h 07b124a4fae40b7a983aa382ae824125e6d25746 > core/document.h bac38f89f85980d478e6252ecc8dc823cbe4359a > core/document.cpp 468a8a9fbadaef5ca8540cf113d5397b989f2aa5 > core/document_p.h 8b20c5586f641f6f9ec3a6a6f0d978848a2cb7c8 > core/documentcommands.cpp aafc45a1a989a7240520f2168e253d51d6744f7c > core/documentcommands_p.h 616999dd68406590b304cf648878fa8acb3ec6e0 > generators/poppler/annots.cpp df67986adc076f722e601c3bea187200ecf9df31 > ui/pagepainter.cpp c5eb1ef90e4af48c644a7890a60fa3892cdcf08a > ui/pageview.cpp 8992539c7c282e865a3e5d20e119c8790d79e925 > ui/pageviewmouseannotation.h PRE-CREATION > ui/pageviewmouseannotation.cpp PRE-CREATION > > Diff: https://git.reviewboard.kde.org/r/127366/diff/ > > > Testing > ------- > > 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°) > > Selection is canceled > -when currently selected annotation is deleted > -on mouse click outside of currently selected annotation > -ESC is pressed > > 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. > > 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". > > > Thanks, > > Tobias Deiminger > >
