Bug 205084

Please verify whether it works in KDE 4.3.x or not. Current trunk potentially has another issue which could be misleading you in this.

Good call, Pino! I have tracked down the problem to the changes introduced in revision 989285 (i.e. the switch to QAbstractScrollArea). In revision 989284 forward search works just fine. This stuff is a bit beyond my knowledge of Qt, though. Any hints what I could be looking for?

Cheers, Jochen

Okay, I might have found the problem. In PageView::slotRelayoutPages(), part 4) [line 3003ff in ui/pageview.cpp in trunk)] is only executed
if ( fullWidth != contentAreaWidth() || fullHeight != contentAreaHeight() )
Commenting out this condition makes forward search work. It seems as though part 4) is also needed when the size has not changed at all, but the document has been reloaded. Not sure about the correct condition to use, though. Any suggestion, Pino?

Now that this part works for me, I am encountering yet another problem. The pageview correctly changes to the requested viewport, but the coordinates that are being requested seem wrong (the normalizedY coordinate is typically very close to zero and negative!). I'll investigate this further and open another bug report once I know where this is coming from (should be something in the source anchor code).

Cheers, Jochen

How did further investigation ended up?

I can't reproduce the problem with the wrong normalizedY coordinate anymore. Maybe it got fixed somehow in the meantime.

So, to fix this bug for me, the if condition mentioned in comment #3 needs to be removed. The respective code needs to be executed also if the size hasn't changed but the document has been reloaded in the meantime. I don't know if there is an easy way to test for this rather than to remove the condition all together. Removing it doesn't seem to have any noticeable side effects on my system.

Could you please attach the necessary files to help me reproducing this?

Created attachment 39637 [details]
dvi file showing the issue, see below for instructions

Created attachment 39638 [details]
pdf file showing the issue, see beolw for instructions

Created attachment 39639 [details]
synctex data for test case

Created attachment 39641 [details]
synctex data for test case

Created attachment 39642 [details]
latex source for the above

Above is a somewhat minimal test case for this bug. The dvi file can be tested stand-alone, the pdf file and the synctex.gz file need to be in the same folder to test. The .tex file is for your info and not strictly needed to test this.

Run

okular --unique test.{dvi|pdf}#src:33test.tex

and the file should open roughly in the middle of page 3. Follow up with

okular --unique test.{dvi|pdf}#src:57test.tex

