[Okular-devel] Review Request: Merge request for 'viewerinterface' branch

Tue Nov 1 08:22:02 UTC 2011

> On Oct. 31, 2011, 4:25 p.m., Albert Astals Cid wrote:
> > core/document.h, line 779
> > <http://git.reviewboard.kde.org/r/102946/diff/2/?file=39710#file39710line779>
> >
> >     We try to maintain source and binary compatibility in the okular core library. This is source compatible but unfortunately not binary compatible, do you really need to store the viewport type? This information does not really seem to belong to the viewport to me, a viewport is a given position into the view, how you got to create it should not be part of it in my opinion. Maybe you can store something like m_lastViewportChangeBecauseOfBlaBla in document and the use it on notifyviewport?
> 
> Michel Ludwig wrote:
>     I thought about adding a flag to the document which indicates that the last viewport results from a source location. But that becomes a bit messy, I think, as one has to take care that other code paths which also manipulate the viewport clear that flag.
>     
>     Hence, I came up with the idea to add a type to the viewport. I think it is justified as the source location viewports are somewhat special. :)
>     
>     What do you think of not modifying the constructor but simply adding a new method like 'setFromSourceLocation(bool)' to the viewport class? That should be binary compatible.
>     
>     Or maybe you have another suggestion?
> 
> Albert Astals Cid wrote:
>     Unfortunately adding a setFromSourceLocation is still not binary compatible since you are still growing the size of the class by adding the type member variable. 
>     
>     And really i am not convinced a source location is special at all, it is just a viewport in a page as all other viewports are.
>     
>     For example, we store viewports in the history, if you go back and forward in history, you probably do not care if it was a source location one or not, you only want to move though the document history points.
>     
>     Also you are setting SourceLocationViewport unconditionally in DocumentPrivate::nextDocumentViewport() and that is not not correct since a regular Link::GoTo will end up there too. Can you explain a bit more what you are trying to do with this flag? Maybe with you explaining it and I reading it we get to a better solution

Right, I forgot about the member variable.

Currently, SourceLocationViewport is only set if there was a named destination. The intention for that was that only source locations are named destinations, but I probably have missed some other cases.

The general idea was that the latest source location requested by the user is shown graphically in the displayed document. For that I added this flag to the DocumentViewport class. Then, if PageView::notifyViewportChanged is called on a viewport that corresponds to a source location, that location in the document is stored in order for it to be painted on the document.

So what we need is

(1) a way to uniquely identify source locations, which are probably named destinations starting with "src:" only, and
(2) a way to inform PageView::notifyViewportChanged that a source location has been activated so that it can store the coordinates.

> On Oct. 31, 2011, 4:25 p.m., Albert Astals Cid wrote:
> > part.cpp, line 2447
> > <http://git.reviewboard.kde.org/r/102946/diff/2/?file=39716#file39716line2447>
> >
> >     You sure we do not need this anymore? Doesn't seem much related to the general patch
> 
> Michel Ludwig wrote:
>     This was done twice before, I think. Once in 'unsetDummyMode()' and once in the constructor. Now, it's only done in the constructor (line 494).
> 
> Albert Astals Cid wrote:
>     And what about the other callers of slotNewConfig ?

Sorry, I don't follow. I only removed updateViewActions() from unsetDummyMode(), which is only called in the constructor of Part. And there, the call to unsetDummyMode() was followed by a call to updateViewActions() anyway. So, no overall change. Or am I missing something?

But before this discussion becomes too long, I can just as well add it back as it's not essential :)

- Michel

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/102946/#review7777
-----------------------------------------------------------

On Oct. 31, 2011, 9:55 p.m., Michel Ludwig wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/102946/
> -----------------------------------------------------------
> 
> (Updated Oct. 31, 2011, 9:55 p.m.)
> 
> 
> Review request for Okular.
> 
> 
> Description
> -------
> 
> The functionality that I need for Kile should now be implemented, i.e. handling of source references, disabling of certain actions and configuration options in 'ViewerWidgetMode', and drawing of source locations.
> 
> Note that currently the drawing of source locations doesn't work correctly for rotated pages. This is due to the fact that locating source references for rotated doesn't seem to be implemented in Okular yet. I'm thinking of disabling the rotate-page actions in 'ViewerWidgetMode' until this is implemented in Okular.
> 
> If you want to see the viewer mode in action, you can try it out by following the instructions given here:
> 
> http://sourceforge.net/apps/mediawiki/kile/index.php?title=Live_Preview
> 
> 
> Diffs
> -----
> 
>   CMakeLists.txt f8dcba0 
>   conf/dlggeneral.h 1ee2768 
>   conf/dlggeneral.cpp 80478e6 
>   conf/okular.kcfg b7d511c 
>   conf/preferencesdialog.h 72a7072 
>   conf/preferencesdialog.cpp 5ea6269 
>   core/document.h 2bcf280 
>   core/document.cpp a417828 
>   core/global.h 24cef77 
>   interfaces/viewerinterface.h PRE-CREATION 
>   part-viewermode.rc PRE-CREATION 
>   part.h f6203d7 
>   part.cpp 275bf4d 
>   ui/pagepainter.h 4044dd8 
>   ui/pagepainter.cpp 2bd2f23 
>   ui/pageview.h 9800c55 
>   ui/pageview.cpp 2a17369 
> 
> Diff: http://git.reviewboard.kde.org/r/102946/diff/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Michel Ludwig
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/okular-devel/attachments/20111101/71a13826/attachment.html>