D6268: HiDPI Support for Okular

hetzenecker abandoned this revision. REPOSITORY R223 Okular REVISION DETAIL https://phabricator.kde.org/D6268 To: hetzenecker, davidedmundson, aacid Cc: ltoscano, gladhorn, ngraham, rkflx, sander, anthonyfieroni, #okular, aacid

D6268: HiDPI Support for Okular

hetzenecker reclaimed this revision. REPOSITORY R223 Okular REVISION DETAIL https://phabricator.kde.org/D6268 To: hetzenecker, davidedmundson, aacid Cc: ltoscano, gladhorn, ngraham, rkflx, sander, anthonyfieroni, #okular, aacid

D6268: HiDPI Support for Okular

aacid added a comment. Next time please mark it correctly as submitted not as abandoned. REPOSITORY R223 Okular REVISION DETAIL https://phabricator.kde.org/D6268 To: hetzenecker, davidedmundson, aacid Cc: ltoscano, gladhorn, ngraham, rkflx, sander, anthonyfieroni, #okular, aacid

D6268: HiDPI Support for Okular

hetzenecker abandoned this revision. hetzenecker added a comment. Thanks for all your comments! This was merged with commit https://phabricator.kde.org/R223:ecc1141e0293e1a30e0f8787d86dcc6309ffb0c0 ( https://cgit.kde.org/okular.git/commit/ui?id=ecc1141e0293e1a30e0f8787d86dcc6309ffb0c0 )

D6268: HiDPI Support for Okular

ngraham added a comment. Gotcha. I've manually closed the bugs. REPOSITORY R223 Okular REVISION DETAIL https://phabricator.kde.org/D6268 To: hetzenecker, davidedmundson, aacid Cc: ltoscano, gladhorn, ngraham, rkflx, sander, anthonyfieroni, #okular, aacid

D6268: HiDPI Support for Okular

ltoscano added a comment. Closing this won't close the bugs. Unfortunately the syntax used for the committed change is incorrect: - two different BUG: lines should have been used - REVIEW is for reviewboard. The syntax for Phabricator is different. Too late now. The bugs should

D6268: HiDPI Support for Okular

ngraham added a comment. Nice! Can we close this out so the bugs mentioned in the Summary get updated and marked as resolved? REPOSITORY R223 Okular REVISION DETAIL https://phabricator.kde.org/D6268 To: hetzenecker, davidedmundson, aacid Cc: gladhorn, ngraham, rkflx, sander, anthon

D6268: HiDPI Support for Okular

ngraham edited the summary of this revision. REPOSITORY R223 Okular REVISION DETAIL https://phabricator.kde.org/D6268 To: hetzenecker, davidedmundson, aacid Cc: gladhorn, ngraham, rkflx, sander, anthonyfieroni, #okular, aacid

D6268: HiDPI Support for Okular

davidedmundson accepted this revision. REPOSITORY R223 Okular REVISION DETAIL https://phabricator.kde.org/D6268 To: hetzenecker, davidedmundson, aacid Cc: gladhorn, ngraham, rkflx, sander, anthonyfieroni, #okular, aacid

D6268: HiDPI Support for Okular

gladhorn added a comment. I started running with this branch, and it enables me to use Okular again, without this the rendering is so blurry that I switched to another PDF viewer. So thanks a lot for all the work that went into this :) REPOSITORY R223 Okular REVISION DETAIL https://phab

D6268: HiDPI Support for Okular

hetzenecker added a comment. I just had a look at it.. and no, sorry, I didn't have enough time to spot the error. Guess you are on your own :/ REPOSITORY R223 Okular REVISION DETAIL https://phabricator.kde.org/D6268 To: hetzenecker, davidedmundson, aacid Cc: ngraham, rkflx, sander, ant

D6268: HiDPI Support for Okular

sander added a comment. Enjoy! I may have take on at https://bugs.kde.org/show_bug.cgi?id=384143 myself. Any ideas on where to start looking? REPOSITORY R223 Okular REVISION DETAIL https://phabricator.kde.org/D6268 To: hetzenecker, davidedmundson, aacid Cc: ngraham, rkflx, sander,

D6268: HiDPI Support for Okular

hetzenecker added a comment. I am going on a journey this month - this unfortunately means I also won't have access to the internet. It'd pick up work on this patch after I return though, if nobody else already has ;) REPOSITORY R223 Okular REVISION DETAIL https://phabricator.kde.org/

D6268: HiDPI Support for Okular

sander added a comment. The problem with drawing in presentation mode is now a bug tracker issue: https://bugs.kde.org/show_bug.cgi?id=384143 REPOSITORY R223 Okular REVISION DETAIL https://phabricator.kde.org/D6268 To: hetzenecker, davidedmundson, aacid Cc: ngraham, rkflx, sander, a

D6268: HiDPI Support for Okular

hetzenecker added inline comments. INLINE COMMENTS > widgetconfigurationtoolsbase.cpp:29 > m_list = new QListWidget( this ); > -m_list->setIconSize( QSize( 64, 64 ) ); > +m_list->setIconSize( QSize( 32, 32 ) ); > hBoxLayout->addWidget( m_list ); The size for the pixmaps is hard

D6268: HiDPI Support for Okular

hetzenecker added a comment. Oh, and before I forget: TIFF files are generally broken, there is a bug report for that: https://bugs.kde.org/show_bug.cgi?id=371828 (noticed that after endless hours of debugging ;) ) REPOSITORY R223 Okular REVISION DETAIL https://phabricator.kde.org/D62

D6268: HiDPI Support for Okular

hetzenecker updated this revision to Diff 18871. hetzenecker added a comment. Adressed raised problems by sander and David: - fix annotation icon size in settings dialog (i don't know how this could have ever worked ;) ) - removed redundant else branch REPOSITORY R223 Okular CHANGES

D6268: HiDPI Support for Okular

rkflx added a comment. Regarding the annotation toolbar (BUG 383589): Icons looking much better now, thanks! Two observations, though: - The rounded top and bottom corners are still pixelated (the handle probably too). - In Settings > Configure Okular > Annotations the annotation tool

D6268: HiDPI Support for Okular

davidedmundson added a comment. Oh I didn't mean it like that. Your input has been really really valuable. I was just trying to work out if we should block this and get it fixed, or try to merge this and then work on any remaining improvements. REPOSITORY R223 Okular REVISION DETAIL

D6268: HiDPI Support for Okular

sander added a comment. Another one is https://bugs.kde.org/show_bug.cgi?id=383943 But as noted there, it is not a regression, and should be handled separately. REPOSITORY R223 Okular REVISION DETAIL https://phabricator.kde.org/D6268 To: hetzenecker, davidedmundson, aacid Cc:

D6268: HiDPI Support for Okular

sander added a comment. Midair collision. I'm not saying it is not good to go. I am just saying that there are more HiDPI issues left. REPOSITORY R223 Okular REVISION DETAIL https://phabricator.kde.org/D6268 To: hetzenecker, davidedmundson, aacid Cc: sander, anthonyfieroni, #okular, a

D6268: HiDPI Support for Okular

sander added a comment. I don't know. But as this thread's title is "HiDPI Support for Okular", and as Lukas has turned into an Okular rendering expert I just mention here everything I notice. Feel free to ignore me. REPOSITORY R223 Okular REVISION DETAIL https://phabricator.kde.org/

D6268: HiDPI Support for Okular

davidedmundson added a comment. From my POV (pending a reply from Sander) this is good to go. INLINE COMMENTS > pageviewannotator.cpp:1122 > +imageVariant = "@2x"; > +} else { > +imageVariant = ""; that's a redundant else branch, and it won't get optimised out as technica

D6268: HiDPI Support for Okular

davidedmundson added a comment. > But of course I also found yet another issue: :-) When drawing in presentation mode, the resolution of the drawing is too low. The lines look pixelish. Can I confirm that that's not a regression from the pre-patch status but just something else that cou

D6268: HiDPI Support for Okular

sander added a comment. Hi Lukas, I tested and all my previous complaints seem to be resolved. Thank you! But of course I also found yet another issue: :-) When drawing in presentation mode, the resolution of the drawing is too low. The lines look pixelish. REPOSITORY R223 Okular

D6268: HiDPI Support for Okular

hetzenecker marked 5 inline comments as done. REPOSITORY R223 Okular REVISION DETAIL https://phabricator.kde.org/D6268 To: hetzenecker, davidedmundson, aacid Cc: sander, anthonyfieroni, #okular, aacid

D6268: HiDPI Support for Okular

hetzenecker updated this revision to Diff 18574. hetzenecker marked an inline comment as done. hetzenecker added a comment. Addressed the previous comments, change includes in particular: - shallow copy of pixmaps - fix for annotation drawing with tilemanager - HiDPI pixmaps for annota

D6268: HiDPI Support for Okular

sander added a comment. F3867184: okular-hidpi-render-glitch.png REPOSITORY R223 Okular REVISION DETAIL https://phabricator.kde.org/D6268 To: hetzenecker, davidedmundson, aacid Cc: sander, anthonyfieroni, #okular, aacid

D6268: HiDPI Support for Okular

sander added a comment. I do see minor rendering artifacts every now and then. Single rows of pixels seem to occur twice---possibly on tile boundaries. See the attached example image: the center line of text is slightly stretched. This happens at certain zoom levels. I don't really know

D6268: HiDPI Support for Okular

sander added a comment. Incidentally, I found another Okular hidpi problem: https://bugs.kde.org/show_bug.cgi?id=383589 :-) REPOSITORY R223 Okular REVISION DETAIL https://phabricator.kde.org/D6268 To: hetzenecker, davidedmundson, aacid Cc: sander, anthonyfieroni, #okular, aac

