D10974: Add option to ignore print margins for non-PDF generators

[Michael Weghorn]

michaelweghorn added a comment.


  In D10974#442825 , @ngraham wrote:
  
  > Do you really not have commit access yet? You should apply. :) 
https://techbase.kde.org/Contribute/Get_a_Contributor_Account
  >
  > Meanwhile, I'll land this for you.
  
  
  Thanks a lot! Actually, I haven't contributed very much to KDE so far, so 
don't think I really qualify for commit access.

REPOSITORY
  R223 Okular

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

To: michaelweghorn, #okular, ngraham
Cc: fvogt, rkflx, arthurpeters, ltoscano, okular-devel, aacid, ngraham, 
joaonetto, tfella, darcyshen

D10974: Add option to ignore print margins for non-PDF generators

[Michael Weghorn]

michaelweghorn removed a dependent revision: D18179: PDF: Implement scaling 
options for non-rasterized printing.

REPOSITORY
  R223 Okular

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

To: michaelweghorn, #okular, ngraham
Cc: fvogt, rkflx, arthurpeters, ltoscano, okular-devel, aacid, ngraham, 
joaonetto, tfella, darcyshen

D10974: Add option to ignore print margins for non-PDF generators

[Nathaniel Graham]

This revision was automatically updated to reflect the committed changes.
Closed by commit R223:8bf1a911751b: Add option to ignore print margins for 
non-PDF generators (authored by michaelweghorn, committed by ngraham).

REPOSITORY
  R223 Okular

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10974?vs=49697=55345

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

AFFECTED FILES
  CMakeLists.txt
  core/document.h
  core/fileprinter.cpp
  core/printoptionswidget.cpp
  core/printoptionswidget.h
  generators/poppler/generator_pdf.cpp
  generators/poppler/generator_pdf.h
  interfaces/printinterface.h
  part.cpp

To: michaelweghorn, #okular, ngraham
Cc: fvogt, rkflx, arthurpeters, ltoscano, okular-devel, aacid, ngraham, 
joaonetto, tfella, darcyshen

D10974: Add option to ignore print margins for non-PDF generators

[Nathaniel Graham]

ngraham added a comment.


  Do you really not have commit access yet? You should apply. :) 
https://techbase.kde.org/Contribute/Get_a_Contributor_Account
  
  Meanwhile, I'll land this for you.

REPOSITORY
  R223 Okular

BRANCH
  michaelweghorn/UPDATE_D10974

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

To: michaelweghorn, #okular, ngraham
Cc: fvogt, rkflx, arthurpeters, ltoscano, okular-devel, aacid, ngraham, 
joaonetto, tfella, darcyshen

D10974: Add option to ignore print margins for non-PDF generators

[Michael Weghorn]

michaelweghorn added a comment.


  In D10974#442598 , @aacid wrote:
  
  > If someone wants to test it and approve it and Michael says he'll fix any 
bug that arises from here i'm happy enough
  
  
  I promise that I'll try to fix any issues arising from this. The same goes 
for D18179 .

REPOSITORY
  R223 Okular

BRANCH
  michaelweghorn/UPDATE_D10974

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

To: michaelweghorn, #okular, ngraham
Cc: fvogt, rkflx, arthurpeters, ltoscano, okular-devel, aacid, ngraham, 
joaonetto, tfella, darcyshen

D10974: Add option to ignore print margins for non-PDF generators

[Nathaniel Graham]

ngraham accepted this revision.
ngraham added a comment.
This revision is now accepted and ready to land.


  I tested it and it worked. :)

REPOSITORY
  R223 Okular

BRANCH
  michaelweghorn/UPDATE_D10974

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

To: michaelweghorn, #okular, ngraham
Cc: fvogt, rkflx, arthurpeters, ltoscano, okular-devel, aacid, ngraham, 
joaonetto, tfella, darcyshen

D10974: Add option to ignore print margins for non-PDF generators

[Nathaniel Graham]

ngraham added a comment.


  Yes, it does! How nice. +1

REPOSITORY
  R223 Okular

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

To: michaelweghorn, #okular
Cc: fvogt, rkflx, arthurpeters, ltoscano, okular-devel, aacid, ngraham, 
joaonetto, tfella, darcyshen

D10974: Add option to ignore print margins for non-PDF generators

[Albert Astals Cid]

