Sadly Tobias Deiminger that did the original patch is not on phabricator yet so i can't add him for reviews.

Anyone knows why he would have thought of requiring a handle for showing the tooltip?

Sorry for the regression! I don't have a development environment for okular atm., the following is only from top of my head...

Anyone knows why he would have thought of requiring a handle for showing the tooltip?

MouseAnnotation can be in a state where an annotation has been selected by left-click, but then the mouse went further somewhere else on the document - maybe even over another yet unselected annotation. We do not want to show a tooltip for the selected annotation (m_focusedAnnotation), but for that one where the cursor is actually over (m_mouseOverAnnotation). To distinct those cases, MouseAnnotation can internally look at m_handle. m_handle tracks the current position of the mouse while it represents the different functional areas of an annotation under the cursor (upper left corner, upper left corner, inside, nowhere...).

bool MouseAnnotation::isMouseOver() const
{
    return ( m_mouseOverAnnotation.isValid() || m_handle != RH_None );
}

IIRC, isValid() and m_handle != RH_None are not exactly equal. The latter condition is true if the cursor is inside the imaginary surrounding rectangle, while isValid() is only true if mouse is over actual graphics. That makes a difference when you have an annotation like a circle, where only the border is drawn, and the inner area is not part of the annotation.

It seems good to remove the lines as you're doing it with your patch, as they seem to be redundant. We do the same check right before in pageView.cpp by calling MouseAnnotation::isMouseOver():

if ( d->mouseAnnotation->isMouseOver() )
{
    d->mouseAnnotation->routeTooltipEvent( he );
}

Does removing the lines already fix the bug? I'm not sure if it can, it removes redundancy and makes the code cleaner - but does it also change effective behavior?

Does removing the lines already fix the bug? I'm not sure if it can, it removes redundancy and makes the code cleaner - but does it also change effective behavior?

Yes, it fixes the issue, otherwise i would not have proposed the patch saying it fixes the issue 😄

I would really appreciate if you didn't unnecessarily change my summary.

Sorry Albert. I was under the impression that only "BUG: [number]" worked. I didn't know that "BUGS:" worked too. Is that used to indicate multiple bugs?

~/okular:master$ git log | grep BUGS: | wc -l
258

;)

As far as i know it's just a synonym

In D7874#149711, @rkflx wrote:

I can confirm the fix and was not able to find any regressions (found an unrelated bug, though).

Sorry for not noticing this earlier. Maybe you'd interest more reviewers in the future if you followed standard Phab practices, i.e. set some (group or normal) reviewers and provide a test plan.

It gets sent to the okular-devel mailing list automatically, that's where i expect people that care about okular reviews to be :)

For the people that will land up here later: it seems that both are valid (BUG and BUGS), see line 422 here:
https://websvn.kde.org/trunk/kde-common/svn/hooks/post-commit.pl?revision=1446260&view=markup

About the reviewers, you don't need to set the reviewer if the automated Herald rule sets a mailing list to the subscribers list. The mail will land in the proper place, despite what Phabricator says.
And even if having a reviewer is preferred, then the rules should be updated rather than adding explicit reviewers IMHO.

For me it's about signalling what the submitter want's to be done with the review:

• no reviewer: no review wanted, it's more like a WIP
• group reviewer (could automatically default to Okular): everybody interested should review
• explicit reviewer: special expertise wanted

As for the okular-devel: I'm interested in all of KDE's projects, but I won't and can't subscribe to every single mailinglist out there (I may look at the list archive occasionally when I'm focussing on an application like in the last weeks). If you want your reviewers coming mainly from the mailing list, I guess you have enough reviewers already (didn't look like it to me, though).

Anyway, it's not for me to interfere with the inner workings of the Okular project, this was merely a suggestion with only the best intentions in mind.

In D7874#150708, @rkflx wrote:

For me it's about signalling what the submitter want's to be done with the review:

• no reviewer: no review wanted, it's more like a WIP
• group reviewer (could automatically default to Okular): everybody interested should review
• explicit reviewer: special expertise wanted

Please note that I was not talking specifically about Okular, but about the way the Herald rules are set up globally here on phabricator.kde.org.
So your expectation does not match the general expectation. What I wrote is valid for Frameworks, for Plasma, etc.
The expectation is that you don't need to add a reviewer if the automatic rule adds as *subscriber* a mailing list (where the review is delivered) or a project (which people can join and receive the notification).
If you want a special person, sure, you can add that person as reviewer, but again what you expect for "group reviewer" is already fulfilled by groups in the subscriber field.

Anyway, it's not for me to interfere with the inner workings of the Okular project, this was merely a suggestion with only the best intentions in mind.

Again, consider that the above indication is for all phabricator.kde.org.
If you want it to be more in the way you think it should (so using explicit reviewers always), then the automatic rules which add subscribers should be changed (and you can propose this for example on kde-community@, opening a sysadming ticket for reference), but otherwise please follow the general expectation.

Fair enough, let's keep it as is for now (this Diff is the wrong place and I have no time resources to pursue this currently anyway, but maybe later). FWIW, my comment was rooted in those observations:

• Phab shows an annoying warning if no reviewer is set
• in Phab's listings you cannot see whether a Diff has a project or list subscriber added without opening every single Diff
• the wiki states "If you upload a revision with no reviewers, nobody will get notified."
• only a small minority of Diffs actually has "None" as reviewer, making this one seem like an accidental omission