RFC: In annotations list show highlighted text
Needs ReviewPublic

Authored by jangmarker on May 23 2019, 11:26 PM.

Details

Reviewers
aacid
Group Reviewers
Okular
Summary

unfortunately the text detection is not 100 % accurate

Diff Detail

Repository
R223 Okular
Branch
annotations-show-comments-highlighted-text (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 12112
Build 12130: arc lint + arc unit
jangmarker created this revision.May 23 2019, 11:26 PM
Restricted Application added a project: Okular. · View Herald TranscriptMay 23 2019, 11:26 PM
Restricted Application added a subscriber: okular-devel. · View Herald Transcript
jangmarker requested review of this revision.May 23 2019, 11:26 PM
jangmarker added inline comments.
ui/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.

davidhurka edited reviewers, added: aacid; removed: davidhurka.May 25 2019, 12:30 PM
davidhurka added a subscriber: davidhurka.

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, and in T8533 in more general, but this patch is probably fine.
(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.

ui/guiutils.cpp
77

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

80

QString::simplified() is more predictable.

83

Why {}?

198

Move 5 lines up, so code stays uniform.

Your code:

Wouldn’t static_cast<>() be better than a C-style cast?

davidhurka added inline comments.May 26 2019, 10:54 AM
ui/guiutils.cpp
130

i18nc could be appropiate here, underline could be a verb as well.

davidhurka added inline comments.Jun 2 2019, 1:21 PM
ui/guiutils.cpp
77

As I learned recently, normalized coordinates are not necessarily normalized in rotation. We will need to test this with rotated view as well. (Or ignore it and then fix it together with Bug 334297.)

80

Well, it’s probably the same, because of the text reordering in TextPage. :)

This would help to fix/implement bug 377886. The missing part is tweaking the search field above the annotation tree view.

https://bugs.kde.org/show_bug.cgi?id=377886 Store text along with annotations like highlight, underline and squiggle

@jangmarker Are you still there, or did I scare you away?