Bug 304621

Summary:	[PATCH] Fix Tools->review inconsistency when switching tool
Product:	[Applications] okular	Reporter:	Sandro Mani <manisandro>
Component:	general	Assignee:	Okular developers <okular-devel>
Status:	RESOLVED FIXED
Severity:	normal	CC:	aacid
Priority:	NOR
Version:	0.15.0
Target Milestone:	---
Platform:	Fedora RPMs
OS:	Linux
Latest Commit:	http://commits.kde.org/okular/56dd5278d29d117ac9ee04faabea46d7599ecfe7	Version Fixed In:	4.9.1
Attachments:	Patch Updated patch Patch v3 Patch v4 Patch v5

Description Sandro Mani 2012-08-05 14:55:39 UTC

Version:
okular-4.9.0-1.fc18.x86_64

Reproducible: Always

Steps to Reproduce:
1. Open a pdf
2. Show review toolbar (Tools -> Review)
3. Change to text selection tool
4. Review toolbar is hidden, but togglebutton remains checked



Patch attached.

Comment 1 Sandro Mani 2012-08-05 14:56:19 UTC

Created attachment 72972 [details]
Patch

Patch ensures Review togglebutton remains consistent when changing tool.

Comment 2 Albert Astals Cid 2012-08-09 22:34:24 UTC

I'll have a look at the patch "soon"

Comment 3 Albert Astals Cid 2012-08-12 10:50:15 UTC

Hi Sandro, i see that PageView::slotSetMouseZoom and a few other methods seem to have the same construct, so they'd might need the same fix, can you check that?

Thanks!

Comment 4 Sandro Mani 2012-08-12 11:38:55 UTC

Created attachment 73112 [details]
Updated patch

Hello Albert, yes you are right, I've updated the patch accordingly.

Comment 5 Albert Astals Cid 2012-08-12 18:42:37 UTC

Back to me

Comment 6 Albert Astals Cid 2012-08-12 18:49:35 UTC

This breaks one of the features that was that if you were with the review toolbar shown, switched to Selection Tool, and then switched to Browse Tool the review toolbar was shown again, as noted in 

void PageView::slotSetMouseNormal()
{
    ...
    // reshow the annotator toolbar if hiding was forced
    ...
}

This is now broken because it wrongly depended in d->aToggleAnnotator->isChecked(), can you please add some code to your patch so that this feature still works?

Thanks :-)

Comment 7 Sandro Mani 2012-08-12 20:44:46 UTC

Created attachment 73128 [details]
Patch v3

I also added a missing
delete d->annotator
in the PageView destructor.

Comment 8 Albert Astals Cid 2012-08-12 23:23:20 UTC

There's no need to delete the d->annotator, QObject will take care of that.

The rest of the patch seems ok barring spacing and code in the header.

I'll have a look probably tomorrow

Comment 9 Sandro Mani 2012-08-13 10:25:12 UTC

Created attachment 73135 [details]
Patch v4

Whoops, missed the QObject inherticance. Removed the delete statement and added the missing space.

Comment 10 Albert Astals Cid 2012-08-18 17:11:54 UTC

There's still a problem if i do this:
 * Ctrl+3
 * F6
 * Ctrl+3
 * F6

In this situation i'm seeing the review tool bar but the menu is unchecked. Can you have a look?

Comment 11 Sandro Mani 2012-08-19 23:30:27 UTC

Will have a look tomorrow or tuesday.

Comment 12 Sandro Mani 2012-08-23 00:02:07 UTC

Created attachment 73406 [details]
Patch v5

Ok, should really work correctly now...

Comment 13 Albert Astals Cid 2012-08-27 19:00:51 UTC

Git commit a2dd088ebd5511477d0bc6dec6b407886f915772 by Albert Astals Cid, on behalf of Sandro Mani.
Committed on 27/08/2012 at 20:59.
Pushed by aacid into branch 'KDE/4.9'.

Fix Tools->review inconsistency when switching tool
FIXED-IN: 4.9.1

M  +24   -11   ui/pageview.cpp
M  +11   -1    ui/pageviewannotator.cpp
M  +4    -0    ui/pageviewannotator.h

http://commits.kde.org/okular/a2dd088ebd5511477d0bc6dec6b407886f915772

Comment 14 Albert Astals Cid 2012-08-27 19:01:28 UTC

Git commit 56dd5278d29d117ac9ee04faabea46d7599ecfe7 by Albert Astals Cid, on behalf of Sandro Mani.
Committed on 27/08/2012 at 20:59.
Pushed by aacid into branch 'master'.

Fix Tools->review inconsistency when switching tool
FIXED-IN: 4.9.1
(cherry picked from commit a2dd088ebd5511477d0bc6dec6b407886f915772)

M  +24   -11   ui/pageview.cpp
M  +11   -1    ui/pageviewannotator.cpp
M  +4    -0    ui/pageviewannotator.h

http://commits.kde.org/okular/56dd5278d29d117ac9ee04faabea46d7599ecfe7