Bug 304621 - [PATCH] Fix Tools->review inconsistency when switching tool
Summary: [PATCH] Fix Tools->review inconsistency when switching tool
Status: RESOLVED FIXED
Alias: None
Product: okular
Classification: Unclassified
Component: general (show other bugs)
Version: 0.15.0
Platform: Fedora RPMs Linux
: NOR normal (vote)
Target Milestone: ---
Assignee: Okular developers
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-08-05 14:55 UTC by Sandro Mani
Modified: 2012-08-27 19:01 UTC (History)
1 user (show)

See Also:
Latest Commit:
Version Fixed In: 4.9.1


Attachments
Patch (831 bytes, patch)
2012-08-05 14:56 UTC, Sandro Mani
Details
Updated patch (2.67 KB, patch)
2012-08-12 11:38 UTC, Sandro Mani
Details
Patch v3 (5.62 KB, patch)
2012-08-12 20:44 UTC, Sandro Mani
Details
Patch v4 (5.44 KB, patch)
2012-08-13 10:25 UTC, Sandro Mani
Details
Patch v5 (5.64 KB, patch)
2012-08-23 00:02 UTC, Sandro Mani
Details

Note You need to log in before you can comment on or make changes to this bug.
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