> On Aug. 25, 2013, 6: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? > > Albert Astals Cid wrote: > 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"?
First of all, thanks for your time! > Well, i'm undeccided on this one, i'd like to share as much dpi<->pixel > conversion code,... so well think if you could make it so some of the code > that does dpi<->pixel.. OK. Maybe it would be better to to this separaterly (in separate review request)? I can work on that. But it would be simplier having Utils::realDpi() landed. So either you accept this review and then the next one, or this one grows with unrelated to the primary subject stuff. And, naturally, we can extract Utils + LibKScreen part from this one and create 3 reviews (realDPI, PageView, Generator). >It is, but as far as I've been told X only has one DPI setting so... > .. are you sure about this? That is why we ask RandR (via LibKScreen) about _physical_ size of the screens (in millimetres) and their resolution to calculate their (possibly different) DPIs. X, indeed, returns only single value of DPI for virtual screens. - Eugene ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111829/#review38556 ----------------------------------------------------------- On Aug. 21, 2013, 2: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, 2: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 > >
_______________________________________________ Okular-devel mailing list Okular-devel@kde.org https://mail.kde.org/mailman/listinfo/okular-devel