Option to exit after printing
ClosedPublic

Authored by dileepsankhla on Feb 2 2018, 12:45 PM.

Details

Summary

When running okular with the parameter --print to directly open the print mode, it doesn't exit after acknowledging the print dialog. Hence adding --print_and_exit option exits Okular after acknowledging the print dialog and thus is useful for the command line batch processing or a Dolphin service as the issue suggests.

FEATURE: 318998

Test Plan
  1. open a file in Okular using the parameter --print. It will open Okular in print mode with the print dialog
  2. Either print the file or cancel the print dialog
  3. You will find that Okular stays open
  4. Now using this patch, see for available options with the --help parameter. You will find --print_and_exit option
  5. Now open a file in Okular using the parameter --print_and_exit. It will open Okular in print mode with the print dialog
  6. Either print the file or cancel the print dialog
  7. You will find that Okular closes after acknowledging the dialog

Diff Detail

Repository
R223 Okular
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
dileepsankhla created this revision.Feb 2 2018, 12:45 PM
Restricted Application added a project: Okular. · View Herald TranscriptFeb 2 2018, 12:45 PM
Restricted Application added a subscriber: Okular. · View Herald Transcript
dileepsankhla requested review of this revision.Feb 2 2018, 12:45 PM
dileepsankhla edited the summary of this revision. (Show Details)Feb 2 2018, 12:50 PM
ngraham edited the summary of this revision. (Show Details)Feb 2 2018, 3:00 PM
aacid added inline comments.Feb 2 2018, 10:50 PM
part.cpp
3219

You have far too many exit calls, i'm 99% sure you could have just one.

@aacid Agree that only one exit call is required but what about the exit status - EXIT_SUCCESS or EXIT_FAILURE? The issue description talked about the command line batch processing. For that purpose, should I set a bool "isError" when failing condition is encountered like "printing is not allowed" and then based on the isError's status, should I call either exit (EXIT_SUCCESS) or exit (EXIT_FAILURE)?

part.cpp
3219

Agree that only one exit call is required but what about the exit status - EXIT_SUCCESS or EXIT_FAILURE? The issue description talked about the command line batch processing. For that purpose, should I set a bool "isError" when failing condition is encountered like "printing is not allowed" and then based on the isError's status, should I call either exit (EXIT_SUCCESS) or exit (EXIT_FAILURE)?

dileepsankhla updated this revision to Diff 26453.EditedFeb 3 2018, 6:28 PM

Updating D10249 : Option to exit after printing

I have returned the bool value from the doPrint function and inside the slotPrint function, the return value from doPrint is checked and then exits with failure if there was an error in printing else it exits with success.
Instead of using QApplication::quit() or QApplication::exit(), I have used the std::exit() function because QApplication::exit() will do nothing as the event loop is not there (from docs and also tested) and same for QApplication::quit() function which is same as QApplication::exit() and always exit with success value (no option for failure). Hence the only alternative that was left was the std::exit() function. It returns the exit code as desired and also terminates the application properly.

This comment was removed by dileepsankhla.
aacid added a comment.Feb 4 2018, 9:52 PM

Why is "isError" a class member variable?

Updating D10249: Option to exit after printing

I was trying to get rid of extern keyword while declaring "isError" inside the namespace of the header file. Hence I declared it as a class member, my bad.
I have declared it inside the source file "part.cpp" in the namespace definition.

aacid added a comment.Feb 4 2018, 11:36 PM

That's even worse.

I'll give you the answer myself.

Why do you need to have that variable be global at all? It is used *exactly* in one function.

Updating D10249: Option to exit after printing

local variable to slotPrint function

aacid added inline comments.Feb 5 2018, 5:46 PM
part.cpp
3212

What is the value of isError when

if ( printDialog->exec() )

returns false?

dileepsankhla marked an inline comment as done.

Updating D10249: Option to exit after printing
Initialized "isError" with false at the time of declaration.

aacid added inline comments.Feb 7 2018, 10:36 PM
part.cpp
3243

Can you please make doPrint return true on success and not on error?

It's a much saner API.

ltoscano added inline comments.
autotests/mainshelltest.cpp
320

Regarding this option: shouldn't it be -print-and-exit instead? I don't think that underscores are normally used in option names.

Updating D10249: Option to exit after printing
renamed isError variable to success and changed the option with -print-and-exit.

aacid added inline comments.Feb 10 2018, 11:33 PM
part.cpp
3224

We discussed a bit on IRC but i don't remeber which your reason for not using QCoreApplication::exit were, can you please remind me?

dileepsankhla marked an inline comment as done.Feb 11 2018, 7:02 AM

@aacid I have talked about using std::exit instead of QCoreApplication::exit here : https://phabricator.kde.org/D10249#200260

With that, I want to add QCoreApplication::exit does nothing if the event loop is not running. You can find the info in the docs here: http://doc.qt.io/qt-5/qcoreapplication.html#exit

part.cpp
3212

Undefined behavior - isError can either be true or false, my bad. Submitting a new patch.

3224

I have talked about using std::exit instead of QCoreApplication::exit here : https://phabricator.kde.org/D10249#200260

With that, I want to add that QCoreApplication::exit does nothing if the event loop is not running. You can find the info in the docs here: http://doc.qt.io/qt-5/qcoreapplication.html#exit

aacid added inline comments.Feb 11 2018, 11:46 PM
autotests/mainshelltest.cpp
215

isn't this row exactly the same as the previous?

Updating D10249: Option to exit after printing
Tried my best to edit the data driven tests by adding appropriate data to the table using QTest::newRow function.

Have you tried running the test or did you add stuff just blindly in?

aacid requested changes to this revision.Feb 21 2018, 10:25 PM
This revision now requires changes to proceed.Feb 21 2018, 10:25 PM

No I ran the tests too and it was a success.

aacid added a comment.EditedFeb 22 2018, 8:35 AM

You mean you don't get this?

 ./autotests/mainshelltest
********* Start testing of MainShellTest *********
Config: Using QtTest library 5.10.1, Qt 5.10.1 (x86_64-little_endian-lp64 shared (dynamic) release build; by GCC 7.3.0)
PASS   : MainShellTest::initTestCase()
QFATAL : MainShellTest::testShell(just show shell) QFETCH: Requested testdata 'externalProcessExpectPrintDialogAndExit' not available, check your _data function.
FAIL!  : MainShellTest::testShell(just show shell) Received a fatal error.
   Loc: [Unknown file(0)]
Totals: 1 passed, 1 failed, 0 skipped, 0 blacklisted, 4ms
********* Finished testing of MainShellTest *********

Oops! As I was getting errors in the autotests/mainshelltest.cpp file while make okular, I assumed the autotest didn't compile and build successfully. I also assumed the make command runs the tests too. Now running with ./autotests/mainshelltest, I got the error. After rectifying and rebuilding, I run the autotest again and whenever the window opens for the showPrintDialogAndExit, it waits for the user input and exits on clicking either Print or Cancel button. The autotest also stops there without showing any information about the passed and failed tests.
I'm not getting what's going on and how to proceed from this point. I even don't whether it is some error or the std::exit function stops the running test too. Should I eliminate 'showPrintDialogAndExit' row along with externalProcessExpectPrintDialogAndExit column?

Updating D10249: Option to exit after printing

Removed the autotest and just updated the serializedOptions parameters.

aacid accepted this revision.Mar 3 2018, 4:33 PM

Looks good now :)

This revision is now accepted and ready to land.Mar 3 2018, 4:33 PM
This revision was automatically updated to reflect the committed changes.