D10249: Option to exit after printing
This revision was automatically updated to reflect the committed changes. Closed by commit R223:09b7b079ac92: Option to exit after printing (authored by dileepsankhla, committed by aacid). CHANGED PRIOR TO COMMIT https://phabricator.kde.org/D10249?vs=28360=28500#toc REPOSITORY R223 Okular CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D10249?vs=28360=28500 REVISION DETAIL https://phabricator.kde.org/D10249 AFFECTED FILES autotests/mainshelltest.cpp part.cpp part.h shell/main.cpp shell/shell.cpp shell/shellutils.cpp shell/shellutils.h To: dileepsankhla, aacid, #okular, ngraham Cc: ltoscano, ngraham, aacid, #okular, michaelweghorn
D10249: Option to exit after printing
aacid accepted this revision. aacid added a comment. This revision is now accepted and ready to land. Looks good now :) REPOSITORY R223 Okular BRANCH fixing REVISION DETAIL https://phabricator.kde.org/D10249 To: dileepsankhla, aacid, #okular, ngraham Cc: ltoscano, ngraham, aacid, #okular, michaelweghorn
D10249: Option to exit after printing
dileepsankhla updated this revision to Diff 28360. dileepsankhla added a comment. Updating D10249 <https://phabricator.kde.org/D10249>: Option to exit after printing Removed the autotest and just updated the `serializedOptions` parameters. REPOSITORY R223 Okular CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D10249?vs=27086=28360 BRANCH fixing REVISION DETAIL https://phabricator.kde.org/D10249 AFFECTED FILES autotests/mainshelltest.cpp part.cpp part.h shell/main.cpp shell/shell.cpp shell/shellutils.cpp shell/shellutils.h To: dileepsankhla, aacid, #okular, ngraham Cc: ltoscano, ngraham, aacid, #okular, michaelweghorn
D10249: Option to exit after printing
dileepsankhla added a comment. 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? REPOSITORY R223 Okular REVISION DETAIL https://phabricator.kde.org/D10249 To: dileepsankhla, aacid, #okular, ngraham Cc: ltoscano, ngraham, aacid, #okular, michaelweghorn
D10249: Option to exit after printing
aacid added a comment. 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 * REPOSITORY R223 Okular REVISION DETAIL https://phabricator.kde.org/D10249 To: dileepsankhla, aacid, #okular, ngraham Cc: ltoscano, ngraham, aacid, #okular, michaelweghorn
D10249: Option to exit after printing
dileepsankhla added a comment. No I ran the tests too and it was a success. REPOSITORY R223 Okular REVISION DETAIL https://phabricator.kde.org/D10249 To: dileepsankhla, aacid, #okular, ngraham Cc: ltoscano, ngraham, aacid, #okular, michaelweghorn
D10249: Option to exit after printing
aacid requested changes to this revision. This revision now requires changes to proceed. REPOSITORY R223 Okular REVISION DETAIL https://phabricator.kde.org/D10249 To: dileepsankhla, aacid, #okular, ngraham Cc: ltoscano, ngraham, aacid, #okular, michaelweghorn
D10249: Option to exit after printing
aacid added a comment. Have you tried running the test or did you add stuff just blindly in? REPOSITORY R223 Okular REVISION DETAIL https://phabricator.kde.org/D10249 To: dileepsankhla, aacid, #okular, ngraham Cc: ltoscano, ngraham, aacid, #okular, michaelweghorn
D10249: Option to exit after printing
dileepsankhla updated this revision to Diff 27086. dileepsankhla added a comment. Updating D10249 <https://phabricator.kde.org/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. REPOSITORY R223 Okular CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D10249?vs=26778=27086 BRANCH master REVISION DETAIL https://phabricator.kde.org/D10249 AFFECTED FILES autotests/mainshelltest.cpp part.cpp part.h shell/main.cpp shell/shell.cpp shell/shellutils.cpp shell/shellutils.h To: dileepsankhla, aacid, #okular, ngraham Cc: ltoscano, ngraham, aacid, #okular, michaelweghorn
D10249: Option to exit after printing
aacid added inline comments.
INLINE COMMENTS
> mainshelltest.cpp:215
> +QTest::newRow("print attach unique tabs") << file1 << optionsUnique <<
> true << contentsEpub[0] << 0u << false << false << false << true << 2u <<
> false << true;
> +QTest::newRow("print attach unique tabs and exit") << file1 <<
> optionsUnique << true << contentsEpub[0] << 0u << false << false << false <<
> true << 2u << false << true;
> }
isn't this row exactly the same as the previous?
REPOSITORY
R223 Okular
REVISION DETAIL
https://phabricator.kde.org/D10249
To: dileepsankhla, aacid, #okular, ngraham
Cc: ltoscano, ngraham, aacid, #okular, michaelweghorn
D10249: Option to exit after printing
dileepsankhla marked an inline comment as done. dileepsankhla added a comment. @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 INLINE COMMENTS > aacid wrote in part.cpp:3212 > What is the value of isError when > > if ( printDialog->exec() ) > > returns false? Undefined behavior - isError can either be true or false, my bad. Submitting a new patch. > aacid wrote in part.cpp:3217 > 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? 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 REPOSITORY R223 Okular REVISION DETAIL https://phabricator.kde.org/D10249 To: dileepsankhla, aacid, #okular, ngraham Cc: ltoscano, ngraham, aacid, #okular, michaelweghorn
D10249: Option to exit after printing
aacid added inline comments. INLINE COMMENTS > part.cpp:3217 > +if ( success && m_cliPrintAndExit ) > +exit ( EXIT_SUCCESS ); > +else if ( m_cliPrintAndExit ) 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? REPOSITORY R223 Okular REVISION DETAIL https://phabricator.kde.org/D10249 To: dileepsankhla, aacid, #okular, ngraham Cc: ltoscano, ngraham, aacid, #okular, michaelweghorn
D10249: Option to exit after printing
dileepsankhla updated this revision to Diff 26778. dileepsankhla added a comment. Updating https://phabricator.kde.org/D10249: Option to exit after printing renamed isError variable to success and changed the option with -print-and-exit. REPOSITORY R223 Okular CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D10249?vs=26608=26778 BRANCH master REVISION DETAIL https://phabricator.kde.org/D10249 AFFECTED FILES autotests/mainshelltest.cpp part.cpp part.h shell/main.cpp shell/shell.cpp shell/shellutils.cpp shell/shellutils.h To: dileepsankhla, aacid, #okular, ngraham Cc: ltoscano, ngraham, aacid, #okular, michaelweghorn
D10249: Option to exit after printing
ltoscano added inline comments.
INLINE COMMENTS
> mainshelltest.cpp:320
> + if (externalProcessExpectPrintDialogAndExit)
> +args << QStringLiteral("-print_and_exit");
> p.start(QStringLiteral(OKULAR_BINARY), args);
Regarding this option: shouldn't it be -print-and-exit instead? I don't think
that underscores are normally used in option names.
REPOSITORY
R223 Okular
REVISION DETAIL
https://phabricator.kde.org/D10249
To: dileepsankhla, aacid, #okular, ngraham
Cc: ltoscano, ngraham, aacid, #okular, michaelweghorn
D10249: Option to exit after printing
aacid added inline comments.
INLINE COMMENTS
> part.cpp:3240
>
> -void Part::doPrint(QPrinter )
> +bool Part::doPrint(QPrinter )
> {
Can you please make doPrint return true on success and not on error?
It's a much saner API.
REPOSITORY
R223 Okular
REVISION DETAIL
https://phabricator.kde.org/D10249
To: dileepsankhla, aacid, #okular, ngraham
Cc: ngraham, aacid, #okular, michaelweghorn
D10249: Option to exit after printing
dileepsankhla updated this revision to Diff 26608. dileepsankhla marked an inline comment as done. dileepsankhla added a comment. Updating https://phabricator.kde.org/D10249: Option to exit after printing Initialized "isError" with false at the time of declaration. REPOSITORY R223 Okular CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D10249?vs=26550=26608 BRANCH master REVISION DETAIL https://phabricator.kde.org/D10249 AFFECTED FILES autotests/mainshelltest.cpp part.cpp part.h shell/main.cpp shell/shell.cpp shell/shellutils.cpp shell/shellutils.h To: dileepsankhla, aacid, #okular, ngraham Cc: ngraham, aacid, #okular, michaelweghorn
D10249: Option to exit after printing
aacid added inline comments. INLINE COMMENTS > part.cpp:3212 > > +bool isError; > if ( printDialog->exec() ) What is the value of isError when if ( printDialog->exec() ) returns false? REPOSITORY R223 Okular REVISION DETAIL https://phabricator.kde.org/D10249 To: dileepsankhla, aacid, #okular, ngraham Cc: ngraham, aacid, #okular, michaelweghorn
D10249: Option to exit after printing
dileepsankhla updated this revision to Diff 26550. dileepsankhla added a comment. Updating https://phabricator.kde.org/D10249: Option to exit after printing local variable to slotPrint function REPOSITORY R223 Okular CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D10249?vs=26547=26550 BRANCH master REVISION DETAIL https://phabricator.kde.org/D10249 AFFECTED FILES autotests/mainshelltest.cpp part.cpp part.h shell/main.cpp shell/shell.cpp shell/shellutils.cpp shell/shellutils.h To: dileepsankhla, aacid, #okular, ngraham Cc: ngraham, aacid, #okular, michaelweghorn
D10249: Option to exit after printing
aacid added a comment. 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. REPOSITORY R223 Okular REVISION DETAIL https://phabricator.kde.org/D10249 To: dileepsankhla, aacid, #okular, ngraham Cc: ngraham, aacid, #okular, michaelweghorn
D10249: Option to exit after printing
dileepsankhla updated this revision to Diff 26547. dileepsankhla added a comment. Updating https://phabricator.kde.org/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. REPOSITORY R223 Okular CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D10249?vs=26453=26547 BRANCH master REVISION DETAIL https://phabricator.kde.org/D10249 AFFECTED FILES autotests/mainshelltest.cpp part.cpp part.h shell/main.cpp shell/shell.cpp shell/shellutils.cpp shell/shellutils.h To: dileepsankhla, aacid, #okular, ngraham Cc: ngraham, aacid, #okular, michaelweghorn
D10249: Option to exit after printing
aacid added a comment. Why is "isError" a class member variable? REPOSITORY R223 Okular REVISION DETAIL https://phabricator.kde.org/D10249 To: dileepsankhla, aacid, #okular, ngraham Cc: ngraham, aacid, #okular, michaelweghorn
D10249: Option to exit after printing
dileepsankhla added a comment. @ngraham Can you please review my patch? REPOSITORY R223 Okular REVISION DETAIL https://phabricator.kde.org/D10249 To: dileepsankhla, aacid, #okular, ngraham Cc: ngraham, aacid, #okular, michaelweghorn
D10249: Option to exit after printing
dileepsankhla updated this revision to Diff 26453. dileepsankhla added a comment. - # Enter a commit message. 1. Updating https://phabricator.kde.org/D10249: Option to exit after printing # 2. Enter a brief description of the changes included in this update. 3. The first line is used as subject, next lines as comment. # 4. If you intended to create a new revision, use: 5. $ arc diff --create Option to exit after printing REPOSITORY R223 Okular CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D10249?vs=26371=26453 BRANCH master REVISION DETAIL https://phabricator.kde.org/D10249 AFFECTED FILES autotests/mainshelltest.cpp part.cpp part.h shell/main.cpp shell/shell.cpp shell/shellutils.cpp shell/shellutils.h To: dileepsankhla, aacid, #okular, ngraham Cc: ngraham, aacid, #okular, michaelweghorn
D10249: Option to exit after printing
dileepsankhla added a comment. @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)? INLINE COMMENTS > aacid wrote in part.cpp:3219 > You have far too many exit calls, i'm 99% sure you could have just one. 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)? REPOSITORY R223 Okular REVISION DETAIL https://phabricator.kde.org/D10249 To: dileepsankhla, aacid, #okular, ngraham Cc: ngraham, aacid, #okular, michaelweghorn
D10249: Option to exit after printing
aacid added inline comments. INLINE COMMENTS > part.cpp:3219 > + delete printDialog; > + exit ( EXIT_SUCCESS ); > + } You have far too many exit calls, i'm 99% sure you could have just one. REPOSITORY R223 Okular REVISION DETAIL https://phabricator.kde.org/D10249 To: dileepsankhla, aacid, #okular, ngraham Cc: ngraham, aacid, #okular, michaelweghorn
D10249: Option to exit after printing
dileepsankhla added a reviewer: ngraham. REPOSITORY R223 Okular REVISION DETAIL https://phabricator.kde.org/D10249 To: dileepsankhla, aacid, #okular, ngraham Cc: ngraham, aacid, #okular, michaelweghorn
D10249: Option to exit after printing
ngraham edited the summary of this revision. REPOSITORY R223 Okular REVISION DETAIL https://phabricator.kde.org/D10249 To: dileepsankhla, aacid, #okular Cc: ngraham, aacid, #okular, michaelweghorn
D10249: Option to exit after printing
dileepsankhla edited the summary of this revision. REPOSITORY R223 Okular REVISION DETAIL https://phabricator.kde.org/D10249 To: dileepsankhla, aacid, #okular Cc: ngraham, aacid, #okular, michaelweghorn
D10249: Option to exit after printing
dileepsankhla added a reviewer: Okular. REPOSITORY R223 Okular REVISION DETAIL https://phabricator.kde.org/D10249 To: dileepsankhla, aacid, #okular Cc: ngraham, aacid, #okular, michaelweghorn
D10249: Option to exit after printing
dileepsankhla created this revision. dileepsankhla added a reviewer: aacid. Restricted Application added a subscriber: Okular. Restricted Application added a project: Okular. dileepsankhla requested review of this revision. REVISION 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 ackowledging the print dialog and thus useful for command line batch processing or a Dolphin service as the issue suggests. BUG: 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 REPOSITORY R223 Okular BRANCH master REVISION DETAIL https://phabricator.kde.org/D10249 AFFECTED FILES autotests/mainshelltest.cpp part.cpp part.h shell/main.cpp shell/shell.cpp shell/shellutils.cpp shell/shellutils.h To: dileepsankhla, aacid Cc: ngraham, aacid, #okular, michaelweghorn
