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
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,
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 SINC
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
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 :
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
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
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
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 fu
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
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
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,
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
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
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
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
R
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 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
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,
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
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
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
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
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
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
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
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
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
dileepsankhla added a reviewer: Okular.
REPOSITORY
R223 Okular
REVISION DETAIL
https://phabricator.kde.org/D10249
To: dileepsankhla, aacid, #okular
Cc: ngraham, aacid, #okular, michaelweghorn
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
30 matches
Mail list logo
