Skip to content
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

Feature Request - PrintToPDf Scaling #1965

Closed
GrabzIt opened this issue Feb 24, 2017 · 14 comments
Closed

Feature Request - PrintToPDf Scaling #1965

GrabzIt opened this issue Feb 24, 2017 · 14 comments

Comments

@GrabzIt
Copy link
Contributor

GrabzIt commented Feb 24, 2017

This is not so much a bug report as a feature request. Version 57 of Chromium now allows PDF print scaling, which should overcome some of the bad results when trying to print a webpage as PDF.

Unfortunately CEF has yet to add it to there library I have raised a case about it here: https://bitbucket.org/chromiumembedded/cef/issues/2106/please-add-scaling-to-pdf-printing

Is there any way it could be included in the #1963 release so that the option would be exposed here: https://github.com/cefsharp/CefSharp/blob/11dee93bf331f012dd6416fb2f2162549d9137a3/CefSharp/PdfPrintSettings.cs? I am not really sure if this is possible at this time, I was just excited when I saw the new feature because it is something that would be very useful.

If this case needs sponsorship I would be willing to pay. I am hoping it wouldn't take too much work as is just another attribute for a existing class, I would do it myself. But I don't have the level of c++ skills required.

@GrabzIt GrabzIt closed this as completed Feb 27, 2017
@amaitland amaitland changed the title PDF Scaling when Printing Feature Request - PrintToPDf Scaling Mar 6, 2017
@amaitland
Copy link
Member

But I don't have the level of c++ skills required.

@GrabzIt Even with minimal C++ experience I think there's a pretty high chance you could implement this yourself.

  • Add field to cef_pdf_print_settings_t
  • Add field to CefPdfPrintSettingsTraits::Set
  • Populate print_settings dictionary

Here are some relevant links to the CEF source
https://bitbucket.org/chromiumembedded/cef/commits/85f8368#Linclude/internal/cef_types.hT2268
https://bitbucket.org/chromiumembedded/cef/commits/85f8368#Llibcef/browser/printing/print_view_manager.ccT51

What I believe is the relevant dictionary key from the Chromium source.
https://cs.chromium.org/chromium/src/printing/printing_context.cc?type=cs&l=75

https://bitbucket.org/chromiumembedded/cef/wiki/ContributingWithGit.md#markdown-header-working-with-pull-requests

I'm sure you could ask a few pointers on http://magpcss.org/ceforum/index.php

@amaitland amaitland reopened this Mar 6, 2017
@GrabzIt
Copy link
Contributor Author

GrabzIt commented Mar 6, 2017

Thanks for the advice! I will look into it :-)

@GrabzIt
Copy link
Contributor Author

GrabzIt commented Mar 9, 2017

I have submitted this pull request to CEF (https://bitbucket.org/chromiumembedded/cef/pull-requests/110) just waiting for it to be accepted and I will submit the pull request for CEFSharp. :-D

@amaitland
Copy link
Member

I have submitted this pull request to CEF (https://bitbucket.org/chromiumembedded/cef/pull-requests/110)

Great 👍

just waiting for it to be accepted and I will submit the pull request for CEFSharp. :-D

Will need to be merged into the 2987 branch for it to appear in the 57 release.

@GrabzIt
Copy link
Contributor Author

GrabzIt commented Mar 9, 2017

I have requested the change be merged with 2987, hopefully it will be possible.

@amaitland amaitland added this to the 57.0.0 milestone Mar 17, 2017
@amaitland
Copy link
Member

Looks like your change was merged into 2987 with https://bitbucket.org/chromiumembedded/cef/commits/c2b4638063e34456360cc4baec4cf914df41720c

Should appear next time I upgrade master, should be possible to add the property to PdfPrintSettings after that.

I was unable to get PrintToPdf working successfully in 2987, same problem problem reproduces with the cefclient example app.

@GrabzIt
Copy link
Contributor Author

GrabzIt commented Mar 17, 2017

Good news about the change being merged. I am just waiting for a new build to appear here: http://opensource.spotify.com/cefbuilds/index.html

You are right the PrintToPDF feature is not working in version CEF 3.2987.1594.g92fba9c which is before my change, it seems to occur as soon as chromium version 57 is used as all the clients from CEF 3.2987.1590.g1f1b268 / Chromium 57.0.2987.74 I have raised a critical issue about it: https://bitbucket.org/chromiumembedded/cef/issues/2129/print-to-pdf-broken-in-version-57

@amaitland
Copy link
Member

I have raised a critical issue about it: https://bitbucket.org/chromiumembedded/cef/issues/2129/print-to-pdf-broken-in-version-57

Issue has been resolved, I'll upgrade to a version that includes that fix prior to releasing 57.0.0

Feel free to submit a PR that includes the scaling option 👍

@GrabzIt
Copy link
Contributor Author

GrabzIt commented Apr 7, 2017

That's brilliant news!! I am on holiday at the moment but i am coming back tomorrow so i will try to submit a PR, by Sunday.

@GrabzIt
Copy link
Contributor Author

GrabzIt commented Apr 8, 2017

I have added a PR (#2012) to tie up the new scaling property, however it doesn't seem to work. So I think the key used in CEF maybe wrong. Unless I have missed something, but I don't think that is the case.

@GrabzIt
Copy link
Contributor Author

GrabzIt commented Apr 8, 2017

However the good news is print to PDF is working again.

@amaitland
Copy link
Member

So I think the key used in CEF maybe wrong.

I think the Key is correct, though never hurts to double check. Looking a little deeper into the Chromium code, and the same key is used deep in their printing system.

https://cs.chromium.org/chromium/src/printing/print_settings_conversion.cc?type=cs&q=kSettingShouldPrintBackgrounds&l=207

There are other settings that don't appear to be working like backgrounds_enabled, so I think there might be another problem.

The CEF implementation is also hard coded to GRAY, so there are a few things that should be tweaked.

@GrabzIt
Copy link
Contributor Author

GrabzIt commented Apr 10, 2017

You are probably right, I will return to this if I come up with another idea.

Thanks for all your help with this issue btw :-)

@amaitland amaitland modified the milestones: 59.0.0, 57.0.0 Apr 12, 2017
@amaitland amaitland removed this from the 59.0.0 milestone Jun 2, 2017
@GrabzIt
Copy link
Contributor Author

GrabzIt commented Aug 26, 2017

This has been disabled by Chromium it works in the latest versions.

@GrabzIt GrabzIt closed this as completed Aug 26, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants