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

[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

[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

[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

[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

[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

[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

[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

[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

[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