In D20868#615357, @aacid wrote:

Please move as a Merge Request in https://invent.kde.org/kde/okular

We have pre-commit CI and lots of checks including clazy and clang-tidy there so it's a much better place for doing the review/approval/merge of the code.

In T11080#192988, @jglogowski wrote:

IMHO most of your stuff is DE agnostic. And we implemented that with our config framework in Munich.

A few more thoughts on what large deployments may need or care about. Some of those have been mentioned already, some things already work just fine anyway and some others are outside of KDE scope IMHO:

In D9341#482119, @aacid wrote:

kprinter maybe/probably/potentially does things that the qt print dialog doesn't, but yes, it'd be much better if those were upstreamed rather than having another tool you have to learn how to use besides the print dialog of apps

In D9341#479680, @ngraham wrote:

Is this still relevant?

In D20197#443689, @hoffmannrobert wrote:

Thanks, I've learned that QFileDialog only creates a well behaving directory chooser, if the QFileDialog::ShowDirsOnly option is set to true _after_ having set the fileMode to QFileDialog::Directory. If you do it the other way around, it doesn't add directories to the selection.

In D18179#450252, @ngraham wrote:

OK, let's go for it!

@michaelweghorn I hope you know this means you're on the hook to fix any bugs that crop up, right? :)

Changes in this revision:

In D18179#442948, @ngraham wrote:

Never mind, removing the dependency made it work. Stupid Arc. :( Testing now...

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.

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

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.

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

In D18744#412409, @michaelweghorn wrote:

The test plan still says that Ctrl+P will select the page ("or press combination of CTRL and P to select the entire page"). As far as I understand the previous comments, this is Alt+P now, isn't it? (I didn't do any tests.) Can you update the description accordingly?

The test plan still says that Ctrl+P will select the page ("or press combination of CTRL and P to select the entire page"). As far as I understand the previous comments, this is Alt+P now, isn't it? (I didn't do any tests.) Can you update the description accordingly?

Thanks a lot! I'll take a look at these links.

(accidently closed the revision, so reopending it now)

Thanks for the super-quick review!

In D18179#394712, @aacid wrote:

The only way i can think of is making the long form not havve default values

https://paste.kde.org/p55lx6vrh

Changes in this revision:

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.

Adapted according to Albert's feedback:

If you have any other idea on how to avoid that issue with the printFiles() methods, I'd be thankful to hear about this.

In D18179#394030, @aacid wrote:

We would need a @since marker for the enum and for the new methods and a
// TODO merge with function above when a BIC change happens somehwere else
would also make sense

• Adapt order of ScaleMode enum values as implicitly suggested in Albert's comment (I missed that at first)

Changes in this revision:

Changes in this revision:

Thanks for the feedback and info. I've update the change accordingly now.

Undo accidental whitespace changes

Implement improvements suggested by Albert:

Adapt according to Albert's feedback:

In D18179#391664, @aacid wrote:

Maybe an enum is better than a bool so if in the future more scaling options are implemented we don't need to change the signature again?

OK, I understand. I don't have any strong opinion on this then.

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! :)

Changes in this version:

In D18118#391100, @sander wrote:

Do you speak for both mouse clicks and touch screen 'clicks'? Because the original wish is really only about touch screens, and the effects of this patch on mouse clicks was at least partially caused by my laziness (it's currently easier to treat them both the same).

I'd also prefer to leave the current behaviour of a left-click as it is, regardless of the exact position. That is also in line with what e.g. LibreOffice Impress does. Allowing to go back by doing a single-finger swipe sounds great.

Thanks for the explanation!

I find the approach interesting at least. However, I currently don't know enough of the different Poppler backends to really assess the situation properly. My thought was that it might possibly make sense to add an experimental option to enable this and https://git.reviewboard.kde.org/r/130235/ if the Arthur backend has general advantages over Splash and there was the plan to get it "production-ready". Can anybody quickly explain what the situation and plan with those backends is or is that too complex?

In D10974#384499, @ngraham wrote:

Is this even still necessary now that we have D7962? The new scaling combobox has an option to "Fit to full page" which ignores the margins (To respect them, you use "Fit to Printable area")

In D10974#381075, @ngraham wrote:

D7962 just landed! :)

Thanks for your helpful feedback. Given the last comments, I agree that having text selection mode is probably not particularly useful for touch displays. I'm thus abandoning this change.

In D9336#329040, @ngraham wrote:

Doesn't this make Okular much less touch-friendly by default?

While testing, the options did not seem to have any effect for me; changing them did not affect the printout.

In D10974#290190, @michaelweghorn wrote:

[...] Unless there's any objection to this, I'll try to have a closer look at this sometime next week (also after looking more closely at Oliver's updated printing-related changes D7949 and D7962).

In D7949#295165, @sander wrote:

The patch should not influence the behavior of rasterized printing. The code used QPrinter with the poppler splash backend before, and it should still do that.

@arthurpeters: Thanks for your ideas on this. For now, I'd like to stick with the two options that have been discussed so far in this change (see reasoning in D10974#217969) and suggest to handle potential further improvements separately.

In general, I really like the idea of providing this new option.

I might have missed something, but I'm not yet sure what the "correct" wording for the two options in the radio button or combobox would be. :-)

@aacid , @ngraham : Any chance we can reach a concensus here?
I personally don't have a strong opinion on what the best term to use in the dialog is and could live with all suggestions made so far.
Looking at the Firefox screenshot, the option "Ignore print margins and scale to full page" came to my mind in addition (which is quite long, but might be clear on what is happening?).

I'm not really sure by myself yet, but does it help to say "print area" instead of "printable area" (i.e. "Fit to print area") to take into account that the margins being used are not necessarily the hardware margins of the printer (which define the "printable area")? Would it be clear for users what this refers to?

In D10974#268014, @ngraham wrote:

[...]
Another option is to use a radio button:

Scaling: (o) None
         ( ) Auto-fit within printable area

[...]

In D10974#267891, @aacid wrote:

In D10974#267814, @michaelweghorn wrote:

@aacid: What's the right way to properly indicate that an important part of this
was actually done by you (add you in the copyright, mention in the commit message,...)?

Just add a line to the commit message and it's fine for me

In D10974#267887, @ngraham wrote:

From a user perspective, I'm not sure I would know what "Ignore printer margins" means. What's a printer margin? If I turn this on or off, what will happen? It's not very clear at the moment, IMHO.

In D10974#267814, @michaelweghorn wrote:

[....]
(Side note: I missed this at first, since values for the margins in the Qt print
dialog currently don't seem to be initialized with the PPD default values.)

Changes to previous version:

In D10974#264672, @aacid wrote:

[...]
The feature it's not really "Fit to printable area", it's more "Take Margins into account" or if we negate it "Ignore printer margins", because "printable area" is the area of the page where the printer can print, but what we're doing here is ignore the printer margins, that default to the printable area but the user can change at will.

What do you think?

(Don't) Set margins based on QPrinter::fullPage()

In D10974#217646, @aacid wrote:

so you worded point 4 and point 6 (second point 5) differently "the document is not scaled at all now." vs "The document is scaled to the full page size", i would not expect Force Rasterize to cause different behaviour.

Is it just that you wrote it differently or does Force Rasterize actually change the behaviour of "Fit to printable area"?

Once https://phabricator.kde.org/D7962 should make it to Okular, it might make sense to reconsider using the combobox for the scaling options that this adds for the PDF generator case (or how to properly merge the two approaches for the QPrinter and the lpr approaches).

In general, this option is not only relevant for the PDF generator, but (as Albert mentioned on the bug report) for all generators that use FilePrinter.
This change currently only addresses the PDF case.

As mentioned in https://bugs.kde.org/show_bug.cgi?id=389224#c2 , there is the following new commit for Purpose that should fix the build when the "old" name is used for the dependency: https://cgit.kde.org/purpose.git/commit/?id=20a304236100b29014403a894cbda2d93ab28e41

In D9328#179915, @aacid wrote:

@michaelweghorn can you give it a try with the new version of the patch? This should make it better.

I tested this a bit and find it really nice that rendering of the actual page (rather than the thumbnail) usually starts at once when re-rendering is needed (e.g. when zoom level is changed).
Also, closing Okular now works fast, even if rendering is still in progress. (The shell prompt is shown almost immediately after closing Okular when it was started from the command line.)

The 2 suggested improvements are now implemented.

Rename to "kprinter5", update includes

In T4151#116690, @aacid wrote:

See https://phabricator.kde.org/D8642

In D8642#167020, @aacid wrote:

In D8642#165611, @michaelweghorn wrote:

Open pdf file, add anotation, close app
You get dialog about losing changes, check that save, discard, cancel all do what they say

This works as described. One additional thought on that:
Some users may want to save the modified version to a new file instead of overwriting the existing one. While this is already possible via "File" -> "Save As" (or Ctrl+Shift+S), I think it might be helpful to add an additional button "Save as" in that dialog as well. This would also help avoid users accidently overwriting the original file. (Evince, for example only offers a "Save As" option and users switching between the two programs might not be aware of the difference at once.)

I disagree, does libreoffice suggest "Save As" when you do open an existing file, do some changes and close the app? Do you know of any program that does offer "Save As" in that situation?

Open pdf file, add anotation, close app
You get dialog about losing changes, check that save, discard, cancel all do what they say

In D7949#154764, @sander wrote:

Background: [...] and we plan to have some features implemented in the print dialog (for Qt/Okular).

Can I learn more about this somewhere?

In D7949#154351, @sander wrote:

large filesize (5x), because of letters being vectors instead of fonts

Yeah, I know about this one. There's nothing really do be done about this on the Okular side, this is Qt's doing.
On the other hand one could argue that this is what "printing to a file" actually means: you lose all semantic
information like letters, annotations, tags, etc.