D6268: HiDPI Support for Okular

[Lukas Hetzenecker]

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

[Lukas Hetzenecker]

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

[Albert Astals Cid]

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

[Lukas Hetzenecker]

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
 )
  Unfortunately I've used the wrong REVIEW tag, so this didn't get closed 
automatically.

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

[Nathaniel Graham]

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

[Luigi Toscano]

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 be closed manually.

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

[Nathaniel Graham]

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, anthonyfieroni, #okular, aacid

D6268: HiDPI Support for Okular

[Nathaniel Graham]

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

[David Edmundson]

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

[Frederik Gladhorn]

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://phabricator.kde.org/D6268

To: hetzenecker, davidedmundson, aacid
Cc: gladhorn, ngraham, rkflx, sander, anthonyfieroni, #okular, aacid

D6268: HiDPI Support for Okular

[Lukas Hetzenecker]

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, anthonyfieroni, #okular, aacid

D6268: HiDPI Support for Okular

[Oliver Sander]

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, anthonyfieroni, #okular, aacid

D6268: HiDPI Support for Okular

[Lukas Hetzenecker]

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

To: hetzenecker, davidedmundson, aacid
Cc: ngraham, rkflx, sander, anthonyfieroni, #okular, aacid

D6268: HiDPI Support for Okular

[Oliver Sander]

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, anthonyfieroni, #okular, aacid

D6268: HiDPI Support for Okular

[Lukas Hetzenecker]

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 hardcoded to 32 in pageviewannotator.cpp, so I 
don't quite understand how this could have worked before..

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D6268

To: hetzenecker, davidedmundson, aacid
Cc: ngraham, rkflx, sander, anthonyfieroni, #okular, aacid

D6268: HiDPI Support for Okular

[Lukas Hetzenecker]

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/D6268

To: hetzenecker, davidedmundson, aacid
Cc: ngraham, rkflx, sander, anthonyfieroni, #okular, aacid

D6268: HiDPI Support for Okular

[Lukas Hetzenecker]

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 SINCE LAST UPDATE
  https://phabricator.kde.org/D6268?vs=18574=18871

REVISION DETAIL
  https://phabricator.kde.org/D6268

AFFECTED FILES
  conf/widgetconfigurationtoolsbase.cpp
  core/document.cpp
  core/generator.cpp
  shell/main.cpp
  ui/data/CMakeLists.txt
  ui/data/tool-base-oku...@2x.png
  ui/data/tool-highlighter-okular-coloriza...@2x.png
  ui/data/tool-ink-okular-coloriza...@2x.png
  ui/data/tool-note-inline-okular-coloriza...@2x.png
  ui/data/tool-note-okular-coloriza...@2x.png
  ui/pagepainter.cpp
  ui/pagepainter.h
  ui/pageview.cpp
  ui/pageview.h
  ui/pageviewannotator.cpp
  ui/presentationwidget.cpp

To: hetzenecker, davidedmundson, aacid
Cc: ngraham, rkflx, sander, anthonyfieroni, #okular, aacid

D6268: HiDPI Support for Okular

[Henrik Fehlauer]

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 icons are 
now too large (looks like 2x images unnecessarily scaled up again).

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D6268

To: hetzenecker, davidedmundson, aacid
Cc: rkflx, sander, anthonyfieroni, #okular, aacid

D6268: HiDPI Support for Okular

[David Edmundson]

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
  https://phabricator.kde.org/D6268

To: hetzenecker, davidedmundson, aacid
Cc: sander, anthonyfieroni, #okular, aacid

D6268: HiDPI Support for Okular

[Oliver Sander]

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: sander, anthonyfieroni, #okular, aacid

D6268: HiDPI Support for Okular

[Oliver Sander]

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, aacid

D6268: HiDPI Support for Okular

[Oliver Sander]

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

To: hetzenecker, davidedmundson, aacid
Cc: sander, anthonyfieroni, #okular, aacid

D6268: HiDPI Support for Okular

[David Edmundson]

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 technically

QString() is different from QString("");

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D6268

To: hetzenecker, davidedmundson, aacid
Cc: sander, anthonyfieroni, #okular, aacid

D6268: HiDPI Support for Okular

[David Edmundson]

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 could be improved upon.

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D6268

To: hetzenecker, davidedmundson, aacid
Cc: sander, anthonyfieroni, #okular, aacid

D6268: HiDPI Support for Okular

[Oliver Sander]

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

REVISION DETAIL
  https://phabricator.kde.org/D6268

To: hetzenecker, davidedmundson, aacid
Cc: sander, anthonyfieroni, #okular, aacid

D6268: HiDPI Support for Okular

[Lukas Hetzenecker]

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

