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

RFC: Support Pango text rendering by default #269

Merged
merged 7 commits into from
Jul 25, 2018

Conversation

filipnavara
Copy link
Contributor

Currently there are two text rendering backends, a Cairo based one and Pango based one. Historically the Pango backend was incomplete and buggy, so it was hidden under --with-pango option. I think the code is now mature enough that we should remove all the warnings about it being unsupported. I'd even go one step further and make it the default as long as the Pango libraries are installed on the system.

Rationale:

cc @kasper3

@filipnavara filipnavara force-pushed the pango-configure branch 3 times, most recently from cef9131 to f23cb91 Compare April 1, 2018 12:33
@ghost
Copy link

ghost commented Apr 1, 2018

make it the default as long as the Pango libraries are installed on the system

Should --with-pango option be removed so there is no opt-in or opt-out and on a build machine, autoconfig/cmake should fail without the required dependency present?

@filipnavara
Copy link
Contributor Author

filipnavara commented Apr 1, 2018

I don't think we should remove the Cairo support entirely yet. Accordingly it should be possible to specify --without-pango on machines with Pango installed to be able to test the Cairo code. Also, I have no idea how wide-spread is the use of --with-pango option in packing systems, so it's safer to keep it.

As for the reasons to keep the Cairo code for now:

  • Pango is heavy dependency
  • No Win32 build support for Pango
  • Ubuntu 14.04 LTS on the build machines has too old Pango
  • Distribution of Pango-based builds on macOS as part of other packages (eg. Mono) is not tested

@ghost
Copy link

ghost commented Apr 1, 2018

so it's safer to keep it

If we are requiring a dependency, then apt, yum, emerge, pkgsrc, fbsdports, aports etc. will add it under required section of their package manifest. If we keep it optional, then they may or may not.

If this is the plan then I think then we need to have some API for System.Drawing to query if pango (an optional dependency) is available.

Otherwise what we are being presented inaccurate information in System.Drawing docs.
Otherwise we need to tell dotnet/docs guys to add this fact in remarks.

Much easier to make this dependency mandatory. cc @akoeplinger, @safern

@filipnavara
Copy link
Contributor Author

filipnavara commented Apr 4, 2018

Ok, I looked at various places where libgdiplus is built and I am starting to lean towards making Pango a required dependency. Here's what I found:

  • Bockbuild (used to build Mono) already builds Pango. It's an ancient version, but at least that makes an excuse to update it and drop all the custom patches that were already merged and/or superseded by better patches.
  • Vcpkg can build Pango, so it should be simple to use it there! (Thanks @hughbe & @akoeplinger!)
  • Homebrew has a Pango formula, so it's a matter of simple reference.
  • All Linux distributions supported by CoreFX have Pango in the package management system.

The only problem I found is that we now require Pango version >= 1.38 for pango_fc_font_map_set_config. That is not available on Debian Jessie, Ubuntu 14.04 and possibly other systems. At the expense of some performance penalty it should be possible to use FcConfigSetCurrent and some mutexes as a substitute.

@qmfrederik
Copy link
Contributor

That is not available on Debian Jessie, Ubuntu 14.04 and possibly other systems.

One reason to keep support for Debian Jessie would be that the default Docker container images for both Mono and .NET Core are based off Debian Jessie.

If the pango_fc_font_map_set_config method is difficult to subsitute, another option may be to just create a Pango 1.38 or later package for Jessie/Ubuntu 14.04.

@filipnavara
Copy link
Contributor Author

Dropping support for Jessie/Ubuntu 14.04 is not an option as long as they are supported by .NET Core. Since Ubuntu 14.04 is LTS release we talking at least another year. I'm not familiar with the packaging systems and the respective policies of these distributions, so I didn't really consider it an option to get newer Pango on these systems.

I'll look at the possibility of substituting pango_fc_font_map_set_config, but it may be difficult. Other option is just not to support custom font collections and return an error.

@qmfrederik
Copy link
Contributor

Other option is just not to support custom font collections and return an error.

That sounds reasonable - I'm not sure how often custom font collections are used, getting a PlatformNotSupportedException or something similar (on the managed side of things) indicating this feature is available on a newer version of Debian/Ubuntu sounds acceptable, plus the problem will be void a year from now when support for these distros is removed.

@filipnavara
Copy link
Contributor Author

filipnavara commented Apr 5, 2018

I'd also consider it reasonable. However, two things should be considered:

  • It would change behaviour if the library is updated on these systems. Previously they would support custom fonts, but have broken rendering of complex layouts. Now it would be able to render complex layouts, but it would lose custom font support. (No one will update the official libgdiplus packages on these platforms though :D)
  • The CI machines running Jenkins and Travis are currently running on Ubuntu 14.04, so we would lose part of the tests. May not be a big deal since they would still run on Travis+macOS at least.

This was referenced Apr 15, 2018
@filipnavara filipnavara mentioned this pull request Jul 6, 2018
@marek-safar
Copy link
Member

/cc @migueldeicaza please review

Copy link
Contributor

@migueldeicaza migueldeicaza left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fine

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants