Bug 363776

Summary:	Text selection begins prematurely following change to tool
Product:	[Applications] okular	Reporter:	Jonathan <jonathan>
Component:	general	Assignee:	Okular developers <okular-devel>
Status:	RESOLVED FIXED
Severity:	minor	CC:	aacid, oliver.sander
Priority:	NOR
Version First Reported In:	unspecified
Target Milestone:	---
Platform:	unspecified
OS:	Linux
Latest Commit:	https://commits.kde.org/okular/d3b5e13dade487184d07d2c0c42848b7ae0268f3	Version Fixed In:
Sentry Crash Report:
Attachments:	PDF that demonstrates bug Screen capture video demonstrating bug Patch to fix problem Alberts patch to fix the problem

Description Jonathan 2016-06-01 08:11:20 UTC

When changing the mouse tool to text selection, okular sometimes behaves as though the mouse button is already pressed, forcing the user to click once to cancel and again to begin text selection.

Reproducible: Always

Steps to Reproduce:
1. Create an annotation if one does not already exist.
2. Select the browse tool
3. Right click on the annotation
4. Hit escape to cancel the pop-up menu.
5. Change to the text selection tool

Actual Results:  
Text selection begins immediately, as though a mouse button was held down.

Expected Results:  
Text selection should not begin until a mouse button is pressed at the start of the desired selection.

The PageView class loses track of its internal state. In part this is related to the fact that the annotation pop-up menu appears when the right mouse button is pressed, rather than when it is released, as all other pop-up menus do.

Comment 1 Oliver Sander 2017-02-14 05:33:57 UTC

I cannot confirm the "text selection begins immediately" part, neither with the okular from git master nor the one from Debian testing.

What I can confirm is that the annotation context menues really do appear on mouse-press rather than on mouse release.

Comment 2 Jonathan 2017-02-14 22:05:26 UTC

Created attachment 104034 [details]
PDF that demonstrates bug

Comment 3 Jonathan 2017-02-14 22:06:20 UTC

Created attachment 104035 [details]
Screen capture video demonstrating bug

Comment 4 Jonathan 2017-02-14 22:08:09 UTC

I just reproduced the problem once again using Okular from Debian/unstable. I've attached a copy of the PDF and a screen capture video to demonstrate. The important bit to notice is at 0:22 after I choose "Text Selection Tool" from the "Tools" menu. I then move the mouse across the text, which begins selecting even though I have not pressed the mouse button to begin selection.

Comment 5 Jonathan 2017-02-15 00:59:04 UTC

Created attachment 104037 [details]
Patch to fix problem

It's not very elegant, but this patch solves the problem. It does this by creating a function handleGenericRightButtonRelease, partly replacing handling right-click in different places in the code. It has the beneficial side-effect of resolving the inconsistent response to right-clicking while in text selection mode as described here: https://git.reviewboard.kde.org/r/127496/

Comment 6 Oliver Sander 2017-02-15 09:59:17 UTC

Now I can confirm the problem, both with the Debian version of okular and the one from git master.  My problem was

>  5. Change to the text selection tool

I was using the keyboard short-cut for this, and then there is no bug.  When I use the menu then text selection indeed begins immediately.

Comment 7 Albert Astals Cid 2017-03-07 23:32:03 UTC

Created attachment 104443 [details]
Alberts patch to fix the problem

This is a much simpler fix to the problem, i'll commit it in ~24 h if noone disagrees.

About the other review you posted, shit it's been sitting there for almost a year. Someone should really have a look at it.

Comment 8 Jonathan 2017-03-07 23:43:23 UTC

Comment on attachment 104443 [details]
Alberts patch to fix the problem

Sure that is a simple and effective solution to this bug. However it does nothing for two identified inconsistencies: 1. annotation context menus appearing on mouse-press instead of mouse-release; and 2. differing responses in different modes for no good reason to right-click.

Comment 9 Albert Astals Cid 2017-03-08 23:38:04 UTC

Git commit d3b5e13dade487184d07d2c0c42848b7ae0268f3 by Albert Astals Cid.
Committed on 08/03/2017 at 23:38.
Pushed by aacid into branch 'master'.

Reset d->mousePressPos correctly in mousePress after popup exec

M  +3    -0    ui/pageview.cpp

https://commits.kde.org/okular/d3b5e13dade487184d07d2c0c42848b7ae0268f3

Comment 10 Albert Astals Cid 2017-03-08 23:38:52 UTC

(In reply to Jonathan from comment #8)
> Comment on attachment 104443 [details]
> Alberts patch to fix the problem
> 
> Sure that is a simple and effective solution to this bug. However it does
> nothing for two identified inconsistencies: 1. annotation context menus
> appearing on mouse-press instead of mouse-release; and 2. differing
> responses in different modes for no good reason to right-click.

Those are possibly valid issues but unrelated to the "Text selection begins prematurely followint change to tool" bug.

Comment 11 Jonathan 2017-03-09 00:24:46 UTC

I beg to differ. The failure to set d->mousePressPos is a direct consequence of handling this particular click differently from every other one. So although the patch removes one obvious malfunction, it is one more step down the road of inconsistent coding and function in pageview.