[Lukas Hetzenecker]

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 annotation symbols
  - (hopefully) minor rendering artifacts on tile boundaries

REPOSITORY
  R223 Okular

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D6268?vs=18216=18574

REVISION DETAIL
  https://phabricator.kde.org/D6268

AFFECTED FILES
  core/document.cpp
  core/generator.cpp
  shell/main.cpp
  ui/data/CMakeLists.txt
  ui/data/tool-base-oku...@2x.png
  ui/data/tool-highlighter-okular-coloriza...@2x.png
  ui/data/tool-ink-okular-coloriza...@2x.png
  ui/data/tool-note-inline-okular-coloriza...@2x.png
  ui/data/tool-note-okular-coloriza...@2x.png
  ui/pagepainter.cpp
  ui/pagepainter.h
  ui/pageview.cpp
  ui/pageview.h
  ui/pageviewannotator.cpp
  ui/presentationwidget.cpp

To: hetzenecker, davidedmundson, aacid
Cc: sander, anthonyfieroni, #okular, aacid

D6268: HiDPI Support for Okular

[Oliver Sander]

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

[Oliver Sander]

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 how to reproduce it reliably.

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D6268

To: hetzenecker, davidedmundson, aacid
Cc: sander, anthonyfieroni, #okular, aacid

D6268: HiDPI Support for Okular

[Oliver Sander]

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, aacid

D6268: HiDPI Support for Okular

[Oliver Sander]

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 open the annotations toolbar
  - Select (e.g.) freehand annotations
  - Draw a line somewhere
  - ... except, no line appears :-)
  - trigger a repaint, e.g., by stepping to the next page and back
  - The line appears.

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D6268

To: hetzenecker, davidedmundson, aacid
Cc: sander, anthonyfieroni, #okular, aacid

D6268: HiDPI Support for Okular

[David Edmundson]

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());
> +

can you add some comments on your "d" prefix.