aacid added a comment.


  The code looks sane from a "i have read it but not tried it" point of view.
  
  At this point this is all my stamina allows me to do.
  
  If someone wants to test it and approve it and Michael says he'll fix any bug 
that arises from here i'm happy enough

REPOSITORY
  R223 Okular

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

To: michaelweghorn, #okular
Cc: fvogt, rkflx, arthurpeters, ltoscano, okular-devel, aacid, ngraham, 
joaonetto, tfella, darcyshen

D10974: Add option to ignore print margins for non-PDF generators

[Michael Weghorn]

michaelweghorn added a comment.


  In D10974#442381 , @ngraham wrote:
  
  >   I'm still not thrilled about the {nav Force Rasterization} situation (I 
think it should be implicitly turned on when the user chooses an option that 
requires it) but the combobox is good.
  
  
  Does D18179  solve this? It makes the 
scaling options work without having to enable "Force Rasterization", so there 
should no longer be any need to implicitly turn it on.

REPOSITORY
  R223 Okular

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

To: michaelweghorn, #okular
Cc: fvogt, rkflx, arthurpeters, ltoscano, okular-devel, aacid, ngraham, 
joaonetto, tfella, darcyshen

D10974: Add option to ignore print margins for non-PDF generators

[Nathaniel Graham]

ngraham added a comment.


  FWIW this works well in my testing. I'm still not thrilled about the Force 
Rasterization situation (I think it should be implicitly turned on when the 
user chooses an option that requires it) but the combobox is good.
  
  @aacid, are you good with this now?

REPOSITORY
  R223 Okular

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

To: michaelweghorn, #okular
Cc: fvogt, rkflx, arthurpeters, ltoscano, okular-devel, aacid, ngraham, 
joaonetto, tfella, darcyshen

D10974: Add option to ignore print margins for non-PDF generators

[Michael Weghorn]

michaelweghorn added a comment.


  Is there anything I can still do right now or do I just need to wait for the 
review?

REPOSITORY
  R223 Okular

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

To: michaelweghorn, #okular
Cc: fvogt, rkflx, arthurpeters, ltoscano, okular-devel, aacid, ngraham, 
joaonetto, tfella, darcyshen

D10974: Add option to ignore print margins for non-PDF generators

[Michael Weghorn]

michaelweghorn added a comment.


  In D10974#394709 , @aacid wrote:
  
  > I don't really have the time to spend print stuff to check if this does 
what it says it does, so i'm just doing code review from the pure formal way.
  
  
  Many thanks for taking the time to look at this anyway!

REPOSITORY
  R223 Okular

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

To: michaelweghorn, #okular
Cc: fvogt, rkflx, arthurpeters, ltoscano, okular-devel, aacid, ngraham, tfella, 
darcyshen

D10974: Add option to ignore print margins for non-PDF generators

[Michael Weghorn]

michaelweghorn marked 4 inline comments as done.

REPOSITORY
  R223 Okular

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

To: michaelweghorn, #okular
Cc: fvogt, rkflx, arthurpeters, ltoscano, okular-devel, aacid, ngraham, tfella, 
darcyshen

D10974: Add option to ignore print margins for non-PDF generators

[Michael Weghorn]

michaelweghorn updated this revision to Diff 49697.
michaelweghorn added a comment.


  Adapted according to Albert's feedback:
  
  - Remove (now) unneeded imclude
  - remove 'virtual' keyword in derived class

REPOSITORY
  R223 Okular

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10974?vs=49608=49697

BRANCH
  michaelweghorn/UPDATE_D10974

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

AFFECTED FILES
  CMakeLists.txt
  core/document.h
  core/fileprinter.cpp
  core/printoptionswidget.cpp
  core/printoptionswidget.h
  generators/poppler/generator_pdf.cpp
  generators/poppler/generator_pdf.h
  interfaces/printinterface.h
  part.cpp

To: michaelweghorn, #okular
Cc: fvogt, rkflx, arthurpeters, ltoscano, okular-devel, aacid, ngraham, tfella, 
darcyshen

D10974: Add option to ignore print margins for non-PDF generators

[Albert Astals Cid]

aacid added a comment.


  I don't really have the time to spend print stuff to check if this does what 
it says it does, so i'm just doing code review from the pure formal way.

INLINE COMMENTS

