Remember printing option and print range in the print dialog
Needs RevisionPublic

Authored by ahmadosama on Oct 17 2018, 9:39 PM.

Details

Reviewers
aacid
Group Reviewers
Okular
Summary

I updated the xml file used for saving current page and current page rotation to store the printing option and print range,
and added new parameters to the document class to store the required information about printing.

BUG: 98192

Diff Detail

Repository
R223 Okular
Branch
remember_print_range
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 4215
Build 4233: arc lint + arc unit
ahmadosama created this revision.Oct 17 2018, 9:39 PM
Restricted Application added a project: Okular. · View Herald TranscriptOct 17 2018, 9:39 PM
Restricted Application added a subscriber: okular-devel. · View Herald Transcript
ahmadosama requested review of this revision.Oct 17 2018, 9:39 PM
ahmadosama retitled this revision from Allow saving loading the printing option and range, missing to get data from the print dialog to save to Remember printing option and print range in the print dialog.Oct 17 2018, 9:44 PM
ahmadosama edited the summary of this revision. (Show Details)
ahmadosama added a reviewer: Okular.

This new information is saved/remembered per-document and not globally, right?

This new information is saved/remembered per-document and not globally, right?

Given it's in a document.cpp I would assume so, that's probably also where scroll position and what not is persisted

ahmadosama updated this revision to Diff 44036.Oct 21 2018, 5:38 PM
  • Resetting the printing option to the default options when closing document

This new information is saved/remembered per-document and not globally, right?

The new information was saved for all documents if Okular was not closed, closing the document only did not reset the printing options.
I reset the printing options when closing the document to avoid printing options from being stored for all opened documents before closing Okular.

Great, that sounds like exactly what would be expected. +1.

yurchor added inline comments.
core/document.cpp
652

Is it possible to keep the consistent formatting with other if's?

It seems that in other code it is "if ( condition )", but here it is "if( condition )" (without space between "if" and "(".

1245

Same here.

part.cpp
3241

Similar here.

3275

Same here.

aacid added a subscriber: aacid.Oct 21 2018, 6:17 PM

If you're not going to save the data, why are you adding it to the docinfo xml?

If this is only for remembering the options while okular is open i don't even think it makes sense storing it into document at all and should just be stored in Part as something like a QMap<Document,PrinterOptions>

If you're not going to save the data, why are you adding it to the docinfo xml?

If this is only for remembering the options while okular is open i don't even think it makes sense storing it into document at all and should just be stored in Part as something like a QMap<Document,PrinterOptions>

I am adding the data to the docinfo xml to remember those data when the file is reopened even after closing Okular the printing data would be remembered for the same file.

ahmadosama updated this revision to Diff 44041.Oct 21 2018, 6:40 PM
  • Reformatting the code to be consistent with other parts
ahmadosama updated this revision to Diff 44043.Oct 21 2018, 6:43 PM
  • Reformat other parts of the code

@yurchor
I have reformatted the code to be consistent, sorry for this.

aacid added a comment.Oct 21 2018, 7:17 PM

If you're not going to save the data, why are you adding it to the docinfo xml?

If this is only for remembering the options while okular is open i don't even think it makes sense storing it into document at all and should just be stored in Part as something like a QMap<Document,PrinterOptions>

I am adding the data to the docinfo xml to remember those data when the file is reopened even after closing Okular the printing data would be remembered for the same file.

But you just said "I reset the printing options when closing the document".

So I'm confused.

Is the plan to remember options between different Okular executions or is it no?

If you're not going to save the data, why are you adding it to the docinfo xml?

If this is only for remembering the options while okular is open i don't even think it makes sense storing it into document at all and should just be stored in Part as something like a QMap<Document,PrinterOptions>

I am adding the data to the docinfo xml to remember those data when the file is reopened even after closing Okular the printing data would be remembered for the same file.

But you just said "I reset the printing options when closing the document".

So I'm confused.

Is the plan to remember options between different Okular executions or is it no?

I apologize for my confusing comments.
Yes, the plan is to remember printing options between different executions similar to remembering the document rotation.

The problem in my first patch was that if I have an opened document X and then close this document,
and opened another document Y the printing options of X will be used in Y, so I added this part

d->m_printRangeOption = QPrinter::AllPages;
d->m_printFrom = -1;
d->m_printTo = -1;

similar to what is done to the rotation:

d->m_rotation = Rotation0;

to restore the printing options to the default values.
This is what I meant by resetting and I apologize again for not being clear.

aacid added a comment.Oct 22 2018, 9:27 PM

Ok, understood.

Personally i do not like this feature, the fact that i printed page 2 out of 400 of a document four months ago really means nothing regarding what i want to print now, in fact, most probably page 2 is exactly what i do not want to print.
Also it is not a behaviour the rest of KDE Applications has so it creates confusion amonsgt users why Okular seems to remember some settings but the rest of applications that seemingly have "the same printing dialog" behave different.

Besides that, i think the function names/comments could be improved

/**
 * Sets the printing range of the current document
 */
void setPrintRange(int from, int to);

doesn't really have to do anything with setting the printing rahge, all it does is save some value in an xml file.

So probably something like

/**
 * Sets the last page range used when printing the current document
 */
void setLastPrintRange(int from, int to);

makes more sense.

ahmadosama updated this revision to Diff 44260.Oct 26 2018, 6:44 PM
  • Changing variables, function names and comments to be more meaningful
aacid requested changes to this revision.Feb 21 2020, 6:56 PM

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.

This revision now requires changes to proceed.Feb 21 2020, 6:56 PM