> pagepainter.cpp:110
> +if ( p != NULL ) {
> +pixmap = p->copy();
> +pixmap.setDevicePixelRatio( qApp->devicePixelRatio() );

we don't want to be doing this - copy does a deep copy which is expensive

QPixmap pixmap = *p would probably work as well, and that shares the underlying 
image data.

or we could just use p for this section, it doesn't seem like pixmap is used 
outside here.

> presentationwidget.cpp:842
>  // then blend the overlay (a piece of) over the background
> +m_lastRenderedOverlay.save("/tmp/overlay.png");
>  QRect ovr = m_overlayGeometry.intersected( r );

...

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D6268

To: hetzenecker, davidedmundson, aacid
Cc: sander, anthonyfieroni, #okular, aacid

D6268: HiDPI Support for Okular

[Lukas Hetzenecker]

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 
also simplified the code, because all PixmapRequests can be upscaled by the 
highest DPR. No more ABI breakage, because my changes can now be isolated to 
PixmapRequest and PagePainter/PresentationWidget

REPOSITORY
  R223 Okular

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D6268?vs=15570=18216

REVISION DETAIL
  https://phabricator.kde.org/D6268

AFFECTED FILES
  core/generator.cpp
  shell/main.cpp
  ui/pagepainter.cpp
  ui/pagepainter.h
  ui/pageview.cpp
  ui/pageview.h
  ui/presentationwidget.cpp

To: hetzenecker, davidedmundson, aacid
Cc: sander, anthonyfieroni, #okular, aacid

D6268: HiDPI Support for Okular

[Albert Astals Cid]

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://phabricator.kde.org/D6268

To: hetzenecker, davidedmundson, aacid
Cc: sander, anthonyfieroni, #okular, aacid

D6268: HiDPI Support for Okular

[David Edmundson]

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 mean we need to break ABI though.
  
  ---
  
  @hetzenecker
  
  There are two breaks RequestPixmap::RequestPixmap and Page::_o_nearest
  
  ---
  
  For requestPixmap we're sure to not process any screen events in the 
meantime, so you can use qApp->devicePixelRatio() in the two places in document 
and in the line in generator.
  
  For use of Page::_o_nearest you can't.
  However, there is an easy way to keep ABI for this sort of thing.
  
  If you have
  someMethod(int a) and want to change it to someMethod(int a, qreal dpr)
  
  keep both, but make the first method simply call the second with a default 
parameter.
  
  -

INLINE COMMENTS

> generator.cpp:106
> +QPixmap *p = new QPixmap( QPixmap::fromImage( img ) );
> +p->setDevicePixelRatio( p->devicePixelRatioF() );
> +request->page()->setPixmap( request->observer(), p, 
> request->normalizedRect() );

sure about this one?

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D6268

To: hetzenecker, davidedmundson, aacid
Cc: sander, anthonyfieroni, #okular, aacid

D6268: HiDPI Support for Okular

[Lukas Hetzenecker]

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 same basically in all the app?
  
  
  Yes and no. While it would be possible to use `qApp->devicePixelRatio()` (as 
I've done in `mobile/components`), this would return the highest pixel ratio in 
the system, in case there are more physical screens with different Scale 
Factors connected. 
  So, as alternative `QScreen::devicePixelRatio` could be used to return the 
DPR of the screen the application is shown. I'm not sure yet if this is 
possible without changing the ABI, but I'll try to.

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D6268

To: hetzenecker, davidedmundson, aacid
Cc: sander, anthonyfieroni, #okular, aacid

D6268: HiDPI Support for Okular

[Albert Astals Cid]

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/D6268

To: hetzenecker, davidedmundson, aacid
Cc: sander, anthonyfieroni, #okular, aacid

D6268: HiDPI Support for Okular

[Lukas Hetzenecker]

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)
  and will look into fixing presentation mode and some remaining zooming issues 
for the next 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

[Lukas Hetzenecker]

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

[Lukas Hetzenecker]

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
  
  > After brief testing, the patch appears to do what it is supposed to do in 
normal page view mode.  In presentation mode, however, the documents still look 
blurry.
  > 
  > Strangely, with the patch applied, I cannot change slides anymore in 
presentation mode.  Does anybody else see this behavior?
  
  I could reproduce this and included presentation mode in my test plan. Will 
look into this for the next 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

[Lukas Hetzenecker]

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: sander, anthonyfieroni, #okular, aacid

D6268: HiDPI Support for Okular

[Oliver Sander]

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 mode, however, the documents still look 
blurry.
  
  Strangely, with the patch applied, I cannot change slides anymore in 
presentation mode.  Does anybody else see this behavior?

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D6268

To: hetzenecker, davidedmundson, aacid
Cc: sander, anthonyfieroni, #okular, aacid

D6268: HiDPI Support for Okular

[Anthony Fieroni]

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, 
cause a crash in line 5334

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D6268

To: hetzenecker, davidedmundson, aacid
Cc: anthonyfieroni, #okular, aacid

D6268: HiDPI Support for Okular

[Anthony Fieroni]

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

[David Edmundson]

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 of thing.

INLINE COMMENTS

> pagepainter.cpp:75-76
>  
> +int dScaledWidth = floor(scaledWidth * dpr) + 1;
> +int dScaledHeight = floor(scaledHeight * dpr) + 1;
> +const QRect dLimits(QRectF(limits.x() * dpr, limits.y() * dpr, 
> limits.width() * dpr, limits.height() * dpr).toAlignedRect());

Why?

(maybe ceil() ?

> pagepainter.cpp:410
>  // highlight composition (product: highlight color * 
> destcolor)
> +/*
> +QRect highlightRect = r.geometry( dScaledWidth, 
> dScaledHeight ).translated( -dScaledCrop.topLeft() ).intersected( dLimits );

If the code below works, I'd avoid changing it.

Painting something on top isn't quite the same as the exitsing multiplying the 
two values together, and the old code has that //for 
odt or epub hack swapping black pixels for white ones.

> pagepainter.cpp:911
>  
> -void PagePainter::scalePixmapOnImage ( QImage & dest, const QPixmap * src,
> -int scaledWidth, int scaledHeight, const QRect & cropRect, 
> QImage::Format format )

Can you explain on Phabricator the changes you've made in PagePainter, and that 
benchmarking you did.

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D6268

To: hetzenecker, davidedmundson
Cc: #okular, aacid

D6268: HiDPI Support for Okular

[Lukas Hetzenecker]

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
  QPainter code is ajusted to take the DPR value into account
  Whereever possible, code was simplified to make use of modern QPainter 
features

TEST PLAN
  TBD

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D6268

AFFECTED FILES
  autotests/documenttest.cpp
  core/document.cpp
  core/generator.cpp
  core/generator.h
  core/generator_p.h
  core/page.cpp
  core/page.h
  core/tilesmanager.cpp
  core/tilesmanager_p.h
  generators/kimgio/tests/kimgiotest.cpp
  generators/poppler/generator_pdf.cpp
  mobile/components/pageitem.cpp
  shell/main.cpp
  ui/magnifierview.cpp
  ui/pagepainter.cpp
  ui/pagepainter.h
  ui/pageview.cpp
  ui/pageview.h
  ui/pageviewutils.cpp
  ui/pageviewutils.h
  ui/presentationwidget.cpp
  ui/thumbnaillist.cpp

To: hetzenecker, davidedmundson
Cc: #okular, aacid