> document.cpp:83
>  #include "pagecontroller_p.h"
> +#include "printoptionswidget.h"
>  #include "scripter.h"

i guess you don't need this include?

> printoptionswidget.h:46
> +
> +virtual bool ignorePrintMargins() const override;
> +

i think best practices say to not include virtual when it's an override

REPOSITORY
  R223 Okular

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

To: michaelweghorn, #okular
Cc: fvogt, rkflx, arthurpeters, ltoscano, okular-devel, aacid, ngraham, tfella, 
darcyshen

D10974: Add option to ignore print margins for non-PDF generators

[Michael Weghorn]

michaelweghorn updated this revision to Diff 49608.
michaelweghorn added a comment.


  Changes in this revision:
  
  - undo changes in 'interfaces/'
  - emit a qWarning() when 'dynamic_cast' fails

REPOSITORY
  R223 Okular

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10974?vs=49510=49608

BRANCH
  
michaelweghorn/WAITING_FOR_REVIEW_D10974_20190116T1115_add_scaling_options_for_other_generators

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

AFFECTED FILES
  CMakeLists.txt
  core/document.cpp
  core/document.h
  core/fileprinter.cpp
  core/printoptionswidget.cpp
  core/printoptionswidget.h
  generators/poppler/generator_pdf.cpp
  generators/poppler/generator_pdf.h
  interfaces/printinterface.h
  part.cpp

To: michaelweghorn, #okular
Cc: fvogt, rkflx, arthurpeters, ltoscano, okular-devel, aacid, ngraham, tfella, 
darcyshen

D10974: Add option to ignore print margins for non-PDF generators

[Albert Astals Cid]

aacid added inline comments.

INLINE COMMENTS

> michaelweghorn wrote in document.h:728
> I have changed the return type to a QWidget* again, but only for the Document 
> class itself, not in the other places. As far as I understand, this place 
> here is the only one relevant for the ABI. Is this correct?
> 
> Currently, there is no complaint if a plain QWidget* is returned, it's just 
> handled as it used to be then. Should I change that (e.g. emit a 
> 'qWarning()')?

interfaces/ is also installed when doing make install so compatibility there is 
also important.

Since you have a dynamic cast in part.cpp i guess it makes sense to have a 
warning when the dynamic cast fails? Telling the "developer" that he should 
update her implementation of the method?

REPOSITORY
  R223 Okular

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

To: michaelweghorn, #okular
Cc: fvogt, rkflx, arthurpeters, ltoscano, okular-devel, aacid, ngraham, tfella, 
darcyshen

D10974: Add option to ignore print margins for non-PDF generators

[Michael Weghorn]

michaelweghorn edited the summary of this revision.

REPOSITORY
  R223 Okular

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

To: michaelweghorn, #okular
Cc: fvogt, rkflx, arthurpeters, ltoscano, okular-devel, aacid, ngraham, tfella, 
darcyshen

D10974: Add option to ignore print margins for non-PDF generators

[Michael Weghorn]

michaelweghorn marked an inline comment as done.
michaelweghorn added inline comments.

INLINE COMMENTS

> aacid wrote in document.h:728
> this is binary incompatible :/
> 
> We could just let it be a QWidget but document it has to be a 
> PrintOptionsWidge subclass, and then on the user do a dynamic_cast and 
> complain if it is not a PrintOptionsWidget, but maybe we can just say meh and 
> break the BC.

I have changed the return type to a QWidget* again, but only for the Document 
class itself, not in the other places. As far as I understand, this place here 
is the only one relevant for the ABI. Is this correct?

Currently, there is no complaint if a plain QWidget* is returned, it's just 
handled as it used to be then. Should I change that (e.g. emit a 'qWarning()')?

REPOSITORY
  R223 Okular

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

To: michaelweghorn, #okular
Cc: fvogt, rkflx, arthurpeters, ltoscano, okular-devel, aacid, ngraham, tfella, 
darcyshen

D10974: Add option to ignore print margins for non-PDF generators

[Michael Weghorn]

michaelweghorn updated this revision to Diff 49510.
michaelweghorn edited the summary of this revision.
michaelweghorn added a comment.


  Adapt according to Albert's feedback:
  
  - undo ABI breakage by changing back signature of 
'Document::printConfigurationWidget()' to  what it is like without this change, 
and use a 'dynamic_cast' in 'Part::slotPrint'.
  - drop 'virtual' for 'PDFOptionsPage::ignorePrintMargins()'