D6268: HiDPI Support for Okular

sander added a comment. I just tested again, and it looks much better than before. Thanks! There is now a problem with annotation drawing, though. When annotating a document, no annotation will actually be drawn until a repaint is triggered. Steps to reproduce: - Press F6 to o

D6268: HiDPI Support for Okular

davidedmundson added inline comments. INLINE COMMENTS > pagepainter.cpp:70 > QRect scaledCrop = crop.geometry( scaledWidth, scaledHeight ); > +const QRect dScaledCrop(QRectF(scaledCrop.x() * dpr, scaledCrop.y() * > dpr, scaledCrop.width() * dpr, scaledCrop.height() * dpr).toAlignedRect(

D6268: HiDPI Support for Okular

hetzenecker updated this revision to Diff 18216. hetzenecker added a comment. Fixed presentation mode Fixed scaling using TileManager All pixmaps get cached with the highest DPR of all screens. When moving the application to another screen, the cache doesn't have to be invalidated. This

D6268: HiDPI Support for Okular

aacid requested changes to this revision. aacid added a comment. This revision now requires changes to proceed. Marking as needs changes so it does not appear on my "needs review" list, please mark it back once you fix the comments by david REPOSITORY R223 Okular REVISION DETAIL https://

D6268: HiDPI Support for Okular

davidedmundson added a comment. qApp->DPR can change within the lifespan of the app; for example plugging in a new monitor can change things. This does mean it's very important to keep the metadata of the pixmap's dpr with the pixmap. It ends up simpler and safer overall. Doesn't mea

D6268: HiDPI Support for Okular

hetzenecker added a comment. Thanke for your feedback. In https://phabricator.kde.org/D6268#121135, @aacid wrote: > I would very much prefer if you didn't break the ABI of the files in core/ > > Also not sure why you need to pass the devicepixel ratio up and down, isn't it the sa

D6268: HiDPI Support for Okular

aacid added a comment. I would very much prefer if you didn't break the ABI of the files in core/ Also not sure why you need to pass the devicepixel ratio up and down, isn't it the same basically in all the app? REPOSITORY R223 Okular REVISION DETAIL https://phabricator.kde.org/D626

D6268: HiDPI Support for Okular

hetzenecker added a comment. I've defined my test cases here (sorry for using an external service, i wanted to show the test cases in a matrix/spreadsheet; you don't need an account for viewing)

D6268: HiDPI Support for Okular

hetzenecker edited the test plan for this revision. REPOSITORY R223 Okular REVISION DETAIL https://phabricator.kde.org/D6268 To: hetzenecker, davidedmundson, aacid Cc: sander, anthonyfieroni, #okular, aacid

D6268: HiDPI Support for Okular

hetzenecker added a comment. In https://phabricator.kde.org/D6268#117469, @sander wrote: > For me the patch didn't quite apply to the latest okular git master. Easy to fix, but still. The problem was the cosmetic changes in shell/main.cpp. I've rebased my branch to master >

D6268: HiDPI Support for Okular

hetzenecker added a comment. Thanks for all your feedback so far, I promise to address it soon in detail; just want to get another patch out before continuing with this one REPOSITORY R223 Okular REVISION DETAIL https://phabricator.kde.org/D6268 To: hetzenecker, davidedmundson, aacid Cc

D6268: HiDPI Support for Okular

sander added a comment. For me the patch didn't quite apply to the latest okular git master. Easy to fix, but still. The problem was the cosmetic changes in shell/main.cpp. After brief testing, the patch appears to do what it is supposed to do in normal page view mode. In presentation

D6268: HiDPI Support for Okular

anthonyfieroni added inline comments. INLINE COMMENTS > pageview.cpp:5324 > { > -PageViewItem currentPageItem = NULL; > +const PageViewItem *currentPageItem; > QSize viewportSize = viewport()->size(); This introduce a regression, since pointer can stay uninitialized or nullptr, ca

D6268: HiDPI Support for Okular

anthonyfieroni added a reviewer: aacid. REPOSITORY R223 Okular REVISION DETAIL https://phabricator.kde.org/D6268 To: hetzenecker, davidedmundson, aacid Cc: #okular, aacid

D6268: HiDPI Support for Okular

davidedmundson added a comment. "Test plan" simply needs to be a list of things you've tried doing so the maintainers can point out if there's any parts you've not thought about. So saying if you tested annotations and what types of files you opened, checked with zoom = 100%..that sort

D6268: HiDPI Support for Okular

hetzenecker created this revision. Restricted Application added a subscriber: Okular. Restricted Application added a project: Okular. REVISION SUMMARY HiDPI Support: This is my initial draft that enables HiDPI throught the application Every pixmap is multiplied by the devicePixelRatioF QPai