davidhurka edited reviewers, added: aacid; removed: davidhurka. davidhurka added a comment.
You are not touching “Highlight with Comment”-style texts, which is probably good. (And would fit in another commit anyway.) There was a discussion about this in D10797 <https://phabricator.kde.org/D10797>, and in T8533 <https://phabricator.kde.org/T8533> in more general, but this patch is probably fine. (T8533 <https://phabricator.kde.org/T8533> is why I thought the annotations would have been y-ordered.) Your code: - You use `auto` some times. I think that’s only good if the type doesn’t matter or is too long to write it twice on the same line. NormalizedPoint and HighlightAnnotation::Quad will make it more readable. - You use braces for switch cases. Didn’t see that before. For consistency with the rest of Okular: remove them? - Bonus: If you create new functions or types, or modify functions you understand, could you write documentation for them? I’m also wondering whether the Annotation specific members of GuiUtils shouldn’t be members of Annotation. Didn’t know HighlightAnnotation::Quad, will add that to my documentation todo list. Probably important for Bug 334297. INLINE COMMENTS > jangmarker wrote in guiutils.cpp:77 > I'd be grateful for any ideas on how to make this more accurate. Sometimes it > captures text of the previous or next line, sometimes it just misses some > text. If you use TextPage::textArea( textSelction ), you will get something like (using above source lines as example...): xtSelection(leftTop, rightDown);// TODO this does not seem if only seem was highlighted, right? That’s because textArea() is this monster function which implements text selection for Text Selection mode. Did you try to make a RegularAreaRect from all NormalizedRect from leftTop and rightDown of each Quad, and pass that to text()? > guiutils.cpp:80 > + } > + return text.replace('\n', ""); > + } QString::simplified() is more predictable. > guiutils.cpp:83 > + > + return QString{}; > +} Why `{}`? > guiutils.cpp:198 > > + QString highlightedText = highlightedTextForAnnotation(ann, page); > + if ( !highlightedText.isEmpty() ) Move 5 lines up, so code stays uniform. REPOSITORY R223 Okular REVISION DETAIL https://phabricator.kde.org/D21376 To: jangmarker, #okular, aacid Cc: davidhurka, okular-devel, joaonetto, tfella, ngraham, darcyshen, aacid
