> 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 > necessary for me to gain an overview of existing features and behavior, and > it should aid keeping consistency and avoiding conflicts in the UI. It also > helps to identify and separate common code. On the other hand there's more to > review for you, and there's higher risk of introducing regressions. > > Are both changes OK for you? If so, I'd go on with identifiying missing > things, tidying up and testing. > > If you don't agree, I'll stop for now and wait until there's time for > discussion on alternate approaches. > > Cheers > Tobias > > Albert Astals Cid wrote: > Sorry, i have a huge backlog of thigns to do, will try to get this review > done as soon as possible :/
Tested against latest git, works fine. The "select by left click, then operate on" UI is pretty nice. You may have to regenerate the patch. Some files started to show offsets when applying. - Carl-Daniel ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/127366/#review93501 ----------------------------------------------------------- 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: 177778 > http://bugs.kde.org/show_bug.cgi?id=177778 > > > Repository: okular > > > Description > ------- > > This diff adds an annotation resize feature to okular (see Bug 177778). > > Usage: > If you left-click at an annotation, it gets selected and 8 resize handles > appear on the corners/edges of the selection rectangle. When cursor is moved > over one of the handles, the cursor shape indicates resize mode (everywhere > else on the annotation means "move", just as it was before resize feature was > added). Press ESC, or click an area outside the annotation to cancel > selection. Feature is only applicable for annotation types AText, AStamp and > AGeom. > > Notable changes: > It works by eventually changing AnnotationPrivate::m_boundary and notifying > generator (i.e. poppler) about that change, similar to the existing move > functionality. > -Separated annotation state handling out of PageView into a new class > MouseAnnotation (ui/pageviewmouseannotation.cpp) > -Added method Document::adjustPageAnnotation, backed by a QUndoCommand class > Okular::AdjustAnnotationCommand > -Added method Annotation::adjust > -Draw resize handles and selection boundary in MouseAnnotation::routePaint > -Draw only a bounding rectangle during resize, if annotation is rendered > externally > Some functionality unrelated to the resize feature is also shifted to class > MouseAnnotation, to establish a single place of responsibility for annotation > interactions. > > Known Bugs: > -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 > >
