> 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 > 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 :/ > > Carl-Daniel Hailfinger wrote: > 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. > > Oliver Sander wrote: > May I politely ask about the status of this? Does it still need further > review, and manpower is lacking? > > The patch applies to the master branch. Will it get merged first, and > then forward-ported to the frameworks branch? > Or does it need to get ported to KF5 first, and then get merged directly > into frameworks (skipping master)? > > Tobias Deiminger wrote: > This is up to the okular developer team, so I just join Olivers polite > ping here. From my side there are some minor changes and bug fixes pending. > Some more effort should go into considering and testing impact on more exotic > annotation subtypes. I'll stay tuned and will try to help once there's time > for further actions. > > Jonathan Schultz wrote: > I'm also pretty interested in this issue as part of a broader agenda of > building a more elaborate coding system based on Okular. I won't go into more > detail here, except to say that I think a lot of useful stuff like Tobias' > patch could usefully be incorporated into Okular. So FWIW I'm happy to do > some building/testing/reviewing/extending as may help to make progress. > > Oliver Sander wrote: > The patch does not apply anymore now that the frameworks branch has been > merged into master. Would it be able for you rebase the patch and do the > necessary changes to make it work with QT5 etc? Thanks! > > Oliver Sander wrote: > Just tested the new frameworks version, and it seems to work very nicely. > > Can we please get code review and have this merged? Is there anything I > can do to help? > > Oliver Sander wrote: > Polite ping? > > Albert Astals Cid wrote: > There's things marked as "Known Bugs", are those going to be fixed? > > Tobias Deiminger wrote: > Yes, I will fix them (or disable buggy functions if it'd turned out that > fixing is too tough). > It'd be great to get green lights for the overall approach before > spending more time - Albert, have you or someone from the okular team already > had a chance to look at the patch in more depth? > > Albert Astals Cid wrote: > > Albert, have you or someone from the okular team already had a chance > to look at the patch in more depth? > > There's some obvious but easy to fix mistakes like renaming an enum and > leaving the @since untouched; the variables of adjust() missing const; the > virtuals in the AdjustAnnotationCommand have to be overrides, the connects > using old style instead of new, etc. but this is just small stuff. > > Then as a user, it's obvious that once you let me select an annotation by > mouse click, i want the Delete key to delete it. > > Tobias Deiminger wrote: > Thanks for the review! Rev 7 fixes the mistakes you mentioned and > implements the Delete key feature. A few comments: > > > virtuals in the AdjustAnnotationCommand have to be overrides > > They're overrides, aren't they? I double checked the signature of the > virtuals from QUndoCommand and AdjustAnnotationCommand, but couldn't see a > difference. > > > renaming an enum and leaving the @since untouched > > While fixing this I realized that annotation.h is part of a public API > (there's even a Debian packages for it, > https://packages.debian.org/sid/libokularcore7). > Renaming BeingMoved to BeingModified would break existing client code, so > I'm keeping BeingMoved and add BeingResized now. > > I'll address the "Known bug" things soon. If you find other issues just > let me know.
> They're overrides, aren't they? I double checked the signature of the > virtuals from QUndoCommand and AdjustAnnotationCommand, but couldn't see a > difference. Yes, they are overrides, and thus should be marked as such, i.e. using the override keyword instead of the virtual one (since we use C++11 now) - Albert ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/127366/#review93501 ----------------------------------------------------------- On Jan. 26, 2017, 9:47 p.m., Tobias Deiminger wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/127366/ > ----------------------------------------------------------- > > (Updated Jan. 26, 2017, 9:47 p.m.) > > > Review request for Okular. > > > Bugs: 177778 > http://bugs.kde.org/show_bug.cgi?id=177778 > > > Repository: okular > > > Description > ------- > > This diff adds an annotation resize feature to okular (see Bug 177778). > > Usage: > If you left-click at an annotation, it gets selected and 8 resize handles > appear on the corners/edges of the selection rectangle. When cursor is moved > over one of the handles, the cursor shape indicates resize mode (everywhere > else on the annotation means "move", just as it was before resize feature was > added). Press ESC, or click an area outside the annotation to cancel > selection. Feature is only applicable for annotation types AText, AStamp and > AGeom. > > Notable changes: > It works by eventually changing AnnotationPrivate::m_boundary and notifying > generator (i.e. poppler) about that change, similar to the existing move > functionality. > -Separated annotation state handling out of PageView into a new class > MouseAnnotation (ui/pageviewmouseannotation.cpp) > -Added method Document::adjustPageAnnotation, backed by a QUndoCommand class > Okular::AdjustAnnotationCommand > -Added method Annotation::adjust > -Draw resize handles and selection boundary in MouseAnnotation::routePaint > -Draw only a bounding rectangle during resize, if annotation is rendered > externally > Some functionality unrelated to the resize feature is also shifted to class > MouseAnnotation, to establish a single place of responsibility for annotation > interactions. > > Known Bugs: > -A comment annotation (subtype 1) with embedded appearance can be selected > for resize. But the resize changes only the selection rectangle, while the > annotation is rendered unchanged. > -AWidget (subtype 13) can be selected for move and resize, but nothing > happens when performing those actions. > > TODO: > -Consider z-Order for overlapping annotations? (currently, selection > rectangle is always drawn on top, while the related annotation may be in > background). > -Provide a PDF document with suitable content, where supported annotation > types and and possible regressions can be tested. > -Add test cases once requirements are fixed. > > > Diffs > ----- > > CMakeLists.txt 78a2e855c22ebb31a9195c679cf0adf981d5443f > core/annotations.h 5653097fe892ce57c8e81615a1c20217e538e1de > core/annotations.cpp c95fe877316398a5341e29146379dd231dfa40c7 > core/annotations_p.h 07b124a4fae40b7a983aa382ae824125e6d25746 > core/document.h bac38f89f85980d478e6252ecc8dc823cbe4359a > core/document.cpp 468a8a9fbadaef5ca8540cf113d5397b989f2aa5 > core/document_p.h 8b20c5586f641f6f9ec3a6a6f0d978848a2cb7c8 > core/documentcommands.cpp aafc45a1a989a7240520f2168e253d51d6744f7c > core/documentcommands_p.h 616999dd68406590b304cf648878fa8acb3ec6e0 > generators/poppler/annots.cpp df67986adc076f722e601c3bea187200ecf9df31 > ui/pagepainter.cpp c5eb1ef90e4af48c644a7890a60fa3892cdcf08a > ui/pageview.cpp 8992539c7c282e865a3e5d20e119c8790d79e925 > ui/pageviewmouseannotation.h PRE-CREATION > ui/pageviewmouseannotation.cpp PRE-CREATION > > Diff: https://git.reviewboard.kde.org/r/127366/diff/ > > > Testing > ------- > > Resize and move work > -for types AText, AStamp and AGeom > -on all pages of document > -when viewport position changes > -when zoom level changes > -for all page rotations (0°, 90°, 180°, 270°) > > Selection is canceled > -when currently selected annotation is deleted > -on mouse click outside of currently selected annotation > -ESC is pressed > > Viewport is shifted when mouse cursor during move/resize comes close to > viewport border. > Resize to negative is prevented. > Tiny annotations are still selectable. > If mouse is moved over an annotation type that we can focus, and the > annotation is not yet focused, mouse cursor shape changes to arrow. > If mouse cursor rests over an annotation A, while annotation B is focused, a > tooltip for annotation A is shown. > > Test for regressions: > -Annotation interaction (focus, move, resize, start playback, ...) are only > done in mode EnumMouseMode::Browse. > -If mouse is moved over an annotation type where we can start an action, > mouse cursor shape changes to pointing hand. > -If mouse is moved over an annotation type that we can't interact with, mouse > cursor shape stays a open hand. > -If mouse cursor rests over an annotation of any type, a tooltip for that > annotation is shown. > -Grab/move scroll area (on left click + mouse move) is prevented, if mouse is > over focused annotation, or over AMovie/AScreen/AFileAttachment annotation. > -A double click on a annotation starts the "annotator". > > > Thanks, > > Tobias Deiminger > >