and you will see the view changing to page 1 with the page indicator flipping to page 5 (as it should). The problem is that the viewport does not get updated correctly (see comment #3 and comment #5).

The file should be self-explanatory once you see it, so you will be able to create more sophisticated tests with it.

Incidentally, it may be that bug #199184 is essentially the same bug since the workaround proposed there (press Ctrl+F triggering the search bar which also updates the viewport) works here as well. The reporter of that bug doesn't mention source references, though. Maybe the problem occurs as well if the dvi/pdf file itself gets changed while viewing it (I can not trigger that on my system, though)? Or the reporter of bug #199184 neglected to mention forward search?

Can you check if this fixes the problem for you?

Index: ui/pageview.cpp
===================================================================
--- ui/pageview.cpp     (revision 1070532)
+++ ui/pageview.cpp     (working copy)
@@ -752,12 +752,15 @@

     // invalidate layout so relayout/repaint will happen on next viewport change
     if ( haspages )
+    {
         // TODO for Enrico: Check if doing always the slotRelayoutPages() is not
         // suboptimal in some cases, i'd say it is not but a recheck will not hurt
         // Need slotRelayoutPages() here instead of d->dirtyLayout = true
         // because opening a document from another document will not trigger a viewportchange
         // so pages are never relayouted
         QMetaObject::invokeMethod(this, "slotRelayoutPages", Qt::QueuedConnection);
+        d->dirtyLayout = true;
+    }
     else
     {
         // update the mouse cursor when closing because we may have close through a link and

Yes, with this change the pdf case works perfectly.

In the dvi case, the viewport now gets updated to the correct page, however, the vertical position is still wrong. It always shows an area near the top of the page. I guess this is the aformentioned problem with normalizedY in the dvi case (see end of comment #3). This appears to be a different bug, though, probably to do with how normalizedY gets calculated in the dvi case vs the pdf case. When I tried to track this down before, I had the impression that this may have to do with order of execution. By the time normalizedY gets computed in the dvi source anchor code, the viewport dimensions are not correctly set, yet. Or something like that. Do you want me to open a separate bug for this or do you want to handle it in this report since it is related to the same changes that triggered this bug (i.e. it used to work in rev. 989284)?

That sounds like a different bug whose fix would be

Index: generator_dvi.cpp
===================================================================
--- generator_dvi.cpp   (revision 1071422)
+++ generator_dvi.cpp   (working copy)
@@ -156,9 +156,9 @@

     SimplePageSize ps = m_dviRenderer->sizeOfPage( vp.pageNumber );
     double resolution = 0;
+    if (ps.isValid()) resolution = (double)(pW)/ps.width().getLength_in_inch();
+    else resolution = m_resolution;

-    resolution = (double)(pW)/ps.width().getLength_in_inch();
-
     double py = (double)anch.distance_from_top.getLength_in_inch()*resolution + 0.5;

     vp.rePos.normalizedX = 0.5;

Works for you?

Yes, indeed, that fixes the dvi case. And even better, it led me to improve the pdf case a bit as well:

--- generator_pdf.cpp.orig      2010-01-08 12:29:07.000000000 +1100
+++ generator_pdf.cpp   2010-01-08 14:05:21.000000000 +1100
@@ -1541,11 +1541,12 @@

             if ( !viewport.isValid() ) return;

-            // TeX small points ...
-            viewport.rePos.normalizedX = (synctex_node_h( node ) * dpiX) / (72.27 * 65536.0 * document()->page(viewport.pageNumber)->width());
-            viewport.rePos.normalizedY = (synctex_node_v( node ) * dpiY) / (72.27 * 65536.0 * document()->page(viewport.pageNumber)->height());
-            viewport.rePos.enabled = true;
-            viewport.rePos.pos = Okular::DocumentViewport::TopLeft;
+           double px = (synctex_node_visible_h( node ) * dpiX) / 72.27;
+            double py = (synctex_node_visible_v( node ) * dpiY) / 72.27;
+            viewport.rePos.normalizedX = px / document()->page(viewport.pageNumber)->width();
+            viewport.rePos.normalizedY = ( py + 0.5 ) / document()->page(viewport.pageNumber)->height();
+           viewport.rePos.enabled = true;
+            viewport.rePos.pos = Okular::DocumentViewport::Center;

             return;
         }

With this change the target gets positioned more accurately within the center of the current view which makes finding it easier in cluttered pages. The trick is the "+ 0.5" in the correct units. Don't ask me why but the dvi case does this as well ...

So, with the changes from comment #13, comment #15 and this comment committed the bug can be finally closed as fixed IMHO.

SVN commit 1072172 by aacid:

Make sure resolution is not inf
Fixes part of bug 205084
CCBUG: 205084


 M  +2 -2      generator_dvi.cpp  


WebSVN link: http://websvn.kde.org/?view=rev&revision=1072172

SVN commit 1072174 by aacid:

backport r1072172 | aacid | 2010-01-09 15:08:53 +0000 (Sat, 09 Jan 2010) | 4 lines

Make sure resolution is not inf
Fixes part of bug 205084
CCBUG: 205084


 M  +2 -2      generator_dvi.cpp  


WebSVN link: http://websvn.kde.org/?view=rev&revision=1072174

SVN commit 1072187 by aacid:

rename delayResizeTimer to delayRelayoutTimer and always create it, not on first resizeEvent
Use the timer for the delayed slotRelayoutPages call in notifySetup instead of a QueuedConnection
Also set the dirtyFlag in notifySetup, that way if we get a set viewport very soon we still end up in the correct place
Stop the timer if we get to slotRelayoutPages so we never relayout twice without a need to
This should fix bug 205084
BUGS: 205084


 M  +16 -15    pageview.cpp  


WebSVN link: http://websvn.kde.org/?view=rev&revision=1072187

SVN commit 1072188 by aacid:

backport r1072187 | aacid | 2010-01-09 15:37:38 +0000 (Sat, 09 Jan 2010) | 7 lines

rename delayResizeTimer to delayRelayoutTimer and always create it, not on first resizeEvent
Use the timer for the delayed slotRelayoutPages call in notifySetup instead of a QueuedConnection
Also set the dirtyFlag in notifySetup, that way if we get a set viewport very soon we still end up in the correct place
Stop the timer if we get to slotRelayoutPages so we never relayout twice without a need to
This should fix bug 205084
BUGS: 205084


 M  +16 -15    pageview.cpp  


WebSVN link: http://websvn.kde.org/?view=rev&revision=1072188

SVN commit 1072189 by aacid:

commit improvement to the location of synctex thingies by Jochen Trumpf
CCBUG: 205084


 M  +5 -3      generator_pdf.cpp  


WebSVN link: http://websvn.kde.org/?view=rev&revision=1072189

SVN commit 1072191 by aacid:

backport r1072189 | aacid | 2010-01-09 15:42:45 +0000 (Sat, 09 Jan 2010) | 3 lines

commit improvement to the location of synctex thingies by Jochen Trumpf
CCBUG: 205084


 M  +5 -3      generator_pdf.cpp  


WebSVN link: http://websvn.kde.org/?view=rev&revision=1072191