[Okular-devel] Review Request 111829: Use DPI of current screen for PDF rendering

Thu Aug 29 21:28:56 UTC 2013

> On Aug. 25, 2013, 4:13 p.m., Albert Astals Cid wrote:
> > Ok, the code looks "sane" but there are two things that still make me unsure about this whole thing:
> > 
> >  * Why you need Pixels?
> >   In the bug you say "Points, but actually it is pixels, DIVIDED by 72."
> >   That is not true, the page size of a PDF is defined in points (well it's actually defined in UserUnits but that defaults to the Point value)
> >   Why do you say it's  pixels DIVIDED by 72?
> >   I'd be happier if we could still have the PDF backend return stuff in points and have all the dpi conversion magic in the layers above it
> > 
> >  * I'm still undecided as of why we need the widget realDpi. Reasons:
> >   As far as I know (though my knowledge may not be very good :D) all the systems force you to have the same DPI in all the monitors, so having to pass the widget seems "nice it's going to do magic" when I move it to another monitor, but that's really not happening no? Or at least if there was a system in which you could have different screens with different monitors, we miss something so that when you move it from one monitor to another the dpi gets recalculated no?
> > 
> > I know this may sound like I'm stalling the patch, but it is really not, I just want to make sure we end up with a patch that we're all happy and convinced with.
> 
> Eugene Shalygin wrote:
>     > * Why you need Pixels?
>     Regarding the "pixels divided..." I'm not sure now :). The story had been started with aim to get correct scale on screen, as you certainly remember. To my understanding, Page::width() and Page::height() are in screen pixels (at least it seems to me like this from reading of e.g. PageView::updateItemSize()). If this is correct, PageView class should handle screen DPI, and all generators should set page size in points (or in something, that has a dimension). Will we go this way? 
>     
>     
>     > * I'm still undecided as of why we need the widget realDpi.
>     I'm afraid I do not understand your comment completetly. I'll answwer on question that I understood. 
>     1. The system can not "force" us to have the same DPI in all monitors, since the system can not change physical DPI of the screen (at least in the Universe #3 :D). 
>     2. We use widget object to get the monitor in which the display the pages (and its current DPI).
>     
>
> 
> Albert Astals Cid wrote:
>     I'm going to be on holiday most of september, so unless someone else takes care of this review you'll have to wait until I come back.
> 
> Eugene Shalygin wrote:
>     Well, then just commit it right now ;)
>     Could you please then try to answer my question before going on holiday?

Should be learning some Japanese, but oh well, i'm already here.

> > * Why you need Pixels?
> Regarding the "pixels divided..." I'm not sure now :). The story had been started with aim to get correct scale on screen, as you certainly remember. To my understanding, Page::width() and Page::height() are in screen
> pixels (at least it seems to me like this from reading of e.g. PageView::updateItemSize()). If this is correct, PageView class should handle screen DPI, and all generators should set page size in points (or in something, > that has a dimension). Will we go this way? 
Well, i'm undeccided on this one, i'd like to share as much dpi<->pixel conversion code, and putting it outside the generator helps this, but of course it means we have to change more *core* code that's also not so great, so well think if you could make it so some of the code that does dpi<->pixel was shared a bit more up the chain. About the other generators, they don't return "Points" so we wouldn't need to change them back and forth

> > * I'm still undecided as of why we need the widget realDpi.
> I'm afraid I do not understand your comment completetly. I'll answwer on question that I understood. 
> 1. The system can not "force" us to have the same DPI in all monitors, since the system can not change physical DPI of the screen (at least in the Universe #3 :D). 
It is, but as far as I've been told X only has one DPI setting so...
> 2. We use widget object to get the monitor in which the display the pages (and its current DPI).
... are you sure about this? Everyone I asked as told me that if I have a two screen desktop shared across two monitors with different DPI, I'll get the same DPI (one of the two monitors) when querying on both, bsically because if this did not happen how would you render something that is shown on both screens because it's "in the middle"?

- Albert

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

On Aug. 21, 2013, 12:56 a.m., Eugene Shalygin wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/111829/
> -----------------------------------------------------------
> 
> (Updated Aug. 21, 2013, 12:56 a.m.)
> 
> 
> Review request for Okular and Albert Astals Cid.
> 
> 
> Description
> -------
> 
> This patch relies on master branch of LibKScreen.
> This patch does not solve the problem in bug completely, but makes Okular behaviour more correct (see below).
> 
> The problem (in the bug) is that Okular uses fixed DPI for PDF rendering (72.0 dots per inch) and therefore scale of rendered document becomes incorrect. With current mainline laptop screens having DPI easily twice larger than this constant (72), the problem shows itself quiet strongly.
> 
> Additional problems araise with multiscreen configuration, when 1) DPI of each individual screen may be different, and 2) there is no tools in Qt to detect DPI of individual screens in virtual desktop mode. Therefore XRandr has to be used for DPI detection. Raw XRandr is bad dependency for Okular and libkscreen is proposed instead.
> 
> This patch approach to the solution in the following way:
> 1. libkscreen detection staff is added to CMakeLists.txt and config.h
> 2. It changes Utils::realDpi() function to use libkscreen if present. With libkscreen the function looks for output that contains maximal part of given widget (suppose to be used for document rendering) and returns DPI of that screen.
> 3. Genenerator interface is extended by adding UtilizeDPI feature and setDPI() method, that is called by Document class right before calling loadXXX() if the generator supports this feature
> 4. Poppler generator is changed to support UtilizeDPI feature.
> 5. PageSizeMetric enum is extended with Pixels value to explicitly say that page size is defined in screen pixels (see my posts in the bug); Poppler generator uses this case.
> 
> 
> To completetly fix the bug, Document must invalidate generated pixmaps after the widget movements into another screen. I do not know how to track this without subclassing the main window class. Therefore I decided to publish this part of work to get your responce, especially regarding item 3 (Generator class changes).
> 
> In the current state, manual reloading of a document after moving Okular to another screen fixes the scale, that is, in my eyes, is quiet helpful already.
> 
> Even if we subclass the Okular main window, I do not know what to do when Okular is used as KPart. 
> 
> 
> This addresses bug 268757.
>     http://bugs.kde.org/show_bug.cgi?id=268757
> 
> 
> Diffs
> -----
> 
>   CMakeLists.txt 217337f 
>   config-okular.h.cmake 7217f8d 
>   core/document.cpp 3af92c8 
>   core/generator.h a5a971b 
>   core/generator.cpp 41beb92 
>   core/generator_p.h b59293a 
>   core/utils.h 8d5d5fc 
>   core/utils.cpp 5dd8448 
>   generators/poppler/generator_pdf.h 5d5853a 
>   generators/poppler/generator_pdf.cpp 1a44523 
> 
> Diff: http://git.reviewboard.kde.org/r/111829/diff/
> 
> 
> Testing
> -------
> 
> Manual. In all screens, that report correct physical size to XRandr, size of documents is correct
> 
> 
> Thanks,
> 
> Eugene Shalygin
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/okular-devel/attachments/20130829/c5b23023/attachment-0001.html>