New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add :print --pdf flag to skip printing dialog #1639

Merged
merged 3 commits into from Jul 12, 2016

Conversation

Projects
None yet
4 participants
@jdkaplan
Contributor

jdkaplan commented Jul 11, 2016

I've given #1630 a try, and I hope it's up to quality standards.

I'm having a lot of trouble writing a test for it. I've never used pytest or BDD, but I figure the strategy would be to open up hello.txt or something, run :print --pdf tmp/dir/hello.pdf, and wait for the pdf to show up. I can keep trying if the test is necessary, but I'll probably need a few hints.


This change is Reviewable

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Jul 11, 2016

Current coverage is 82.75%

Merging #1639 into master will decrease coverage by 0.02%

@@             master      #1639   diff @@
==========================================
  Files           106        106          
  Lines         15429      15438     +9   
  Methods           0          0          
  Messages          0          0          
  Branches       2480       2482     +2   
==========================================
+ Hits          12773      12776     +3   
- Misses         2181       2188     +7   
+ Partials        475        474     -1   

Powered by Codecov. Last updated by 3bb5b5d...a6a030e

codecov-io commented Jul 11, 2016

Current coverage is 82.75%

Merging #1639 into master will decrease coverage by 0.02%

@@             master      #1639   diff @@
==========================================
  Files           106        106          
  Lines         15429      15438     +9   
  Methods           0          0          
  Messages          0          0          
  Branches       2480       2482     +2   
==========================================
+ Hits          12773      12776     +3   
- Misses         2181       2188     +7   
+ Partials        475        474     -1   

Powered by Codecov. Last updated by 3bb5b5d...a6a030e

@The-Compiler

This comment has been minimized.

Show comment
Hide comment
@The-Compiler

The-Compiler Jul 11, 2016

Collaborator

Thanks!

I'd really like a test for this, yeah 😉 If you prefer I can take care of it, I think a few additions to the BDD code (like having something like a (tmpdir) replacement which could be useful for other tests too) are needed first.

Did you autogenerate the documentation via scripts/dev/src2asciidoc.py? I think you didn't, and the next generation would overwrite your docs. When you add metavar='FILE' to @cmdutils.argument you should get similar output though.

Collaborator

The-Compiler commented Jul 11, 2016

Thanks!

I'd really like a test for this, yeah 😉 If you prefer I can take care of it, I think a few additions to the BDD code (like having something like a (tmpdir) replacement which could be useful for other tests too) are needed first.

Did you autogenerate the documentation via scripts/dev/src2asciidoc.py? I think you didn't, and the next generation would overwrite your docs. When you add metavar='FILE' to @cmdutils.argument you should get similar output though.

The-Compiler added a commit that referenced this pull request Jul 11, 2016

@@ -199,6 +199,7 @@ def run_command(quteproc, httpbin, command):
command = command.replace('(port)', str(httpbin.port))
command = command.replace('(testdata)', utils.abs_datapath())
command = command.replace('(tmpdir)', str(tmpdir))

This comment has been minimized.

@The-Compiler

The-Compiler Jul 12, 2016

Collaborator

I did the same thing in master - did this somehow end up in your PR or did you do the same change? I prefer your way of creating a tempdir via Given, didn't think of that 😄

@The-Compiler

The-Compiler Jul 12, 2016

Collaborator

I did the same thing in master - did this somehow end up in your PR or did you do the same change? I prefer your way of creating a tempdir via Given, didn't think of that 😄

This comment has been minimized.

@The-Compiler

The-Compiler Jul 12, 2016

Collaborator

Ah, you do use (tmpdir) in the test - in that case, I'm not sure if the Given a new tmpdir does anything at all, as the tempdir should already be created here

@The-Compiler

The-Compiler Jul 12, 2016

Collaborator

Ah, you do use (tmpdir) in the test - in that case, I'm not sure if the Given a new tmpdir does anything at all, as the tempdir should already be created here

@The-Compiler

This comment has been minimized.

Show comment
Hide comment
@The-Compiler

The-Compiler Jul 12, 2016

Collaborator

Whoops - with the QtWebEngine refactoring I apparently broke printing entirely, and nobody noticed until this test 😆 😊

Are you familiar with git rebase? I'll fix it in master, then you can rebase on the latest master.

Collaborator

The-Compiler commented Jul 12, 2016

Whoops - with the QtWebEngine refactoring I apparently broke printing entirely, and nobody noticed until this test 😆 😊

Are you familiar with git rebase? I'll fix it in master, then you can rebase on the latest master.

@The-Compiler

This comment has been minimized.

Show comment
Hide comment
@The-Compiler

The-Compiler Jul 12, 2016

Collaborator

So I looked into the printing situation for QtWebEngine to figure out what to best add to the new tab API, and so far, only printing to PDF is supported (in Qt 5.7).

I hope you don't mind, but I'll take over this PR, since I plan to add print_to_pdf() and print_to_qprinter() or so to the API. Sorry for that, totally wasn't aware how things are looking like with regards to printing.

Some back story: Until yesterday morning, you could directly access the underlying QWebView - this has now changed with a new AbstractTab API which tries to abstract all QtWebKit specific code so qutebrowser can work with the newer QtWebEngine backend - this is why this suddenly broke, as I totally forgot about printing and there weren't any tests for it (as the print dialog is impossible to test automatically).

Collaborator

The-Compiler commented Jul 12, 2016

So I looked into the printing situation for QtWebEngine to figure out what to best add to the new tab API, and so far, only printing to PDF is supported (in Qt 5.7).

I hope you don't mind, but I'll take over this PR, since I plan to add print_to_pdf() and print_to_qprinter() or so to the API. Sorry for that, totally wasn't aware how things are looking like with regards to printing.

Some back story: Until yesterday morning, you could directly access the underlying QWebView - this has now changed with a new AbstractTab API which tries to abstract all QtWebKit specific code so qutebrowser can work with the newer QtWebEngine backend - this is why this suddenly broke, as I totally forgot about printing and there weren't any tests for it (as the print dialog is impossible to test automatically).

@The-Compiler The-Compiler merged commit 6b2b096 into qutebrowser:master Jul 12, 2016

0 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details
@The-Compiler

This comment has been minimized.

Show comment
Hide comment
@The-Compiler

The-Compiler Jul 12, 2016

Collaborator

Merged with a few chages on top:

  • Add printing to tab API - cd4eff3
  • Remove redundant "Given a new tmpdir" step - fce825f
  • Sanity check the PDF file for :print --pdf test - 7703585 (check that it's actually some sort of PDF and not just an empty file)

Thanks, and sorry for the mess! 😉

Collaborator

The-Compiler commented Jul 12, 2016

Merged with a few chages on top:

  • Add printing to tab API - cd4eff3
  • Remove redundant "Given a new tmpdir" step - fce825f
  • Sanity check the PDF file for :print --pdf test - 7703585 (check that it's actually some sort of PDF and not just an empty file)

Thanks, and sorry for the mess! 😉

@jdkaplan jdkaplan deleted the jdkaplan:issues/1630 branch Jul 12, 2016

@jdkaplan jdkaplan restored the jdkaplan:issues/1630 branch Jul 12, 2016

@jdkaplan jdkaplan deleted the jdkaplan:issues/1630 branch Jul 12, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment