Re: Review Request 127366: Resize annotations

[Tobias Deiminger]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127366/
---

(Updated March 19, 2017, 10:18 p.m.)


Status
--

This change has been marked as submitted.


Review request for Okular.


Changes
---

Submitted with commit 0957abc39ad0597140b068b6f083371df84acf16 by Albert Astals 
Cid on behalf of Tobias Deiminger to branch master.


Bugs: 18, 314843, 358060, and 375648
http://bugs.kde.org/show_bug.cgi?id=18
http://bugs.kde.org/show_bug.cgi?id=314843
http://bugs.kde.org/show_bug.cgi?id=358060
http://bugs.kde.org/show_bug.cgi?id=375648


Repository: okular


Description
---

Add annotation resize functionality (Bug 18, Bug 314843, Bug 358060, Bug 
375648).

Usage:
If you left-click an annotation, it gets selected. Resize handles appear on the 
selection rectangle. When cursor is moved over one of the 8 resize handles on 
the corners/edges, the cursor shape changes to indicate resize mode. Everywhere 
else on the annotation means "move", just as it was before resize feature was 
added. Pressing ESC or clicking an area outside the annotation cancels a 
selection. Pressing Del deletes a selected annotation.

Feature is only applicable for annotation types AText, AStamp and AGeom.

Implementation:
It works by eventually changing AnnotationPrivate::m_boundary and notifying 
generator (i.e. poppler) about that change. Annotation state handling is 
shifted out of PageView into a new class MouseAnnotation 
(ui/pageviewmouseannotation.cpp). Some functionality not related to resizing 
but to annotation interaction in general is also shifted to class 
MouseAnnotation, to build a single place of responsiblity.

Other changes:
Add method Document::adjustPageAnnotation, backed by a QUndoCommand.
class Okular::AdjustAnnotationCommand.
Add Annotation::adjust and Annotation::canBeResized methods.
Draw resize handles in PagePainter::paintCroppedPageOnPainter.


Diffs
-

  CMakeLists.txt 2f75a9f182cb8ded90bdf536893cbb55bee4d7aa 
  core/annotations.h d657602fed83b4850a29d0ccec5c6bca5d7a4365 
  core/annotations.cpp c95fe877316398a5341e29146379dd231dfa40c7 
  core/annotations_p.h 07b124a4fae40b7a983aa382ae824125e6d25746 
  core/document.h 1fd86262f89651b7c09d87c16400aebb79fcfe55 
  core/document.cpp f8f825c6a0037656c9ffc046f20e897ce32dbbf5 
  core/document_p.h 851194d301dd9c085f7d9b7d2be3cf9a0941d723 
  core/documentcommands.cpp aafc45a1a989a7240520f2168e253d51d6744f7c 
  core/documentcommands_p.h b8a6f239ce11374f973baca0c0c31bfd5886bc5e 
  generators/poppler/annots.cpp df67986adc076f722e601c3bea187200ecf9df31 
  ui/pagepainter.cpp 9d8b1da9436b75e7019994a707fa77560d53dbee 
  ui/pageview.cpp e30eb336bd40f5d70075d8333421c58d55e0b8b3 
  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.
Selected Annotation is deleted when Del is pressed.

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

Re: Review Request 127366: Resize annotations

[Tobias Deiminger]

> On März 11, 2017, 4:57 nachm., Albert Astals Cid wrote:
> > Feature Freeze date for 17.04 is March 23, i suggest you try to 
> > workaround/disable the known bugs/todo since this sems a nice feature to 
> > get in. It can always be improved after it lands, but what lands needs to 
> > be "good" and "consistent"

Ok, all obvious bugs and misbehaviors should be fixed now. If more things need 
to be done, I will try to get them finished until March 23.

Testing was done with documents gathered from other projects, and some 
handcrafted Latex stuff. Coverage is not perfect due to lack of professional 
PDF tools or reference documnets on my side.

I've got a minor concern about the UI concept: If some day we should be able to 
move/resize file-, movie-, sound annotations, then there will be a conflict on 
what happens when left-clicking on it. It would mean "save"/"play" and "select" 
at the same time. Currently there's no problem because move/resize is disabled 
for the mentioned annotation types.

And finally, if there's a brave guy who grasps pageview.cpp in its entire 
beautifulness it would be good if she/he could have a careful look at it ;-)


- Tobias


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127366/#review102789
---


On März 13, 2017, 11:42 nachm., Tobias Deiminger wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127366/
> ---
> 
> (Updated März 13, 2017, 11:42 nachm.)
> 
> 
> Review request for Okular.
> 
> 
> Bugs: 18, 314843, 358060, and 375648
> http://bugs.kde.org/show_bug.cgi?id=18
> http://bugs.kde.org/show_bug.cgi?id=314843
> http://bugs.kde.org/show_bug.cgi?id=358060
> http://bugs.kde.org/show_bug.cgi?id=375648
> 
> 
> Repository: okular
> 
> 
> Description
> ---
> 
> Add annotation resize functionality (Bug 18, Bug 314843, Bug 358060, Bug 
> 375648).
> 
> Usage:
> If you left-click an annotation, it gets selected. Resize handles appear on 
> the selection rectangle. When cursor is moved over one of the 8 resize 
> handles on the corners/edges, the cursor shape changes to indicate resize 
> mode. Everywhere else on the annotation means "move", just as it was before 
> resize feature was added. Pressing ESC or clicking an area outside the 
> annotation cancels a selection. Pressing Del deletes a selected annotation.
> 
> Feature is only applicable for annotation types AText, AStamp and AGeom.
> 
> Implementation:
> It works by eventually changing AnnotationPrivate::m_boundary and notifying 
> generator (i.e. poppler) about that change. Annotation state handling is 
> shifted out of PageView into a new class MouseAnnotation 
> (ui/pageviewmouseannotation.cpp). Some functionality not related to resizing 
> but to annotation interaction in general is also shifted to class 
> MouseAnnotation, to build a single place of responsiblity.
> 
> Other changes:
> Add method Document::adjustPageAnnotation, backed by a QUndoCommand.
> class Okular::AdjustAnnotationCommand.
> Add Annotation::adjust and Annotation::canBeResized methods.
> Draw resize handles in PagePainter::paintCroppedPageOnPainter.
> 
> 
> Diffs
> -
> 
>   CMakeLists.txt 2f75a9f182cb8ded90bdf536893cbb55bee4d7aa 
>   core/annotations.h d657602fed83b4850a29d0ccec5c6bca5d7a4365 
>   core/annotations.cpp c95fe877316398a5341e29146379dd231dfa40c7 
>   core/annotations_p.h 07b124a4fae40b7a983aa382ae824125e6d25746 
>   core/document.h 1fd86262f89651b7c09d87c16400aebb79fcfe55 
>   core/document.cpp f8f825c6a0037656c9ffc046f20e897ce32dbbf5 
>   core/document_p.h 851194d301dd9c085f7d9b7d2be3cf9a0941d723 
>   core/documentcommands.cpp aafc45a1a989a7240520f2168e253d51d6744f7c 
>   core/documentcommands_p.h b8a6f239ce11374f973baca0c0c31bfd5886bc5e 
>   generators/poppler/annots.cpp df67986adc076f722e601c3bea187200ecf9df31 
>   ui/pagepainter.cpp 9d8b1da9436b75e7019994a707fa77560d53dbee 
>   ui/pageview.cpp e30eb336bd40f5d70075d8333421c58d55e0b8b3 
>   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 

Re: Review Request 127366: Resize annotations

[Tobias Deiminger]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127366/
---

(Updated März 13, 2017, 11:42 nachm.)


Review request for Okular.


Changes
---

Fix known bugs
-activate selection only if move/resize is applicable
-don't let cursor indicate resize when it is over selection corner but resize 
is not applicable
-route mouse release while autoscrolling


Bugs: 18, 314843, 358060, and 375648
http://bugs.kde.org/show_bug.cgi?id=18
http://bugs.kde.org/show_bug.cgi?id=314843
http://bugs.kde.org/show_bug.cgi?id=358060
http://bugs.kde.org/show_bug.cgi?id=375648


Repository: okular


Description (updated)
---

Add annotation resize functionality (Bug 18, Bug 314843, Bug 358060, Bug 
375648).

Usage:
If you left-click an annotation, it gets selected. Resize handles appear on the 
selection rectangle. When cursor is moved over one of the 8 resize handles on 
the corners/edges, the cursor shape changes to indicate resize mode. Everywhere 
else on the annotation means "move", just as it was before resize feature was 
added. Pressing ESC or clicking an area outside the annotation cancels a 
selection. Pressing Del deletes a selected annotation.

Feature is only applicable for annotation types AText, AStamp and AGeom.

Implementation:
It works by eventually changing AnnotationPrivate::m_boundary and notifying 
generator (i.e. poppler) about that change. Annotation state handling is 
shifted out of PageView into a new class MouseAnnotation 
(ui/pageviewmouseannotation.cpp). Some functionality not related to resizing 
but to annotation interaction in general is also shifted to class 
MouseAnnotation, to build a single place of responsiblity.

Other changes:
Add method Document::adjustPageAnnotation, backed by a QUndoCommand.
class Okular::AdjustAnnotationCommand.
Add Annotation::adjust and Annotation::canBeResized methods.
Draw resize handles in PagePainter::paintCroppedPageOnPainter.


Diffs (updated)
-

  CMakeLists.txt 2f75a9f182cb8ded90bdf536893cbb55bee4d7aa 
  core/annotations.h d657602fed83b4850a29d0ccec5c6bca5d7a4365 
  core/annotations.cpp c95fe877316398a5341e29146379dd231dfa40c7 
  core/annotations_p.h 07b124a4fae40b7a983aa382ae824125e6d25746 
  core/document.h 1fd86262f89651b7c09d87c16400aebb79fcfe55 
  core/document.cpp f8f825c6a0037656c9ffc046f20e897ce32dbbf5 
  core/document_p.h 851194d301dd9c085f7d9b7d2be3cf9a0941d723 
  core/documentcommands.cpp aafc45a1a989a7240520f2168e253d51d6744f7c 
  core/documentcommands_p.h b8a6f239ce11374f973baca0c0c31bfd5886bc5e 
  generators/poppler/annots.cpp df67986adc076f722e601c3bea187200ecf9df31 
  ui/pagepainter.cpp 9d8b1da9436b75e7019994a707fa77560d53dbee 
  ui/pageview.cpp e30eb336bd40f5d70075d8333421c58d55e0b8b3 
  ui/pageviewmouseannotation.h PRE-CREATION 
  ui/pageviewmouseannotation.cpp PRE-CREATION 

Diff: https://git.reviewboard.kde.org/r/127366/diff/


Testing (updated)
---

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.
Selected Annotation is deleted when Del is pressed.

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

Re: Review Request 127366: Resize annotations

[Albert Astals Cid]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127366/#review102789
---



Feature Freeze date for 17.04 is March 23, i suggest you try to 
workaround/disable the known bugs/todo since this sems a nice feature to get 
in. It can always be improved after it lands, but what lands needs to be "good" 
and "consistent"

- Albert Astals Cid


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: 18
> http://bugs.kde.org/show_bug.cgi?id=18
> 
> 
> Repository: okular
> 
> 
> Description
> ---
> 
> This diff adds an annotation resize feature to okular (see Bug 18).
> 
> 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 

Re: Review Request 127366: Resize annotations

[Albert Astals Cid]

