Bug 334251 - Tools don't work if any tab is in review mode
Summary: Tools don't work if any tab is in review mode
Status: RESOLVED FIXED
Alias: None
Product: okular
Classification: Applications
Component: general (show other bugs)
Version: 0.19.0
Platform: Arch Linux Linux
: HI normal
Target Milestone: ---
Assignee: Okular developers
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-05-02 22:23 UTC by Jonathan Doman
Modified: 2014-12-10 14:34 UTC (History)
1 user (show)

See Also:
Latest Commit:
Version Fixed In: 14.12.0


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Doman 2014-05-02 22:23:04 UTC
The non-browse tools (zoom, select, etc.) don't work if any tab in the window is in review mode. Normally, a single tab will enforce the mutual exclusivity of review mode and non-browse tools. But there is some kind of interaction between tabs that causes all tools in all tabs to behave as a browse tool without properly reflecting this in the GUI. I'm happy to fix this but it may be over a month before I have time to do so.

Reproducible: Always

Steps to Reproduce:
1. Open two documents in tabs
2. Enter review mode in doc 1. (At this point, doc 1 will enforce the use of browse tool.)
3. Switch to doc 2 and try to use the zoom tool (or any other tool).
Actual Results:  
The zoom tool will be selected in the GUI and the corresponding tooltip will show, but in fact the browse tool is still active.

Expected Results:  
Each tab should be allowed to use different tools, regardless of whether other tabs are in review mode.
Comment 1 Albert Astals Cid 2014-05-03 17:59:11 UTC
:/ This is bad :/
Comment 2 Albert Astals Cid 2014-07-30 22:17:59 UTC
Jonathan any chance we can get a fix for this for 4.14.0 (tagging in one week)?
Comment 3 Jonathan Doman 2014-10-12 22:19:14 UTC
I took some time to look at this and I'm not sure how to fix it. The problem is that changing the mouse mode writes the global Settings, which triggers all the Parts to reload (and potentially overwrite) the Settings. We could:

1. Don't watch for the Settings::configChanged signal. This is my preference, as I wouldn't expect the program to reload settings whenever the config file is written to.

2. Don't constantly write settings to disk. Most Settings mutations are followed by a writeConfig(), which seems unnecessary to me. Why not just writeConfig() once when the program is exiting?

3. Remove MouseMode as a global setting. Why not just always start the program in Browse mode?

I would ultimately prefer to get rid of all persistent, implicit settings and make everything non-configurable or explicitly configurable because I don't think there's an intuitive way to resolve settings when multiple program instances are simultaneously altering them.
Comment 4 Albert Astals Cid 2014-10-13 19:31:32 UTC
1 + 2: Because it's what we use to pass around the settings change, you write the setting and then rely on slotNewConfig to set everything correctly, if you remove slotNewConfig and writeConfig you're going to need to rewrite lots of stuff.

3. Because people asked for that feature ;)

People is going to hate you if you remove the features they love and use ;)
Comment 5 Albert Astals Cid 2014-10-13 19:32:44 UTC
And yes i realize i didn't give you a solution, i guess you can always store the mouse mode in that struct you added per tab and set it from there when changing tabs?
Comment 6 Jonathan Doman 2014-10-14 05:39:38 UTC
So I tested an older version (tag v4.10.97) and this bug exists for multiple windows as well. It's probably been around forever. And there are similar bugs for other settings like continuous view mode. Basically I think the global Settings object is very inadequate for how it's used, and should be re-architected. I guess I'll keep trying to find a short term solution, though.
Comment 7 Albert Astals Cid 2014-12-10 14:34:45 UTC
Git commit b069e3557f995b3759a505160e3f009e2267506c by Albert Astals Cid, on behalf of Jonathan Doman.
Committed on 10/12/2014 at 14:33.
Pushed by aacid into branch 'Applications/14.12'.

Allow each PageView to use a different tool

Keep a local MouseMode setting, and don't rely on the value returned by Settings::mouseMode().
REVIEW: 120660
FIXED-IN: 14.12.0

M  +25   -17   ui/pageview.cpp

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