REPOSITORY
  R223 Okular

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10974?vs=49223=49510

BRANCH
  
michaelweghorn/WAITING_FOR_REVIEW_D10974_add_scaling_options_for_other_generators

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

AFFECTED FILES
  CMakeLists.txt
  core/document.cpp
  core/document.h
  core/fileprinter.cpp
  core/printoptionswidget.cpp
  core/printoptionswidget.h
  generators/poppler/generator_pdf.cpp
  generators/poppler/generator_pdf.h
  interfaces/printinterface.h
  part.cpp

To: michaelweghorn, #okular
Cc: fvogt, rkflx, arthurpeters, ltoscano, okular-devel, aacid, ngraham, tfella, 
darcyshen

D10974: Add option to ignore print margins for non-PDF generators

[Albert Astals Cid]

aacid added inline comments.

INLINE COMMENTS

> document.h:728
>   */
> -QWidget* printConfigurationWidget() const;
> +PrintOptionsWidget* printConfigurationWidget() const;
>  

this is binary incompatible :/

We could just let it be a QWidget but document it has to be a PrintOptionsWidge 
subclass, and then on the user do a dynamic_cast and complain if it is not a 
PrintOptionsWidget, but maybe we can just say meh and break the BC.

> generator_pdf.cpp:141
>  
> +   virtual bool ignorePrintMargins() const override {
> +   return scaleMode() == FitToPage;

remove the virtual

REPOSITORY
  R223 Okular

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

To: michaelweghorn, #okular
Cc: fvogt, rkflx, arthurpeters, ltoscano, okular-devel, aacid, ngraham, 
darcyshen

D10974: Add option to ignore print margins for non-PDF generators

[Michael Weghorn]

michaelweghorn added a comment.


  In D10974#388761 , @ngraham wrote:
  
  > I agree that the current UI that makes you click "Force Rasterization" 
first is not ideal. Kudos if you find a technically acceptable way to improve 
that! :)
  
  
  My approach to do so is in https://phabricator.kde.org/D18179, which depends 
on this one here.
  
  I've split this change into two now, with this one now covering non-PDF 
generators (so exactly the opposite of the initial version of my patch...), and 
the other one (D18179 ) allowing the 
FilePrinter case to handle the additional option in the PDF generator's print 
dialog as well.

REPOSITORY
  R223 Okular

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

To: michaelweghorn, #okular
Cc: fvogt, rkflx, arthurpeters, ltoscano, okular-devel, aacid, ngraham, 
darcyshen

D10974: Add option to ignore print margins for non-PDF generators

[Michael Weghorn]

michaelweghorn added a dependent revision: D18179: PDF: Implement scaling for 
non-rasterized printing.

REPOSITORY
  R223 Okular

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

To: michaelweghorn, #okular
Cc: fvogt, rkflx, arthurpeters, ltoscano, okular-devel, aacid, ngraham, 
darcyshen

D10974: Add option to ignore print margins for non-PDF generators

[Michael Weghorn]

michaelweghorn updated this revision to Diff 49223.
michaelweghorn retitled this revision from "Add option to ignore print margins" 
to "Add option to ignore print margins for non-PDF generators".
michaelweghorn edited the summary of this revision.
michaelweghorn edited the test plan for this revision.
michaelweghorn added a comment.


  Changes in this version:
  
  - rebase to current master, including Oliver's printing changes for PDF
  - adapt GUI elements to match those used for the PDF case (only "Fit to 
printable area" and "Fit to full page" though)
  - only cover non-PDF generators in this change (PDF case will be dealt with 
in a subsequent change)

REPOSITORY
  R223 Okular

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10974?vs=34821=49223

BRANCH
  michaelweghorn/WIP_add_scaling_options_for_other_generators

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

AFFECTED FILES
  CMakeLists.txt
  core/document.cpp
  core/document.h
  core/fileprinter.cpp
  core/printoptionswidget.cpp
  core/printoptionswidget.h
  generators/poppler/generator_pdf.cpp
  generators/poppler/generator_pdf.h
  interfaces/printinterface.h
  part.cpp

To: michaelweghorn, #okular
Cc: fvogt, rkflx, arthurpeters, ltoscano, okular-devel, aacid, ngraham, 
darcyshen