> On March 14, 2016, 12:46 a.m., Albert Astals Cid wrote:
> > Looks very nice for a start, congratulations.
> > 
> > > Reconsider UI, esp. the Ctrl-way to get focus. Is there something in the 
> > > KDE HIG about it?
> > 
> > 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)
> > 
> > 
> > > Improve handling when annotation gets so small that resize handles 
> > > overlap.
> > 
> > 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?
> > 
> > 
> > > Find approach how to handle "resize to negative geometry".
> > 
> > I think not letting people go negative is what makes more sense
> > 
> > 
> > > Known Bug: In the thumbnail list, resize handles are drawn too big, and 
> > > not refreshed correctly.
> > 
> > I'd say it makes more sense to not draw the handles at all
> 
> Tobias Deiminger wrote:
> 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
> 
> Changed:
> -resize handles are filled slightly transparent
> 
> 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.
> 
> 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.
> 
> 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.
> 
> Btw., sorry, I don't know how inline quote here (using the reviewboard 
> webinterface).
> 
> Jonathan Schultz wrote:
> 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.
> 
> 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.
> 
> Albert Astals Cid wrote:
> > 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.
> 
> It's for the "mobile" version of the app, it doesn't have the complex 
> interaction modes (i think) so ignore for now.
> 
> > Btw., sorry, I don't know how inline quote here (using the reviewboard 
> webinterface).
> 
> 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
> 
> Tobias Deiminger wrote:
> > annotations should be select-able, by left-clicking on some obvious 
> part of them, and once selected can then be moved/resized
> 
> Calligra and Karbon do it like Jonathan says. I liked the idea too, so 
> gave it a try and find the handling better now. What do you think?
> Could there be problems because of conflicting user intents (e.g., move 
> the whole document vs. move the annotation)?
> I can quite easily revert or change it again, as design has also improved 
> now.
> 
> > it overlaps with stuff I am working on to do more with selections
> 
> Sounds interesting... who would eventually care about merging?
> 
> Tobias Deiminger wrote:
> Albert, can you already vote for or against following changes? (concerns 
> rev 5 of the diff)
> 
> 1) The patch changes the UI for annotations: Away from "hold ctrl-key 
> while operating on", towards "select by left click, then operate on". See 
> "Usage" in descirption of this RR, or just apply the patch and try it. It's 
> similiar to other KDE applications (calligra flow, karbon, words, ...), and I 
> find it more self-explaining. On the other hand, it breaks behavior okular 
> users might be used to. If in doubt, rev 5 should be suitable for giving a 
> working prototype to kde-usability list and ask them.
> 
> 2) The patch considerably modifies class PageView. The idea is to have 
> all "operate on annotation" functionality in class MouseAnnotation, instead 
> of interleaving it in the longish PageView. PageView then only delegates 
> events to MouseAnnotation. This concerns also non-resize-related features. 
> E.g. I moved annotation tooltips and video playback there. It was almost 
> 

Re: Review Request 127366: Resize annotations

[Tobias Deiminger]

> On March 14, 2016, 12:46 a.m., Albert Astals Cid wrote:
> > Looks very nice for a start, congratulations.
> > 
> > > Reconsider UI, esp. the Ctrl-way to get focus. Is there something in the 
> > > KDE HIG about it?
> > 
> > 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)
> > 
> > 
> > > Improve handling when annotation gets so small that resize handles 
> > > overlap.
> > 
> > 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?
> > 
> > 
> > > Find approach how to handle "resize to negative geometry".
> > 
> > I think not letting people go negative is what makes more sense
> > 
> > 
> > > Known Bug: In the thumbnail list, resize handles are drawn too big, and 
> > > not refreshed correctly.
> > 
> > I'd say it makes more sense to not draw the handles at all
> 
> Tobias Deiminger wrote:
> 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
> 
> Changed:
> -resize handles are filled slightly transparent
> 
> 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.
> 
> 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.
> 
> 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.
> 
> Btw., sorry, I don't know how inline quote here (using the reviewboard 
> webinterface).
> 
> Jonathan Schultz wrote:
> 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.
> 
> 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.
> 
> Albert Astals Cid wrote:
> > 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.
> 
> It's for the "mobile" version of the app, it doesn't have the complex 
> interaction modes (i think) so ignore for now.
> 
> > Btw., sorry, I don't know how inline quote here (using the reviewboard 
> webinterface).
> 
> 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
> 
> Tobias Deiminger wrote:
> > annotations should be select-able, by left-clicking on some obvious 
> part of them, and once selected can then be moved/resized
> 
> Calligra and Karbon do it like Jonathan says. I liked the idea too, so 
> gave it a try and find the handling better now. What do you think?
> Could there be problems because of conflicting user intents (e.g., move 
> the whole document vs. move the annotation)?
> I can quite easily revert or change it again, as design has also improved 
> now.
> 
> > it overlaps with stuff I am working on to do more with selections
> 
> Sounds interesting... who would eventually care about merging?
> 
> Tobias Deiminger wrote:
> Albert, can you already vote for or against following changes? (concerns 
> rev 5 of the diff)
> 
> 1) The patch changes the UI for annotations: Away from "hold ctrl-key 
> while operating on", towards "select by left click, then operate on". See 
> "Usage" in descirption of this RR, or just apply the patch and try it. It's 
> similiar to other KDE applications (calligra flow, karbon, words, ...), and I 
> find it more self-explaining. On the other hand, it breaks behavior okular 
> users might be used to. If in doubt, rev 5 should be suitable for giving a 
> working prototype to kde-usability list and ask them.
> 
> 2) The patch considerably modifies class PageView. The idea is to have 
> all "operate on annotation" functionality in class MouseAnnotation, instead 
> of interleaving it in the longish PageView. PageView then only delegates 
> events to MouseAnnotation. This concerns also non-resize-related features. 
> E.g. I moved annotation tooltips and video playback there. It was almost 
> 

Re: Review Request 127366: Resize annotations

[Tobias Deiminger]

> 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 

Re: Review Request 127366: Resize annotations

[Jonathan Schultz]

> On April 8, 2016, 7:52 a.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.

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.


- Jonathan


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127366/#review94355

Re: Review Request 127366: Resize annotations

[Jonathan Schultz]

> On April 8, 2016, 7:52 a.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!

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


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127366/#review94355
---


On Jan. 27, 2017, 8:47 a.m., Tobias Deiminger wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127366/
> ---
> 
> (Updated Jan. 27, 2017, 8:47 a.m.)
> 
> 
> Review request for Okular.
> 
> 
> Bugs: 18
> 

Re: Review Request 127366: Resize annotations

[Albert Astals Cid]

> On March 14, 2016, 12:46 a.m., Albert Astals Cid wrote:
> > Looks very nice for a start, congratulations.
> > 
> > > Reconsider UI, esp. the Ctrl-way to get focus. Is there something in the 
> > > KDE HIG about it?
> > 
> > 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)
> > 
> > 
> > > Improve handling when annotation gets so small that resize handles 
> > > overlap.
> > 
> > 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?
> > 
> > 
> > > Find approach how to handle "resize to negative geometry".
> > 
> > I think not letting people go negative is what makes more sense
> > 
> > 
> > > Known Bug: In the thumbnail list, resize handles are drawn too big, and 
> > > not refreshed correctly.
> > 
> > I'd say it makes more sense to not draw the handles at all
> 
> Tobias Deiminger wrote:
> 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
> 
> Changed:
> -resize handles are filled slightly transparent
> 
> 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.
> 
> 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.
> 
> 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.
> 
> Btw., sorry, I don't know how inline quote here (using the reviewboard 
> webinterface).
> 
> Jonathan Schultz wrote:
> 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.
> 
> 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.
> 
> Albert Astals Cid wrote:
> > 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.
> 
> It's for the "mobile" version of the app, it doesn't have the complex 
> interaction modes (i think) so ignore for now.
> 
> > Btw., sorry, I don't know how inline quote here (using the reviewboard 
> webinterface).
> 
> 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
> 
> Tobias Deiminger wrote:
> > annotations should be select-able, by left-clicking on some obvious 
> part of them, and once selected can then be moved/resized
> 
> Calligra and Karbon do it like Jonathan says. I liked the idea too, so 
> gave it a try and find the handling better now. What do you think?
> Could there be problems because of conflicting user intents (e.g., move 
> the whole document vs. move the annotation)?
> I can quite easily revert or change it again, as design has also improved 
> now.
> 
> > it overlaps with stuff I am working on to do more with selections
> 
> Sounds interesting... who would eventually care about merging?
> 
> Tobias Deiminger wrote:
> Albert, can you already vote for or against following changes? (concerns 
> rev 5 of the diff)
> 
> 1) The patch changes the UI for annotations: Away from "hold ctrl-key 
> while operating on", towards "select by left click, then operate on". See 
> "Usage" in descirption of this RR, or just apply the patch and try it. It's 
> similiar to other KDE applications (calligra flow, karbon, words, ...), and I 
> find it more self-explaining. On the other hand, it breaks behavior okular 
> users might be used to. If in doubt, rev 5 should be suitable for giving a 
> working prototype to kde-usability list and ask them.
> 
> 2) The patch considerably modifies class PageView. The idea is to have 
> all "operate on annotation" functionality in class MouseAnnotation, instead 
> of interleaving it in the longish PageView. PageView then only delegates 
> events to MouseAnnotation. This concerns also non-resize-related features. 
> E.g. I moved annotation tooltips and video playback there. It was almost 
> 

Re: Review Request 127366: Resize annotations

[Tobias Deiminger]

> On March 14, 2016, 12:46 a.m., Albert Astals Cid wrote:
> > Looks very nice for a start, congratulations.
> > 
> > > Reconsider UI, esp. the Ctrl-way to get focus. Is there something in the 
> > > KDE HIG about it?
> > 
> > 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)
> > 
> > 
> > > Improve handling when annotation gets so small that resize handles 
> > > overlap.
> > 
> > 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?
> > 
> > 
> > > Find approach how to handle "resize to negative geometry".
> > 
> > I think not letting people go negative is what makes more sense
> > 
> > 
> > > Known Bug: In the thumbnail list, resize handles are drawn too big, and 
> > > not refreshed correctly.
> > 
> > I'd say it makes more sense to not draw the handles at all
> 
> Tobias Deiminger wrote:
> 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
> 
> Changed:
> -resize handles are filled slightly transparent
> 
> 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.
> 
> 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.
> 
> 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.
> 
> Btw., sorry, I don't know how inline quote here (using the reviewboard 
> webinterface).
> 
> Jonathan Schultz wrote:
> 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.
> 
> 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.
> 
> Albert Astals Cid wrote:
> > 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.
> 
> It's for the "mobile" version of the app, it doesn't have the complex 
> interaction modes (i think) so ignore for now.
> 
> > Btw., sorry, I don't know how inline quote here (using the reviewboard 
> webinterface).
> 
> 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
> 
> Tobias Deiminger wrote:
> > annotations should be select-able, by left-clicking on some obvious 
> part of them, and once selected can then be moved/resized
> 
> Calligra and Karbon do it like Jonathan says. I liked the idea too, so 
> gave it a try and find the handling better now. What do you think?
> Could there be problems because of conflicting user intents (e.g., move 
> the whole document vs. move the annotation)?
> I can quite easily revert or change it again, as design has also improved 
> now.
> 
> > it overlaps with stuff I am working on to do more with selections
> 
> Sounds interesting... who would eventually care about merging?
> 
> Tobias Deiminger wrote:
> Albert, can you already vote for or against following changes? (concerns 
> rev 5 of the diff)
> 
> 1) The patch changes the UI for annotations: Away from "hold ctrl-key 
> while operating on", towards "select by left click, then operate on". See 
> "Usage" in descirption of this RR, or just apply the patch and try it. It's 
> similiar to other KDE applications (calligra flow, karbon, words, ...), and I 
> find it more self-explaining. On the other hand, it breaks behavior okular 
> users might be used to. If in doubt, rev 5 should be suitable for giving a 
> working prototype to kde-usability list and ask them.
> 
> 2) The patch considerably modifies class PageView. The idea is to have 
> all "operate on annotation" functionality in class MouseAnnotation, instead 
> of interleaving it in the longish PageView. PageView then only delegates 
> events to MouseAnnotation. This concerns also non-resize-related features. 
> E.g. I moved annotation tooltips and video playback there. It was almost 
> 

Re: Review Request 127366: Resize annotations

[Tobias Deiminger]

---
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.


Changes
---

Add @since to enum and don't rename BeingMoved. Make adjust() variables const. 
Don't use signal but routeKeyPressEvent(). Implement delete annotation by 
Delete key.


Bugs: 18
http://bugs.kde.org/show_bug.cgi?id=18


Repository: okular


Description
---

This diff adds an annotation resize feature to okular (see Bug 18).

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 (updated)
-

  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

Re: Review Request 127366: Resize annotations

[Albert Astals Cid]

> On March 14, 2016, 12:46 a.m., Albert Astals Cid wrote:
> > Looks very nice for a start, congratulations.
> > 
> > > Reconsider UI, esp. the Ctrl-way to get focus. Is there something in the 
> > > KDE HIG about it?
> > 
> > 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)
> > 
> > 
> > > Improve handling when annotation gets so small that resize handles 
> > > overlap.
> > 
> > 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?
> > 
> > 
> > > Find approach how to handle "resize to negative geometry".
> > 
> > I think not letting people go negative is what makes more sense
> > 
> > 
> > > Known Bug: In the thumbnail list, resize handles are drawn too big, and 
> > > not refreshed correctly.
> > 
> > I'd say it makes more sense to not draw the handles at all
> 
> Tobias Deiminger wrote:
> 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
> 
> Changed:
> -resize handles are filled slightly transparent
> 
> 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.
> 
> 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.
> 
> 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.
> 
> Btw., sorry, I don't know how inline quote here (using the reviewboard 
> webinterface).
> 
> Jonathan Schultz wrote:
> 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.
> 
> 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.
> 
> Albert Astals Cid wrote:
> > 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.
> 
> It's for the "mobile" version of the app, it doesn't have the complex 
> interaction modes (i think) so ignore for now.
> 
> > Btw., sorry, I don't know how inline quote here (using the reviewboard 
> webinterface).
> 
> 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
> 
> Tobias Deiminger wrote:
> > annotations should be select-able, by left-clicking on some obvious 
> part of them, and once selected can then be moved/resized
> 
> Calligra and Karbon do it like Jonathan says. I liked the idea too, so 
> gave it a try and find the handling better now. What do you think?
> Could there be problems because of conflicting user intents (e.g., move 
> the whole document vs. move the annotation)?
> I can quite easily revert or change it again, as design has also improved 
> now.
> 
> > it overlaps with stuff I am working on to do more with selections
> 
> Sounds interesting... who would eventually care about merging?
> 
> Tobias Deiminger wrote:
> Albert, can you already vote for or against following changes? (concerns 
> rev 5 of the diff)
> 
> 1) The patch changes the UI for annotations: Away from "hold ctrl-key 
> while operating on", towards "select by left click, then operate on". See 
> "Usage" in descirption of this RR, or just apply the patch and try it. It's 
> similiar to other KDE applications (calligra flow, karbon, words, ...), and I 
> find it more self-explaining. On the other hand, it breaks behavior okular 
> users might be used to. If in doubt, rev 5 should be suitable for giving a 
> working prototype to kde-usability list and ask them.
> 
> 2) The patch considerably modifies class PageView. The idea is to have 
> all "operate on annotation" functionality in class MouseAnnotation, instead 
> of interleaving it in the longish PageView. PageView then only delegates 
> events to MouseAnnotation. This concerns also non-resize-related features. 
> E.g. I moved annotation tooltips and video playback there. It was almost 
> 

Re: Review Request 127366: Resize annotations

[Tobias Deiminger]

> On März 14, 2016, 12:46 vorm., Albert Astals Cid wrote:
> > Looks very nice for a start, congratulations.
> > 
> > > Reconsider UI, esp. the Ctrl-way to get focus. Is there something in the 
> > > KDE HIG about it?
> > 
> > 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)
> > 
> > 
> > > Improve handling when annotation gets so small that resize handles 
> > > overlap.
> > 
> > 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?
> > 
> > 
> > > Find approach how to handle "resize to negative geometry".
> > 
> > I think not letting people go negative is what makes more sense
> > 
> > 
> > > Known Bug: In the thumbnail list, resize handles are drawn too big, and 
> > > not refreshed correctly.
> > 
> > I'd say it makes more sense to not draw the handles at all
> 
> Tobias Deiminger wrote:
> 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
> 
> Changed:
> -resize handles are filled slightly transparent
> 
> 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.
> 
> 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.
> 
> 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.
> 
> Btw., sorry, I don't know how inline quote here (using the reviewboard 
> webinterface).
> 
> Jonathan Schultz wrote:
> 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.
> 
> 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.
> 
> Albert Astals Cid wrote:
> > 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.
> 
> It's for the "mobile" version of the app, it doesn't have the complex 
> interaction modes (i think) so ignore for now.
> 
> > Btw., sorry, I don't know how inline quote here (using the reviewboard 
> webinterface).
> 
> 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
> 
> Tobias Deiminger wrote:
> > annotations should be select-able, by left-clicking on some obvious 
> part of them, and once selected can then be moved/resized
> 
> Calligra and Karbon do it like Jonathan says. I liked the idea too, so 
> gave it a try and find the handling better now. What do you think?
> Could there be problems because of conflicting user intents (e.g., move 
> the whole document vs. move the annotation)?
> I can quite easily revert or change it again, as design has also improved 
> now.
> 
> > it overlaps with stuff I am working on to do more with selections
> 
> Sounds interesting... who would eventually care about merging?
> 
> Tobias Deiminger wrote:
> Albert, can you already vote for or against following changes? (concerns 
> rev 5 of the diff)
> 
> 1) The patch changes the UI for annotations: Away from "hold ctrl-key 
> while operating on", towards "select by left click, then operate on". See 
> "Usage" in descirption of this RR, or just apply the patch and try it. It's 
> similiar to other KDE applications (calligra flow, karbon, words, ...), and I 
> find it more self-explaining. On the other hand, it breaks behavior okular 
> users might be used to. If in doubt, rev 5 should be suitable for giving a 
> working prototype to kde-usability list and ask them.
> 
> 2) The patch considerably modifies class PageView. The idea is to have 
> all "operate on annotation" functionality in class MouseAnnotation, instead 
> of interleaving it in the longish PageView. PageView then only delegates 
> events to MouseAnnotation. This concerns also non-resize-related features. 
> E.g. I moved annotation tooltips and video playback there. It was almost 
> 

Re: Review Request 127366: Resize annotations

[Oliver Sander]

> On March 14, 2016, 12:46 a.m., Albert Astals Cid wrote:
> > Looks very nice for a start, congratulations.
> > 
> > > Reconsider UI, esp. the Ctrl-way to get focus. Is there something in the 
> > > KDE HIG about it?
> > 
> > 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)
> > 
> > 
> > > Improve handling when annotation gets so small that resize handles 
> > > overlap.
> > 
> > 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?
> > 
> > 
> > > Find approach how to handle "resize to negative geometry".
> > 
> > I think not letting people go negative is what makes more sense
> > 
> > 
> > > Known Bug: In the thumbnail list, resize handles are drawn too big, and 
> > > not refreshed correctly.
> > 
> > I'd say it makes more sense to not draw the handles at all
> 
> Tobias Deiminger wrote:
> 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
> 
> Changed:
> -resize handles are filled slightly transparent
> 
> 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.
> 
> 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.
> 
> 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.
> 
> Btw., sorry, I don't know how inline quote here (using the reviewboard 
> webinterface).
> 
> Jonathan Schultz wrote:
> 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.
> 
> 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.
> 
> Albert Astals Cid wrote:
> > 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.
> 
> It's for the "mobile" version of the app, it doesn't have the complex 
> interaction modes (i think) so ignore for now.
> 
> > Btw., sorry, I don't know how inline quote here (using the reviewboard 
> webinterface).
> 
> 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
> 
> Tobias Deiminger wrote:
> > annotations should be select-able, by left-clicking on some obvious 
> part of them, and once selected can then be moved/resized
> 
> Calligra and Karbon do it like Jonathan says. I liked the idea too, so 
> gave it a try and find the handling better now. What do you think?
> Could there be problems because of conflicting user intents (e.g., move 
> the whole document vs. move the annotation)?
> I can quite easily revert or change it again, as design has also improved 
> now.
> 
> > it overlaps with stuff I am working on to do more with selections
> 
> Sounds interesting... who would eventually care about merging?
> 
> Tobias Deiminger wrote:
> Albert, can you already vote for or against following changes? (concerns 
> rev 5 of the diff)
> 
> 1) The patch changes the UI for annotations: Away from "hold ctrl-key 
> while operating on", towards "select by left click, then operate on". See 
> "Usage" in descirption of this RR, or just apply the patch and try it. It's 
> similiar to other KDE applications (calligra flow, karbon, words, ...), and I 
> find it more self-explaining. On the other hand, it breaks behavior okular 
> users might be used to. If in doubt, rev 5 should be suitable for giving a 
> working prototype to kde-usability list and ask them.
> 
> 2) The patch considerably modifies class PageView. The idea is to have 
> all "operate on annotation" functionality in class MouseAnnotation, instead 
> of interleaving it in the longish PageView. PageView then only delegates 
> events to MouseAnnotation. This concerns also non-resize-related features. 
> E.g. I moved annotation tooltips and video playback there. It was almost 
> 

Re: Review Request 127366: Resize annotations

[Oliver Sander]

> On March 14, 2016, 12:46 a.m., Albert Astals Cid wrote:
> > Looks very nice for a start, congratulations.
> > 
> > > Reconsider UI, esp. the Ctrl-way to get focus. Is there something in the 
> > > KDE HIG about it?
> > 
> > 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)
> > 
> > 
> > > Improve handling when annotation gets so small that resize handles 
> > > overlap.
> > 
> > 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?
> > 
> > 
> > > Find approach how to handle "resize to negative geometry".
> > 
> > I think not letting people go negative is what makes more sense
> > 
> > 
> > > Known Bug: In the thumbnail list, resize handles are drawn too big, and 
> > > not refreshed correctly.
> > 
> > I'd say it makes more sense to not draw the handles at all
> 
> Tobias Deiminger wrote:
> 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
> 
> Changed:
> -resize handles are filled slightly transparent
> 
> 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.
> 
> 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.
> 
> 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.
> 
> Btw., sorry, I don't know how inline quote here (using the reviewboard 
> webinterface).
> 
> Jonathan Schultz wrote:
> 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.
> 
> 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.
> 
> Albert Astals Cid wrote:
> > 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.
> 
> It's for the "mobile" version of the app, it doesn't have the complex 
> interaction modes (i think) so ignore for now.
> 
> > Btw., sorry, I don't know how inline quote here (using the reviewboard 
> webinterface).
> 
> 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
> 
> Tobias Deiminger wrote:
> > annotations should be select-able, by left-clicking on some obvious 
> part of them, and once selected can then be moved/resized
> 
> Calligra and Karbon do it like Jonathan says. I liked the idea too, so 
> gave it a try and find the handling better now. What do you think?
> Could there be problems because of conflicting user intents (e.g., move 
> the whole document vs. move the annotation)?
> I can quite easily revert or change it again, as design has also improved 
> now.
> 
> > it overlaps with stuff I am working on to do more with selections
> 
> Sounds interesting... who would eventually care about merging?
> 
> Tobias Deiminger wrote:
> Albert, can you already vote for or against following changes? (concerns 
> rev 5 of the diff)
> 
> 1) The patch changes the UI for annotations: Away from "hold ctrl-key 
> while operating on", towards "select by left click, then operate on". See 
> "Usage" in descirption of this RR, or just apply the patch and try it. It's 
> similiar to other KDE applications (calligra flow, karbon, words, ...), and I 
> find it more self-explaining. On the other hand, it breaks behavior okular 
> users might be used to. If in doubt, rev 5 should be suitable for giving a 
> working prototype to kde-usability list and ask them.
> 
> 2) The patch considerably modifies class PageView. The idea is to have 
> all "operate on annotation" functionality in class MouseAnnotation, instead 
> of interleaving it in the longish PageView. PageView then only delegates 
> events to MouseAnnotation. This concerns also non-resize-related features. 
> E.g. I moved annotation tooltips and video playback there. It was almost 
> 

Re: Review Request 127366: Resize annotations

[Tobias Deiminger]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127366/
---

(Updated Nov. 13, 2016, 9:18 p.m.)


Review request for Okular.


Changes
---

Rebase on the Frameworks-merged master. Additioally, fix selection handling for 
non-rectangular shaped annotations (like ellipse).


Bugs: 18
http://bugs.kde.org/show_bug.cgi?id=18


Repository: okular


Description (updated)
---

This diff adds an annotation resize feature to okular (see Bug 18).

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 (updated)
-

  CMakeLists.txt 9810c9bb552cc3da9cf694fdb61fd22471bad955 
  autotests/translateannotationtest.cpp 
9fda65d50342a8c91df88f9feb4a54413f5819e3 
  core/annotations.h 5653097fe892ce57c8e81615a1c20217e538e1de 
  core/annotations.cpp c95fe877316398a5341e29146379dd231dfa40c7 
  core/annotations_p.h 07b124a4fae40b7a983aa382ae824125e6d25746 
  core/document.h bac38f89f85980d478e6252ecc8dc823cbe4359a 
  core/document.cpp ff9253acb2f533c67eb48c85a1cadbbdce1f5ef6 
  core/document_p.h ef4dda60e9cea272b23b15e2bef84da9d9bfb0ac 
  core/documentcommands.cpp aafc45a1a989a7240520f2168e253d51d6744f7c 
  core/documentcommands_p.h 616999dd68406590b304cf648878fa8acb3ec6e0 
  generators/poppler/annots.cpp df67986adc076f722e601c3bea187200ecf9df31 
  ui/pagepainter.cpp db82119ca399f4b93798198ea92a04bf8e5cfeee 
  ui/pageview.cpp b116b36a0de494f150a1e9fbaadd86a2bad3d6c3 
  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 

Re: Review Request 127366: Resize annotations

[Oliver Sander]

> On March 14, 2016, 12:46 a.m., Albert Astals Cid wrote:
> > Looks very nice for a start, congratulations.
> > 
> > > Reconsider UI, esp. the Ctrl-way to get focus. Is there something in the 
> > > KDE HIG about it?
> > 
> > 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)
> > 
> > 
> > > Improve handling when annotation gets so small that resize handles 
> > > overlap.
> > 
> > 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?
> > 
> > 
> > > Find approach how to handle "resize to negative geometry".
> > 
> > I think not letting people go negative is what makes more sense
> > 
> > 
> > > Known Bug: In the thumbnail list, resize handles are drawn too big, and 
> > > not refreshed correctly.
> > 
> > I'd say it makes more sense to not draw the handles at all
> 
> Tobias Deiminger wrote:
> 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
> 
> Changed:
> -resize handles are filled slightly transparent
> 
> 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.
> 
> 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.
> 
> 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.
> 
> Btw., sorry, I don't know how inline quote here (using the reviewboard 
> webinterface).
> 
> Jonathan Schultz wrote:
> 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.
> 
> 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.
> 
> Albert Astals Cid wrote:
> > 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.
> 
> It's for the "mobile" version of the app, it doesn't have the complex 
> interaction modes (i think) so ignore for now.
> 
> > Btw., sorry, I don't know how inline quote here (using the reviewboard 
> webinterface).
> 
> 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
> 
> Tobias Deiminger wrote:
> > annotations should be select-able, by left-clicking on some obvious 
> part of them, and once selected can then be moved/resized
> 
> Calligra and Karbon do it like Jonathan says. I liked the idea too, so 
> gave it a try and find the handling better now. What do you think?
> Could there be problems because of conflicting user intents (e.g., move 
> the whole document vs. move the annotation)?
> I can quite easily revert or change it again, as design has also improved 
> now.
> 
> > it overlaps with stuff I am working on to do more with selections
> 
> Sounds interesting... who would eventually care about merging?
> 
> Tobias Deiminger wrote:
> Albert, can you already vote for or against following changes? (concerns 
> rev 5 of the diff)
> 
> 1) The patch changes the UI for annotations: Away from "hold ctrl-key 
> while operating on", towards "select by left click, then operate on". See 
> "Usage" in descirption of this RR, or just apply the patch and try it. It's 
> similiar to other KDE applications (calligra flow, karbon, words, ...), and I 
> find it more self-explaining. On the other hand, it breaks behavior okular 
> users might be used to. If in doubt, rev 5 should be suitable for giving a 
> working prototype to kde-usability list and ask them.
> 
> 2) The patch considerably modifies class PageView. The idea is to have 
> all "operate on annotation" functionality in class MouseAnnotation, instead 
> of interleaving it in the longish PageView. PageView then only delegates 
> events to MouseAnnotation. This concerns also non-resize-related features. 
> E.g. I moved annotation tooltips and video playback there. It was almost 
> 

Re: Review Request 127366: Resize annotations

[Jonathan Schultz]

> On March 14, 2016, 12:46 a.m., Albert Astals Cid wrote:
> > Looks very nice for a start, congratulations.
> > 
> > > Reconsider UI, esp. the Ctrl-way to get focus. Is there something in the 
> > > KDE HIG about it?
> > 
> > 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)
> > 
> > 
> > > Improve handling when annotation gets so small that resize handles 
> > > overlap.
> > 
> > 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?
> > 
> > 
> > > Find approach how to handle "resize to negative geometry".
> > 
> > I think not letting people go negative is what makes more sense
> > 
> > 
> > > Known Bug: In the thumbnail list, resize handles are drawn too big, and 
> > > not refreshed correctly.
> > 
> > I'd say it makes more sense to not draw the handles at all
> 
> Tobias Deiminger wrote:
> 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
> 
> Changed:
> -resize handles are filled slightly transparent
> 
> 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.
> 
> 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.
> 
> 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.
> 
> Btw., sorry, I don't know how inline quote here (using the reviewboard 
> webinterface).
> 
> Jonathan Schultz wrote:
> 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.
> 
> 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.
> 
> Albert Astals Cid wrote:
> > 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.
> 
> It's for the "mobile" version of the app, it doesn't have the complex 
> interaction modes (i think) so ignore for now.
> 
> > Btw., sorry, I don't know how inline quote here (using the reviewboard 
> webinterface).
> 
> 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
> 
> Tobias Deiminger wrote:
> > annotations should be select-able, by left-clicking on some obvious 
> part of them, and once selected can then be moved/resized
> 
> Calligra and Karbon do it like Jonathan says. I liked the idea too, so 
> gave it a try and find the handling better now. What do you think?
> Could there be problems because of conflicting user intents (e.g., move 
> the whole document vs. move the annotation)?
> I can quite easily revert or change it again, as design has also improved 
> now.
> 
> > it overlaps with stuff I am working on to do more with selections
> 
> Sounds interesting... who would eventually care about merging?
> 
> Tobias Deiminger wrote:
> Albert, can you already vote for or against following changes? (concerns 
> rev 5 of the diff)
> 
> 1) The patch changes the UI for annotations: Away from "hold ctrl-key 
> while operating on", towards "select by left click, then operate on". See 
> "Usage" in descirption of this RR, or just apply the patch and try it. It's 
> similiar to other KDE applications (calligra flow, karbon, words, ...), and I 
> find it more self-explaining. On the other hand, it breaks behavior okular 
> users might be used to. If in doubt, rev 5 should be suitable for giving a 
> working prototype to kde-usability list and ask them.
> 
> 2) The patch considerably modifies class PageView. The idea is to have 
> all "operate on annotation" functionality in class MouseAnnotation, instead 
> of interleaving it in the longish PageView. PageView then only delegates 
> events to MouseAnnotation. This concerns also non-resize-related features. 
> E.g. I moved annotation tooltips and video playback there. It was almost 
> 

Re: Review Request 127366: Resize annotations

[Tobias Deiminger]

> On March 14, 2016, 12:46 a.m., Albert Astals Cid wrote:
> > Looks very nice for a start, congratulations.
> > 
> > > Reconsider UI, esp. the Ctrl-way to get focus. Is there something in the 
> > > KDE HIG about it?
> > 
> > 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)
> > 
> > 
> > > Improve handling when annotation gets so small that resize handles 
> > > overlap.
> > 
> > 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?
> > 
> > 
> > > Find approach how to handle "resize to negative geometry".
> > 
> > I think not letting people go negative is what makes more sense
> > 
> > 
> > > Known Bug: In the thumbnail list, resize handles are drawn too big, and 
> > > not refreshed correctly.
> > 
> > I'd say it makes more sense to not draw the handles at all
> 
> Tobias Deiminger wrote:
> 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
> 
> Changed:
> -resize handles are filled slightly transparent
> 
> 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.
> 
> 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.
> 
> 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.
> 
> Btw., sorry, I don't know how inline quote here (using the reviewboard 
> webinterface).
> 
> Jonathan Schultz wrote:
> 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.
> 
> 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.
> 
> Albert Astals Cid wrote:
> > 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.
> 
> It's for the "mobile" version of the app, it doesn't have the complex 
> interaction modes (i think) so ignore for now.
> 
> > Btw., sorry, I don't know how inline quote here (using the reviewboard 
> webinterface).
> 
> 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
> 
> Tobias Deiminger wrote:
> > annotations should be select-able, by left-clicking on some obvious 
> part of them, and once selected can then be moved/resized
> 
> Calligra and Karbon do it like Jonathan says. I liked the idea too, so 
> gave it a try and find the handling better now. What do you think?
> Could there be problems because of conflicting user intents (e.g., move 
> the whole document vs. move the annotation)?
> I can quite easily revert or change it again, as design has also improved 
> now.
> 
> > it overlaps with stuff I am working on to do more with selections
> 
> Sounds interesting... who would eventually care about merging?
> 
> Tobias Deiminger wrote:
> Albert, can you already vote for or against following changes? (concerns 
> rev 5 of the diff)
> 
> 1) The patch changes the UI for annotations: Away from "hold ctrl-key 
> while operating on", towards "select by left click, then operate on". See 
> "Usage" in descirption of this RR, or just apply the patch and try it. It's 
> similiar to other KDE applications (calligra flow, karbon, words, ...), and I 
> find it more self-explaining. On the other hand, it breaks behavior okular 
> users might be used to. If in doubt, rev 5 should be suitable for giving a 
> working prototype to kde-usability list and ask them.
> 
> 2) The patch considerably modifies class PageView. The idea is to have 
> all "operate on annotation" functionality in class MouseAnnotation, instead 
> of interleaving it in the longish PageView. PageView then only delegates 
> events to MouseAnnotation. This concerns also non-resize-related features. 
> E.g. I moved annotation tooltips and video playback there. It was almost 
> 

Re: Review Request 127366: Resize annotations

[Oliver Sander]

> On March 14, 2016, 12:46 a.m., Albert Astals Cid wrote:
> > Looks very nice for a start, congratulations.
> > 
> > > Reconsider UI, esp. the Ctrl-way to get focus. Is there something in the 
> > > KDE HIG about it?
> > 
> > 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)
> > 
> > 
> > > Improve handling when annotation gets so small that resize handles 
> > > overlap.
> > 
> > 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?
> > 
> > 
> > > Find approach how to handle "resize to negative geometry".
> > 
> > I think not letting people go negative is what makes more sense
> > 
> > 
> > > Known Bug: In the thumbnail list, resize handles are drawn too big, and 
> > > not refreshed correctly.
> > 
> > I'd say it makes more sense to not draw the handles at all
> 
> Tobias Deiminger wrote:
> 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
> 
> Changed:
> -resize handles are filled slightly transparent
> 
> 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.
> 
> 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.
> 
> 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.
> 
> Btw., sorry, I don't know how inline quote here (using the reviewboard 
> webinterface).
> 
> Jonathan Schultz wrote:
> 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.
> 
> 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.
> 
> Albert Astals Cid wrote:
> > 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.
> 
> It's for the "mobile" version of the app, it doesn't have the complex 
> interaction modes (i think) so ignore for now.
> 
> > Btw., sorry, I don't know how inline quote here (using the reviewboard 
> webinterface).
> 
> 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
> 
> Tobias Deiminger wrote:
> > annotations should be select-able, by left-clicking on some obvious 
> part of them, and once selected can then be moved/resized
> 
> Calligra and Karbon do it like Jonathan says. I liked the idea too, so 
> gave it a try and find the handling better now. What do you think?
> Could there be problems because of conflicting user intents (e.g., move 
> the whole document vs. move the annotation)?
> I can quite easily revert or change it again, as design has also improved 
> now.
> 
> > it overlaps with stuff I am working on to do more with selections
> 
> Sounds interesting... who would eventually care about merging?
> 
> Tobias Deiminger wrote:
> Albert, can you already vote for or against following changes? (concerns 
> rev 5 of the diff)
> 
> 1) The patch changes the UI for annotations: Away from "hold ctrl-key 
> while operating on", towards "select by left click, then operate on". See 
> "Usage" in descirption of this RR, or just apply the patch and try it. It's 
> similiar to other KDE applications (calligra flow, karbon, words, ...), and I 
> find it more self-explaining. On the other hand, it breaks behavior okular 
> users might be used to. If in doubt, rev 5 should be suitable for giving a 
> working prototype to kde-usability list and ask them.
> 
> 2) The patch considerably modifies class PageView. The idea is to have 
> all "operate on annotation" functionality in class MouseAnnotation, instead 
> of interleaving it in the longish PageView. PageView then only delegates 
> events to MouseAnnotation. This concerns also non-resize-related features. 
> E.g. I moved annotation tooltips and video playback there. It was almost 
> 

Re: Review Request 127366: Resize annotations

[Albert Astals Cid]

> On Oct. 16, 2016, 8:16 p.m., Oliver Sander wrote:
> > ui/pagepainter.h, line 36
> > 
> >
> > That comma after '32' is not correct, is it?
> 
> Tobias Deiminger wrote:
> C++11 (7.2/1) and C99 (6.7.2.2) allow, but don't require trailing commas 
> in enum-specifier. C++03 and C89 do not support them. Okular enums are 
> without trailing commas, so I think it's best to remove them.
> There are more occurrences in pageviewmouseannotation.h, I'll remove them 
> accordingly with the next revision.
> 
> Btw., do current Okular coding conventions say anything about C++11 usage 
> in general? Afair there's quite no C++11 anywhere in the code yet.

No c++11 at all in master (while it is qt4 based) once we merge the qt5 
frameworks branch "some" c+11 will be allowed, even there's no guideline for 
what and what not will be allowed.


- Albert


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127366/#review100047
---


On April 28, 2016, 7:13 p.m., Tobias Deiminger wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127366/
> ---
> 
> (Updated April 28, 2016, 7:13 p.m.)
> 
> 
> Review request for Okular.
> 
> 
> Bugs: 18
> http://bugs.kde.org/show_bug.cgi?id=18
> 
> 
> Repository: okular
> 
> 
> Description
> ---
> 
> This diff adds an annotation resize feature to okular (see Bug 18).
> 
> 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:
> -For geometric annotations, only the the shape of the annotation is sensitive 
> for move/resize, but not the selection rectangle.
> 
> 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 97e8db6e4a704fd34331fad7b7628ca2248b62d8 
>   core/annotations.h 4f107440dc824fd9049a30082befd18642e63895 
>   core/annotations.cpp e02994688414bdf485b308d6ef122ee2eff3fbaf 
>   core/annotations_p.h 07b124a4fae40b7a983aa382ae824125e6d25746 
>   core/document.h 63f58741bd6680a673945a7b7c05a10130968beb 
>   core/document.cpp 6953b1fb0dc29a375be7ff331a2a2bccce975366 
>   core/document_p.h fda23275033645ea67f5ad9d27341fc4635ede34 
>   core/documentcommands.cpp 95aded51d73a3d3b98ff26284c9c46fc5c9cf0ca 
>   core/documentcommands_p.h 17394f2a25b187cf4aff66b3a7f891b81be5acdd 
>   generators/poppler/annots.cpp 8cde64833831ec833b3be552608cff99d38f8e63 
>   tests/translateannotationtest.cpp 184b9474e6072a991a5ee5f1116bf7a9ef10cadc 
>   ui/pagepainter.h 68b241658162d9bd6eb187efc594ef17ea99d899 
>   ui/pagepainter.cpp 3bcd8bc4cfe7471bc3c21cfcd3cff50b8a8d49ee 
>   ui/pageview.cpp 3ebf7dcb04aa1942e02f49133d98081e2bbb565c 
>   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 

Re: Review Request 127366: Resize annotations

[Tobias Deiminger]

> On Okt. 16, 2016, 8:16 nachm., Oliver Sander wrote:
> > ui/pagepainter.h, line 36
> > 
> >
> > That comma after '32' is not correct, is it?

C++11 (7.2/1) and C99 (6.7.2.2) allow, but don't require trailing commas in 
enum-specifier. C++03 and C89 do not support them. Okular enums are without 
trailing commas, so I think it's best to remove them.
There are more occurrences in pageviewmouseannotation.h, I'll remove them 
accordingly with the next revision.

Btw., do current Okular coding conventions say anything about C++11 usage in 
general? Afair there's quite no C++11 anywhere in the code yet.


- Tobias


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127366/#review100047
---


On April 28, 2016, 7:13 nachm., Tobias Deiminger wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127366/
> ---
> 
> (Updated April 28, 2016, 7:13 nachm.)
> 
> 
> Review request for Okular.
> 
> 
> Bugs: 18
> http://bugs.kde.org/show_bug.cgi?id=18
> 
> 
> Repository: okular
> 
> 
> Description
> ---
> 
> This diff adds an annotation resize feature to okular (see Bug 18).
> 
> 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:
> -For geometric annotations, only the the shape of the annotation is sensitive 
> for move/resize, but not the selection rectangle.
> 
> 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 97e8db6e4a704fd34331fad7b7628ca2248b62d8 
>   core/annotations.h 4f107440dc824fd9049a30082befd18642e63895 
>   core/annotations.cpp e02994688414bdf485b308d6ef122ee2eff3fbaf 
>   core/annotations_p.h 07b124a4fae40b7a983aa382ae824125e6d25746 
>   core/document.h 63f58741bd6680a673945a7b7c05a10130968beb 
>   core/document.cpp 6953b1fb0dc29a375be7ff331a2a2bccce975366 
>   core/document_p.h fda23275033645ea67f5ad9d27341fc4635ede34 
>   core/documentcommands.cpp 95aded51d73a3d3b98ff26284c9c46fc5c9cf0ca 
>   core/documentcommands_p.h 17394f2a25b187cf4aff66b3a7f891b81be5acdd 
>   generators/poppler/annots.cpp 8cde64833831ec833b3be552608cff99d38f8e63 
>   tests/translateannotationtest.cpp 184b9474e6072a991a5ee5f1116bf7a9ef10cadc 
>   ui/pagepainter.h 68b241658162d9bd6eb187efc594ef17ea99d899 
>   ui/pagepainter.cpp 3bcd8bc4cfe7471bc3c21cfcd3cff50b8a8d49ee 
>   ui/pageview.cpp 3ebf7dcb04aa1942e02f49133d98081e2bbb565c 
>   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 

Re: Review Request 127366: Resize annotations

[Oliver Sander]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127366/#review100047
---




ui/pagepainter.h (line 36)


That comma after '32' is not correct, is it?


- Oliver Sander


On April 28, 2016, 7:13 p.m., Tobias Deiminger wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127366/
> ---
> 
> (Updated April 28, 2016, 7:13 p.m.)
> 
> 
> Review request for Okular.
> 
> 
> Bugs: 18
> http://bugs.kde.org/show_bug.cgi?id=18
> 
> 
> Repository: okular
> 
> 
> Description
> ---
> 
> This diff adds an annotation resize feature to okular (see Bug 18).
> 
> 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:
> -For geometric annotations, only the the shape of the annotation is sensitive 
> for move/resize, but not the selection rectangle.
> 
> 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 97e8db6e4a704fd34331fad7b7628ca2248b62d8 
>   core/annotations.h 4f107440dc824fd9049a30082befd18642e63895 
>   core/annotations.cpp e02994688414bdf485b308d6ef122ee2eff3fbaf 
>   core/annotations_p.h 07b124a4fae40b7a983aa382ae824125e6d25746 
>   core/document.h 63f58741bd6680a673945a7b7c05a10130968beb 
>   core/document.cpp 6953b1fb0dc29a375be7ff331a2a2bccce975366 
>   core/document_p.h fda23275033645ea67f5ad9d27341fc4635ede34 
>   core/documentcommands.cpp 95aded51d73a3d3b98ff26284c9c46fc5c9cf0ca 
>   core/documentcommands_p.h 17394f2a25b187cf4aff66b3a7f891b81be5acdd 
>   generators/poppler/annots.cpp 8cde64833831ec833b3be552608cff99d38f8e63 
>   tests/translateannotationtest.cpp 184b9474e6072a991a5ee5f1116bf7a9ef10cadc 
>   ui/pagepainter.h 68b241658162d9bd6eb187efc594ef17ea99d899 
>   ui/pagepainter.cpp 3bcd8bc4cfe7471bc3c21cfcd3cff50b8a8d49ee 
>   ui/pageview.cpp 3ebf7dcb04aa1942e02f49133d98081e2bbb565c 
>   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 

Re: Review Request 127366: Resize annotations

[Carl-Daniel Hailfinger]

> On März 14, 2016, 12:46 vorm., Albert Astals Cid wrote:
> > Looks very nice for a start, congratulations.
> > 
> > > Reconsider UI, esp. the Ctrl-way to get focus. Is there something in the 
> > > KDE HIG about it?
> > 
> > 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)
> > 
> > 
> > > Improve handling when annotation gets so small that resize handles 
> > > overlap.
> > 
> > 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?
> > 
> > 
> > > Find approach how to handle "resize to negative geometry".
> > 
> > I think not letting people go negative is what makes more sense
> > 
> > 
> > > Known Bug: In the thumbnail list, resize handles are drawn too big, and 
> > > not refreshed correctly.
> > 
> > I'd say it makes more sense to not draw the handles at all
> 
> Tobias Deiminger wrote:
> 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
> 
> Changed:
> -resize handles are filled slightly transparent
> 
> 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.
> 
> 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.
> 
> 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.
> 
> Btw., sorry, I don't know how inline quote here (using the reviewboard 
> webinterface).
> 
> Jonathan Schultz wrote:
> 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.
> 
> 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.
> 
> Albert Astals Cid wrote:
> > 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.
> 
> It's for the "mobile" version of the app, it doesn't have the complex 
> interaction modes (i think) so ignore for now.
> 
> > Btw., sorry, I don't know how inline quote here (using the reviewboard 
> webinterface).
> 
> 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
> 
> Tobias Deiminger wrote:
> > annotations should be select-able, by left-clicking on some obvious 
> part of them, and once selected can then be moved/resized
> 
> Calligra and Karbon do it like Jonathan says. I liked the idea too, so 
> gave it a try and find the handling better now. What do you think?
> Could there be problems because of conflicting user intents (e.g., move 
> the whole document vs. move the annotation)?
> I can quite easily revert or change it again, as design has also improved 
> now.
> 
> > it overlaps with stuff I am working on to do more with selections
> 
> Sounds interesting... who would eventually care about merging?
> 
> Tobias Deiminger wrote:
> Albert, can you already vote for or against following changes? (concerns 
> rev 5 of the diff)
> 
> 1) The patch changes the UI for annotations: Away from "hold ctrl-key 
> while operating on", towards "select by left click, then operate on". See 
> "Usage" in descirption of this RR, or just apply the patch and try it. It's 
> similiar to other KDE applications (calligra flow, karbon, words, ...), and I 
> find it more self-explaining. On the other hand, it breaks behavior okular 
> users might be used to. If in doubt, rev 5 should be suitable for giving a 
> working prototype to kde-usability list and ask them.
> 
> 2) The patch considerably modifies class PageView. The idea is to have 
> all "operate on annotation" functionality in class MouseAnnotation, instead 
> of interleaving it in the longish PageView. PageView then only delegates 
> events to MouseAnnotation. This concerns also non-resize-related features. 
> E.g. I moved annotation tooltips and video playback there. It was almost 
> 

Re: [Okular-devel] Review Request 127366: Resize annotations

[Albert Astals Cid]

> On March 14, 2016, 12:46 a.m., Albert Astals Cid wrote:
> > Looks very nice for a start, congratulations.
> > 
> > > Reconsider UI, esp. the Ctrl-way to get focus. Is there something in the 
> > > KDE HIG about it?
> > 
> > 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)
> > 
> > 
> > > Improve handling when annotation gets so small that resize handles 
> > > overlap.
> > 
> > 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?
> > 
> > 
> > > Find approach how to handle "resize to negative geometry".
> > 
> > I think not letting people go negative is what makes more sense
> > 
> > 
> > > Known Bug: In the thumbnail list, resize handles are drawn too big, and 
> > > not refreshed correctly.
> > 
> > I'd say it makes more sense to not draw the handles at all
> 
> Tobias Deiminger wrote:
> 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
> 
> Changed:
> -resize handles are filled slightly transparent
> 
> 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.
> 
> 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.
> 
> 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.
> 
> Btw., sorry, I don't know how inline quote here (using the reviewboard 
> webinterface).
> 
> Jonathan Schultz wrote:
> 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.
> 
> 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.
> 
> Albert Astals Cid wrote:
> > 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.
> 
> It's for the "mobile" version of the app, it doesn't have the complex 
> interaction modes (i think) so ignore for now.
> 
> > Btw., sorry, I don't know how inline quote here (using the reviewboard 
> webinterface).
> 
> 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
> 
> Tobias Deiminger wrote:
> > annotations should be select-able, by left-clicking on some obvious 
> part of them, and once selected can then be moved/resized
> 
> Calligra and Karbon do it like Jonathan says. I liked the idea too, so 
> gave it a try and find the handling better now. What do you think?
> Could there be problems because of conflicting user intents (e.g., move 
> the whole document vs. move the annotation)?
> I can quite easily revert or change it again, as design has also improved 
> now.
> 
> > it overlaps with stuff I am working on to do more with selections
> 
> Sounds interesting... who would eventually care about merging?
> 
> Tobias Deiminger wrote:
> Albert, can you already vote for or against following changes? (concerns 
> rev 5 of the diff)
> 
> 1) The patch changes the UI for annotations: Away from "hold ctrl-key 
> while operating on", towards "select by left click, then operate on". See 
> "Usage" in descirption of this RR, or just apply the patch and try it. It's 
> similiar to other KDE applications (calligra flow, karbon, words, ...), and I 
> find it more self-explaining. On the other hand, it breaks behavior okular 
> users might be used to. If in doubt, rev 5 should be suitable for giving a 
> working prototype to kde-usability list and ask them.
> 
> 2) The patch considerably modifies class PageView. The idea is to have 
> all "operate on annotation" functionality in class MouseAnnotation, instead 
> of interleaving it in the longish PageView. PageView then only delegates 
> events to MouseAnnotation. This concerns also non-resize-related features. 
> E.g. I moved annotation tooltips and video playback there. It was almost 
> 

Re: [Okular-devel] Review Request 127366: Resize annotations

[Tobias Deiminger]

> On März 14, 2016, 12:46 vorm., Albert Astals Cid wrote:
> > Looks very nice for a start, congratulations.
> > 
> > > Reconsider UI, esp. the Ctrl-way to get focus. Is there something in the 
> > > KDE HIG about it?
> > 
> > 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)
> > 
> > 
> > > Improve handling when annotation gets so small that resize handles 
> > > overlap.
> > 
> > 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?
> > 
> > 
> > > Find approach how to handle "resize to negative geometry".
> > 
> > I think not letting people go negative is what makes more sense
> > 
> > 
> > > Known Bug: In the thumbnail list, resize handles are drawn too big, and 
> > > not refreshed correctly.
> > 
> > I'd say it makes more sense to not draw the handles at all
> 
> Tobias Deiminger wrote:
> 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
> 
> Changed:
> -resize handles are filled slightly transparent
> 
> 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.
> 
> 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.
> 
> 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.
> 
> Btw., sorry, I don't know how inline quote here (using the reviewboard 
> webinterface).
> 
> Jonathan Schultz wrote:
> 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.
> 
> 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.
> 
> Albert Astals Cid wrote:
> > 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.
> 
> It's for the "mobile" version of the app, it doesn't have the complex 
> interaction modes (i think) so ignore for now.
> 
> > Btw., sorry, I don't know how inline quote here (using the reviewboard 
> webinterface).
> 
> 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
> 
> Tobias Deiminger wrote:
> > annotations should be select-able, by left-clicking on some obvious 
> part of them, and once selected can then be moved/resized
> 
> Calligra and Karbon do it like Jonathan says. I liked the idea too, so 
> gave it a try and find the handling better now. What do you think?
> Could there be problems because of conflicting user intents (e.g., move 
> the whole document vs. move the annotation)?
> I can quite easily revert or change it again, as design has also improved 
> now.
> 
> > it overlaps with stuff I am working on to do more with selections
> 
> Sounds interesting... who would eventually care about merging?

Albert, can you already vote for or against following changes? (concerns rev 5 
of the diff)

1) The patch changes the UI for annotations: Away from "hold ctrl-key while 
operating on", towards "select by left click, then operate on". See "Usage" in 
descirption of this RR, or just apply the patch and try it. It's similiar to 
other KDE applications (calligra flow, karbon, words, ...), and I find it more 
self-explaining. On the other hand, it breaks behavior okular users might be 
used to. If in doubt, rev 5 should be suitable for giving a working prototype 
to kde-usability list and ask them.

2) The patch considerably modifies class PageView. The idea is to have all 
"operate on annotation" functionality in class MouseAnnotation, instead of 
interleaving it in the longish PageView. PageView then only delegates events to 
MouseAnnotation. This concerns also non-resize-related features. E.g. I moved 
annotation tooltips and video playback there. It was almost necessary for me to 
gain an overview of existing features and behavior, and it 

Re: [Okular-devel] Review Request 127366: Resize annotations

[Tobias Deiminger]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127366/
---

(Updated April 28, 2016, 7:13 nachm.)


Review request for Okular.


Changes
---

Linked with bug report on bugs.kde.org


Bugs: 18
http://bugs.kde.org/show_bug.cgi?id=18


Repository: okular


Description
---

This diff adds an annotation resize feature to okular (see Bug 18).

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:
-For geometric annotations, only the the shape of the annotation is sensitive 
for move/resize, but not the selection rectangle.

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 97e8db6e4a704fd34331fad7b7628ca2248b62d8 
  core/annotations.h 4f107440dc824fd9049a30082befd18642e63895 
  core/annotations.cpp e02994688414bdf485b308d6ef122ee2eff3fbaf 
  core/annotations_p.h 07b124a4fae40b7a983aa382ae824125e6d25746 
  core/document.h 63f58741bd6680a673945a7b7c05a10130968beb 
  core/document.cpp 6953b1fb0dc29a375be7ff331a2a2bccce975366 
  core/document_p.h fda23275033645ea67f5ad9d27341fc4635ede34 
  core/documentcommands.cpp 95aded51d73a3d3b98ff26284c9c46fc5c9cf0ca 
  core/documentcommands_p.h 17394f2a25b187cf4aff66b3a7f891b81be5acdd 
  generators/poppler/annots.cpp 8cde64833831ec833b3be552608cff99d38f8e63 
  tests/translateannotationtest.cpp 184b9474e6072a991a5ee5f1116bf7a9ef10cadc 
  ui/pagepainter.h 68b241658162d9bd6eb187efc594ef17ea99d899 
  ui/pagepainter.cpp 3bcd8bc4cfe7471bc3c21cfcd3cff50b8a8d49ee 
  ui/pageview.cpp 3ebf7dcb04aa1942e02f49133d98081e2bbb565c 
  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

___
Okular-devel mailing list
Okular-devel@kde.org
https://mail.kde.org/mailman/listinfo/okular-devel

Re: [Okular-devel] Review Request 127366: Resize annotations

[Tobias Deiminger]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127366/
---

(Updated April 27, 2016, 5:26 nachm.)


Review request for Okular.


Changes
---

Fixed: Mouse cursor keeps correct shape during resize action.
Fixed: Don't perform move/resize action immediately on click for select, but 
only upon first mouse move.
Changed: Class MouseAnnotation cares about starting 
AMovie/AScreen/AFileAttachment actions (e.g., start playback of embedded video 
annotation).
Changed: Class MouseAnnotation cares about annotation tooltip events.
Changed: Remember the resize/move handle, once an command is started.
Changed: Mouse cursor changes to arrow or pointing hand shape once the mouse is 
over an annotation (indicates the thing is selectable, or action can be 
triggered).
Changed: AnnotationDescription is kept as data member and passed by reference 
now.


Repository: okular


Description (updated)
---

This diff adds an annotation resize feature to okular (see Bug 18).

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:
-For geometric annotations, only the the shape of the annotation is sensitive 
for move/resize, but not the selection rectangle.

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 (updated)
-

  CMakeLists.txt 97e8db6e4a704fd34331fad7b7628ca2248b62d8 
  core/annotations.h 4f107440dc824fd9049a30082befd18642e63895 
  core/annotations.cpp e02994688414bdf485b308d6ef122ee2eff3fbaf 
  core/annotations_p.h 07b124a4fae40b7a983aa382ae824125e6d25746 
  core/document.h 63f58741bd6680a673945a7b7c05a10130968beb 
  core/document.cpp 6953b1fb0dc29a375be7ff331a2a2bccce975366 
  core/document_p.h fda23275033645ea67f5ad9d27341fc4635ede34 
  core/documentcommands.cpp 95aded51d73a3d3b98ff26284c9c46fc5c9cf0ca 
  core/documentcommands_p.h 17394f2a25b187cf4aff66b3a7f891b81be5acdd 
  generators/poppler/annots.cpp 8cde64833831ec833b3be552608cff99d38f8e63 
  tests/translateannotationtest.cpp 184b9474e6072a991a5ee5f1116bf7a9ef10cadc 
  ui/pagepainter.h 68b241658162d9bd6eb187efc594ef17ea99d899 
  ui/pagepainter.cpp 3bcd8bc4cfe7471bc3c21cfcd3cff50b8a8d49ee 
  ui/pageview.cpp 3ebf7dcb04aa1942e02f49133d98081e2bbb565c 
  ui/pageviewmouseannotation.h PRE-CREATION 
  ui/pageviewmouseannotation.cpp PRE-CREATION 

Diff: https://git.reviewboard.kde.org/r/127366/diff/


Testing (updated)
---

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 

Re: [Okular-devel] Review Request 127366: Resize annotations

[Tobias Deiminger]

> On April 7, 2016, 9:52 nachm., 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.

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!


- Tobias


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127366/#review94355
---


On April 5, 2016, 8:02 vorm., Tobias Deiminger wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127366/
> ---
> 
> (Updated April 5, 2016, 8:02 vorm.)
> 
> 
> Review request for Okular.
> 
> 
> Repository: okular
> 
> 
> Description
> ---
> 
> This diff adds an annotation resize feature to okular (see Bug 18).
> 
> 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
> 
> TODO:
> -Add test cases once requirements are fixed.
> 
> 
> Diffs
> -
> 
>   CMakeLists.txt 97e8db6e4a704fd34331fad7b7628ca2248b62d8 
>   core/annotations.h 4f107440dc824fd9049a30082befd18642e63895 
>   

Re: [Okular-devel] Review Request 127366: Resize annotations

[Jonathan Schultz]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127366/#review94355
---



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.

- Jonathan Schultz


On April 5, 2016, 8:02 a.m., Tobias Deiminger wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127366/
> ---
> 
> (Updated April 5, 2016, 8:02 a.m.)
> 
> 
> Review request for Okular.
> 
> 
> Repository: okular
> 
> 
> Description
> ---
> 
> This diff adds an annotation resize feature to okular (see Bug 18).
> 
> 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
> 
> TODO:
> -Add test cases once requirements are fixed.
> 
> 
> Diffs
> -
> 
>   CMakeLists.txt 97e8db6e4a704fd34331fad7b7628ca2248b62d8 
>   core/annotations.h 4f107440dc824fd9049a30082befd18642e63895 
>   core/annotations.cpp e02994688414bdf485b308d6ef122ee2eff3fbaf 
>   core/annotations_p.h 07b124a4fae40b7a983aa382ae824125e6d25746 
>   core/document.h 63f58741bd6680a673945a7b7c05a10130968beb 
>   core/document.cpp 6953b1fb0dc29a375be7ff331a2a2bccce975366 
>   core/document_p.h fda23275033645ea67f5ad9d27341fc4635ede34 
>   core/documentcommands.cpp 95aded51d73a3d3b98ff26284c9c46fc5c9cf0ca 
>   core/documentcommands_p.h 17394f2a25b187cf4aff66b3a7f891b81be5acdd 
>   generators/poppler/annots.cpp 8cde64833831ec833b3be552608cff99d38f8e63 
>   tests/translateannotationtest.cpp 184b9474e6072a991a5ee5f1116bf7a9ef10cadc 
>   ui/pagepainter.h 68b241658162d9bd6eb187efc594ef17ea99d899 
>   ui/pagepainter.cpp 3bcd8bc4cfe7471bc3c21cfcd3cff50b8a8d49ee 
>   ui/pageview.cpp 3ebf7dcb04aa1942e02f49133d98081e2bbb565c 
>   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 

Re: [Okular-devel] Review Request 127366: Resize annotations

[Tobias Deiminger]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127366/
---

(Updated April 5, 2016, 8:02 vorm.)


Review request for Okular.


Changes
---

API semantics: Changed Document::resizePageAnnotation to 
Document::adjustPageAnnotation (seems more consistent to 
translatePageAnnotation).
Cancel selection when a annotation is deleted.
Paint only areas of interest.
Refactored internals of PageViewMouseAnnotation.
Fixes in paint-, rotation- and "click outside" handling.


Repository: okular


Description (updated)
---

This diff adds an annotation resize feature to okular (see Bug 18).

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

TODO:
-Add test cases once requirements are fixed.


Diffs (updated)
-

  CMakeLists.txt 97e8db6e4a704fd34331fad7b7628ca2248b62d8 
  core/annotations.h 4f107440dc824fd9049a30082befd18642e63895 
  core/annotations.cpp e02994688414bdf485b308d6ef122ee2eff3fbaf 
  core/annotations_p.h 07b124a4fae40b7a983aa382ae824125e6d25746 
  core/document.h 63f58741bd6680a673945a7b7c05a10130968beb 
  core/document.cpp 6953b1fb0dc29a375be7ff331a2a2bccce975366 
  core/document_p.h fda23275033645ea67f5ad9d27341fc4635ede34 
  core/documentcommands.cpp 95aded51d73a3d3b98ff26284c9c46fc5c9cf0ca 
  core/documentcommands_p.h 17394f2a25b187cf4aff66b3a7f891b81be5acdd 
  generators/poppler/annots.cpp 8cde64833831ec833b3be552608cff99d38f8e63 
  tests/translateannotationtest.cpp 184b9474e6072a991a5ee5f1116bf7a9ef10cadc 
  ui/pagepainter.h 68b241658162d9bd6eb187efc594ef17ea99d899 
  ui/pagepainter.cpp 3bcd8bc4cfe7471bc3c21cfcd3cff50b8a8d49ee 
  ui/pageview.cpp 3ebf7dcb04aa1942e02f49133d98081e2bbb565c 
  ui/pageviewmouseannotation.h PRE-CREATION 
  ui/pageviewmouseannotation.cpp PRE-CREATION 

Diff: https://git.reviewboard.kde.org/r/127366/diff/


Testing (updated)
---

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.


Thanks,

Tobias Deiminger

___
Okular-devel mailing list
Okular-devel@kde.org
https://mail.kde.org/mailman/listinfo/okular-devel

Re: [Okular-devel] Review Request 127366: Resize annotations

[Tobias Deiminger]

> On März 14, 2016, 12:46 vorm., Albert Astals Cid wrote:
> > Looks very nice for a start, congratulations.
> > 
> > > Reconsider UI, esp. the Ctrl-way to get focus. Is there something in the 
> > > KDE HIG about it?
> > 
> > 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)
> > 
> > 
> > > Improve handling when annotation gets so small that resize handles 
> > > overlap.
> > 
> > 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?
> > 
> > 
> > > Find approach how to handle "resize to negative geometry".
> > 
> > I think not letting people go negative is what makes more sense
> > 
> > 
> > > Known Bug: In the thumbnail list, resize handles are drawn too big, and 
> > > not refreshed correctly.
> > 
> > I'd say it makes more sense to not draw the handles at all
> 
> Tobias Deiminger wrote:
> 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
> 
> Changed:
> -resize handles are filled slightly transparent
> 
> 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.
> 
> 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.
> 
> 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.
> 
> Btw., sorry, I don't know how inline quote here (using the reviewboard 
> webinterface).
> 
> Jonathan Schultz wrote:
> 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.
> 
> 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.
> 
> Albert Astals Cid wrote:
> > 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.
> 
> It's for the "mobile" version of the app, it doesn't have the complex 
> interaction modes (i think) so ignore for now.
> 
> > Btw., sorry, I don't know how inline quote here (using the reviewboard 
> webinterface).
> 
> 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

> annotations should be select-able, by left-clicking on some obvious part of 
> them, and once selected can then be moved/resized

Calligra and Karbon do it like Jonathan says. I liked the idea too, so gave it 
a try and find the handling better now. What do you think?
Could there be problems because of conflicting user intents (e.g., move the 
whole document vs. move the annotation)?
I can quite easily revert or change it again, as design has also improved now.

> it overlaps with stuff I am working on to do more with selections

Sounds interesting... who would eventually care about merging?


- Tobias


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127366/#review93501
---


On März 26, 2016, 8:25 nachm., Tobias Deiminger wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127366/
> ---
> 
> (Updated März 26, 2016, 8:25 nachm.)
> 
> 
> Review request for Okular.
> 
> 
> Repository: okular
> 
> 
> Description
> ---
> 
> This diff adds an annotation resize feature to okular (see Bug 18).
> 
> 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 

Re: [Okular-devel] Review Request 127366: Resize annotations

[Tobias Deiminger]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127366/
---

(Updated März 26, 2016, 8:25 nachm.)


Review request for Okular.


Repository: okular


Description (updated)
---

This diff adds an annotation resize feature to okular (see Bug 18).

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::resizePageAnnotation, backed by a QUndoCommand class 
Okular::ResizeAnnotationCommand
-Added method Annotation::resize
-Draw resize handles and selection boundary in MouseAnnotation::routePaint
-Draw only a bounding rectangle during resize, if annotation is rendered 
externally

TODO:
-Add test cases once requirements are fixed.


Diffs (updated)
-

  CMakeLists.txt 97e8db6e4a704fd34331fad7b7628ca2248b62d8 
  core/annotations.h 4f107440dc824fd9049a30082befd18642e63895 
  core/annotations.cpp e02994688414bdf485b308d6ef122ee2eff3fbaf 
  core/annotations_p.h 07b124a4fae40b7a983aa382ae824125e6d25746 
  core/document.h 63f58741bd6680a673945a7b7c05a10130968beb 
  core/document.cpp 6953b1fb0dc29a375be7ff331a2a2bccce975366 
  core/document_p.h fda23275033645ea67f5ad9d27341fc4635ede34 
  core/documentcommands.cpp 95aded51d73a3d3b98ff26284c9c46fc5c9cf0ca 
  core/documentcommands_p.h 17394f2a25b187cf4aff66b3a7f891b81be5acdd 
  generators/poppler/annots.cpp 8cde64833831ec833b3be552608cff99d38f8e63 
  tests/translateannotationtest.cpp 184b9474e6072a991a5ee5f1116bf7a9ef10cadc 
  ui/pagepainter.h 68b241658162d9bd6eb187efc594ef17ea99d899 
  ui/pagepainter.cpp 3bcd8bc4cfe7471bc3c21cfcd3cff50b8a8d49ee 
  ui/pageview.cpp 3ebf7dcb04aa1942e02f49133d98081e2bbb565c 
  ui/pageviewmouseannotation.h PRE-CREATION 
  ui/pageviewmouseannotation.cpp PRE-CREATION 

Diff: https://git.reviewboard.kde.org/r/127366/diff/


Testing
---


File Attachments


annotationresize.diff
  
https://git.reviewboard.kde.org/media/uploaded/files/2016/03/18/c9141599-8eb1-4054-8eb3-d81eec44fb94__annotationresize.diff
annotationresize.diff
  
https://git.reviewboard.kde.org/media/uploaded/files/2016/03/18/9d63276d-4765-406f-ad1d-6d4176b88c53__annotationresize.diff


Thanks,

Tobias Deiminger

___
Okular-devel mailing list
Okular-devel@kde.org
https://mail.kde.org/mailman/listinfo/okular-devel

Re: [Okular-devel] Review Request 127366: Resize annotations

[Albert Astals Cid]

> On March 14, 2016, 12:46 a.m., Albert Astals Cid wrote:
> > Looks very nice for a start, congratulations.
> > 
> > > Reconsider UI, esp. the Ctrl-way to get focus. Is there something in the 
> > > KDE HIG about it?
> > 
> > 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)
> > 
> > 
> > > Improve handling when annotation gets so small that resize handles 
> > > overlap.
> > 
> > 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?
> > 
> > 
> > > Find approach how to handle "resize to negative geometry".
> > 
> > I think not letting people go negative is what makes more sense
> > 
> > 
> > > Known Bug: In the thumbnail list, resize handles are drawn too big, and 
> > > not refreshed correctly.
> > 
> > I'd say it makes more sense to not draw the handles at all
> 
> Tobias Deiminger wrote:
> 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
> 
> Changed:
> -resize handles are filled slightly transparent
> 
> 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.
> 
> 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.
> 
> 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.
> 
> Btw., sorry, I don't know how inline quote here (using the reviewboard 
> webinterface).
> 
> Jonathan Schultz wrote:
> 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.
> 
> 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.

> 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.

It's for the "mobile" version of the app, it doesn't have the complex 
interaction modes (i think) so ignore for now.

> Btw., sorry, I don't know how inline quote here (using the reviewboard 
> webinterface).

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


- Albert


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127366/#review93501
---


On March 18, 2016, 8:06 p.m., Tobias Deiminger wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127366/
> ---
> 
> (Updated March 18, 2016, 8:06 p.m.)
> 
> 
> Review request for Okular.
> 
> 
> Repository: okular
> 
> 
> Description
> ---
> 
> This diff adds a first version of an annotation resize feature to okular (see 
> Bug 18). 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.
> 
> 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.
> 
> 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 
> 

Re: [Okular-devel] Review Request 127366: Resize annotations

[Jonathan Schultz]

> On March 14, 2016, 12:46 a.m., Albert Astals Cid wrote:
> > Looks very nice for a start, congratulations.
> > 
> > > Reconsider UI, esp. the Ctrl-way to get focus. Is there something in the 
> > > KDE HIG about it?
> > 
> > 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)
> > 
> > 
> > > Improve handling when annotation gets so small that resize handles 
> > > overlap.
> > 
> > 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?
> > 
> > 
> > > Find approach how to handle "resize to negative geometry".
> > 
> > I think not letting people go negative is what makes more sense
> > 
> > 
> > > Known Bug: In the thumbnail list, resize handles are drawn too big, and 
> > > not refreshed correctly.
> > 
> > I'd say it makes more sense to not draw the handles at all
> 
> Tobias Deiminger wrote:
> 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
> 
> Changed:
> -resize handles are filled slightly transparent
> 
> 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.
> 
> 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.
> 
> 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.
> 
> Btw., sorry, I don't know how inline quote here (using the reviewboard 
> webinterface).

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.

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.


- Jonathan


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127366/#review93501
---


On March 18, 2016, 8:06 p.m., Tobias Deiminger wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127366/
> ---
> 
> (Updated March 18, 2016, 8:06 p.m.)
> 
> 
> Review request for Okular.
> 
> 
> Repository: okular
> 
> 
> Description
> ---
> 
> This diff adds a first version of an annotation resize feature to okular (see 
> Bug 18). 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.
> 
> 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.
> 
> 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
> 
> 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 

Re: [Okular-devel] Review Request 127366: Resize annotations

[Tobias Deiminger]

> On März 14, 2016, 12:46 vorm., Albert Astals Cid wrote:
> > Looks very nice for a start, congratulations.
> > 
> > > Reconsider UI, esp. the Ctrl-way to get focus. Is there something in the 
> > > KDE HIG about it?
> > 
> > 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)
> > 
> > 
> > > Improve handling when annotation gets so small that resize handles 
> > > overlap.
> > 
> > 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?
> > 
> > 
> > > Find approach how to handle "resize to negative geometry".
> > 
> > I think not letting people go negative is what makes more sense
> > 
> > 
> > > Known Bug: In the thumbnail list, resize handles are drawn too big, and 
> > > not refreshed correctly.
> > 
> > I'd say it makes more sense to not draw the handles at all

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

Changed:
-resize handles are filled slightly transparent

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.

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.

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.

Btw., sorry, I don't know how inline quote here (using the reviewboard 
webinterface).


- Tobias


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127366/#review93501
---


On März 18, 2016, 8:06 nachm., Tobias Deiminger wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127366/
> ---
> 
> (Updated März 18, 2016, 8:06 nachm.)
> 
> 
> Review request for Okular.
> 
> 
> Repository: okular
> 
> 
> Description
> ---
> 
> This diff adds a first version of an annotation resize feature to okular (see 
> Bug 18). 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.
> 
> 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.
> 
> 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
> 
> 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.
> 
> 
> Diffs
> -
> 
>   CMakeLists.txt 97e8db6e4a704fd34331fad7b7628ca2248b62d8 
>   core/annotations.h 4f107440dc824fd9049a30082befd18642e63895 
>   core/annotations.cpp e02994688414bdf485b308d6ef122ee2eff3fbaf 
>   core/annotations_p.h 07b124a4fae40b7a983aa382ae824125e6d25746 
>   core/document.h 63f58741bd6680a673945a7b7c05a10130968beb 
>   core/document.cpp 6953b1fb0dc29a375be7ff331a2a2bccce975366 
>   

Re: [Okular-devel] Review Request 127366: Resize annotations

[Tobias Deiminger]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127366/
---

(Updated März 18, 2016, 8:06 nachm.)


Review request for Okular.


Repository: okular


Description (updated)
---

This diff adds a first version of an annotation resize feature to okular (see 
Bug 18). 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.

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.

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

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.


Diffs (updated)
-

  CMakeLists.txt 97e8db6e4a704fd34331fad7b7628ca2248b62d8 
  core/annotations.h 4f107440dc824fd9049a30082befd18642e63895 
  core/annotations.cpp e02994688414bdf485b308d6ef122ee2eff3fbaf 
  core/annotations_p.h 07b124a4fae40b7a983aa382ae824125e6d25746 
  core/document.h 63f58741bd6680a673945a7b7c05a10130968beb 
  core/document.cpp 6953b1fb0dc29a375be7ff331a2a2bccce975366 
  core/document_p.h fda23275033645ea67f5ad9d27341fc4635ede34 
  core/documentcommands.cpp 95aded51d73a3d3b98ff26284c9c46fc5c9cf0ca 
  core/documentcommands_p.h 17394f2a25b187cf4aff66b3a7f891b81be5acdd 
  generators/poppler/annots.cpp 8cde64833831ec833b3be552608cff99d38f8e63 
  ui/pagepainter.h 68b241658162d9bd6eb187efc594ef17ea99d899 
  ui/pagepainter.cpp 57e8620cdefc36e40660fce242d7ea8197c25339 
  ui/pageview.cpp 3ebf7dcb04aa1942e02f49133d98081e2bbb565c 
  ui/pageviewmouseannotation.h PRE-CREATION 
  ui/pageviewmouseannotation.cpp PRE-CREATION 

Diff: https://git.reviewboard.kde.org/r/127366/diff/


Testing
---


File Attachments (updated)


annotationresize.diff
  
https://git.reviewboard.kde.org/media/uploaded/files/2016/03/18/c9141599-8eb1-4054-8eb3-d81eec44fb94__annotationresize.diff
annotationresize.diff
  
https://git.reviewboard.kde.org/media/uploaded/files/2016/03/18/9d63276d-4765-406f-ad1d-6d4176b88c53__annotationresize.diff


Thanks,

Tobias Deiminger

___
Okular-devel mailing list
Okular-devel@kde.org
https://mail.kde.org/mailman/listinfo/okular-devel

Re: [Okular-devel] Review Request 127366: Resize annotations

[Albert Astals Cid]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127366/#review93501
---



Looks very nice for a start, congratulations.

> Reconsider UI, esp. the Ctrl-way to get focus. Is there something in the KDE 
> HIG about it?

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)


> Improve handling when annotation gets so small that resize handles overlap.

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?


> Find approach how to handle "resize to negative geometry".

I think not letting people go negative is what makes more sense


> Known Bug: In the thumbnail list, resize handles are drawn too big, and not 
> refreshed correctly.

I'd say it makes more sense to not draw the handles at all

- Albert Astals Cid


On March 13, 2016, 8:25 p.m., Tobias Deiminger wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127366/
> ---
> 
> (Updated March 13, 2016, 8:25 p.m.)
> 
> 
> Review request for Okular.
> 
> 
> Repository: okular
> 
> 
> Description
> ---
> 
> This diff adds a first version of an annotation resize feature to okular (see 
> Bug 18). 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.
> 
> 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.
> 
> 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
> 
> 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.
> -Find approach how to handle "resize to negative geometry". Maybe flip resize 
> mode, e.g. from top left to bottom right when hitting size 0. Or just deny 
> further lessening at size 0.
> -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.
> -Known Bug: In the thumbnail list, resize handles are drawn too big, and not 
> refreshed correctly.
> -Known Bug: When page orientation is changed (e.g. rotated by 90 deg.), wrong 
> resize mode is applied.
> 
> 
> Diffs
> -
> 
>   CMakeLists.txt 97e8db6e4a704fd34331fad7b7628ca2248b62d8 
>   core/annotations.h 4f107440dc824fd9049a30082befd18642e63895 
>   core/annotations.cpp e02994688414bdf485b308d6ef122ee2eff3fbaf 
>   core/annotations_p.h 07b124a4fae40b7a983aa382ae824125e6d25746 
>   core/document.h 63f58741bd6680a673945a7b7c05a10130968beb 
>   core/document.cpp 6953b1fb0dc29a375be7ff331a2a2bccce975366 
>   core/document_p.h fda23275033645ea67f5ad9d27341fc4635ede34 
>   core/documentcommands.cpp 95aded51d73a3d3b98ff26284c9c46fc5c9cf0ca 
>   core/documentcommands_p.h 17394f2a25b187cf4aff66b3a7f891b81be5acdd 
>   generators/poppler/annots.cpp 8cde64833831ec833b3be552608cff99d38f8e63 
>   ui/pagepainter.h 68b241658162d9bd6eb187efc594ef17ea99d899 
>   ui/pagepainter.cpp 57e8620cdefc36e40660fce242d7ea8197c25339 
>   ui/pageview.cpp 3ebf7dcb04aa1942e02f49133d98081e2bbb565c 
>   ui/pageviewmouseannotation.h PRE-CREATION 
>   ui/pageviewmouseannotation.cpp PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/127366/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Tobias Deiminger
> 
>

___

[Okular-devel] Review Request 127366: Resize annotations

[Tobias Deiminger]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127366/
---

Review request for Okular.


Repository: okular


Description
---

This diff adds a first version of an annotation resize feature to okular (see 
Bug 18). 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.

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.

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

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.
-Find approach how to handle "resize to negative geometry". Maybe flip resize 
mode, e.g. from top left to bottom right when hitting size 0. Or just deny 
further lessening at size 0.
-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.
-Known Bug: In the thumbnail list, resize handles are drawn too big, and not 
refreshed correctly.
-Known Bug: When page orientation is changed (e.g. rotated by 90 deg.), wrong 
resize mode is applied.


Diffs
-

  CMakeLists.txt 97e8db6e4a704fd34331fad7b7628ca2248b62d8 
  core/annotations.h 4f107440dc824fd9049a30082befd18642e63895 
  core/annotations.cpp e02994688414bdf485b308d6ef122ee2eff3fbaf 
  core/annotations_p.h 07b124a4fae40b7a983aa382ae824125e6d25746 
  core/document.h 63f58741bd6680a673945a7b7c05a10130968beb 
  core/document.cpp 6953b1fb0dc29a375be7ff331a2a2bccce975366 
  core/document_p.h fda23275033645ea67f5ad9d27341fc4635ede34 
  core/documentcommands.cpp 95aded51d73a3d3b98ff26284c9c46fc5c9cf0ca 
  core/documentcommands_p.h 17394f2a25b187cf4aff66b3a7f891b81be5acdd 
  generators/poppler/annots.cpp 8cde64833831ec833b3be552608cff99d38f8e63 
  ui/pagepainter.h 68b241658162d9bd6eb187efc594ef17ea99d899 
  ui/pagepainter.cpp 57e8620cdefc36e40660fce242d7ea8197c25339 
  ui/pageview.cpp 3ebf7dcb04aa1942e02f49133d98081e2bbb565c 
  ui/pageviewmouseannotation.h PRE-CREATION 
  ui/pageviewmouseannotation.cpp PRE-CREATION 

Diff: https://git.reviewboard.kde.org/r/127366/diff/


Testing
---


Thanks,

Tobias Deiminger

___
Okular-devel mailing list
Okular-devel@kde.org
https://mail.kde.org/mailman/listinfo/okular-devel