Remove X11 and non-Cairo drawing backends#5612
Conversation
|
I didn't review this thoroughly, but the idea looks reasonable. GDK backend has been enabled for at least 6 years, and I guess raw X11 is not really needed anymore. Still, I'd like to seek additional opinions for such a bold move. |
|
@valadaptive, this is something that @iorsh and I have discussed doing in the future and shall likely happen, but it is going to require a little consultation with downstream parties and then a deprecation notice in order to limit disruption. Our limited market power means that changes that break somebody's packaging script can lead to the version getting pinned for a long time. |
|
Which downstream packagers still enable the X11 backend? |
|
Also, as @iorsh mentioned in #5611:
The latest master is already broken if the X11 backend is enabled. I definitely understand if you don't want this change being made by a new contributor though. |
|
@frank-trampe Is the implication that #5546 either needs to be backed out or needs an X11-specific work-around? |
|
@valadaptive, I was unable to reproduce the arbitrary zoom issue you encountered, but I guess it's reasonably common if you stumbled upon it. @skef, a build fix for #5611 is possible. It would slowly degrade the user experience with X11 backend (with newer GTK windows working, but buggy) instead of shutting it off at once. |
|
I remain skeptical of the inferences being made about the user base. What significant contact have we even had with the user base over the past, say, 4 years? How much script-level development has shifted to fontTools over the past, say, 7 years? What users who are relying on the X11 back-end specifically wouldn't be adequately served by simply sticking with an older version of the program? |
Perhaps relevant, I read this article on Hacker News, and it seems to show how people see FontForge nowadays. |
|
I have no current specific information from downstream users/packagers, just memories of past large changes not going over very well. Having been a victim of a few abrupt removals of non-deprecated features in other products, I am sensitive to the general importance of giving notice of large changes. FontForge has always been clunky, even when it had fewer features, and it was once a lot buggier than it is now. (Try a version from 2012.) Accordingly, evidence-based disdain for FontForge is nothing new. There are nonetheless a lot of people who still use FontForge, and it is important to avoid unnecessarily inconveniencing them. |
|
I did some checks, and of all the major distros I could recall none seems to override the default option of
|
|
Good. Our informal deprecation has been fruitful. |
iorsh
left a comment
There was a problem hiding this comment.
Excellent job, kudos! Just a few minor comments.
I meant to only remove cv->filled.
|
I've made the requested changes. I've also noticed that |
iorsh
left a comment
There was a problem hiding this comment.
Please also remove a few more defines I missed earlier:
- _NO_XKB
- _NO_XINPUT
- BUILT_WITH_XORG
- _NO_LIBCAIRO (there are few remnants in startui.c)
The file cmake/backports/3.15.7/FindX11.cmake should probably also go.
Please don't force-push, it break the possibility to do incremental review. Thanks!
|
I've removed those additional I think the entire CMake backports folder can probably go away in another PR--even Debian oldoldstable ships cmake 3.18, and CMake prints a big ugly deprecation warning if we set |
iorsh
left a comment
There was a problem hiding this comment.
This PR looks good to me. There is MacOS CI failure, but it should fix itself in a few days.
I'll merge this when the CI tests are clear, unless there are extraordinary objections.
CMake was introduced by @jtanx, and I don't know what were his considerations when adding local @jtanx, would it be safe to remove these files and rely on the standard CMake distribution? In any case, I'd prefer to defer such PR until after the official release. We've got enough risks for this release already. |
|
|
Let's also leave any |
As far as I could understand, this was the issue with #5610. Do we have @frank-trampe, do you have specific changes in mind? |
|
Also, it looks like @jtanx explicitly killed
It was noticed by downstream maintainers - e.g. https://bugs.archlinux.org/task/65877.html, but some of them probably chose to keep maintaining it internally, hence the versioning Ubuntu, for example, slowly pushes out the 2019 version - https://launchpad.net/ubuntu/focal/amd64/libfontforge-dev |
|
I somehow missed that at the time. It is certainly a problem that we need to fix at some point. |
|
|
||
| /* Sigh. I have to do my own clipping because at large magnifications */ | ||
| /* things can easily exceed 16 bits */ | ||
| static int CVSplineOutside(CharView *cv, Spline *spline) { |
There was a problem hiding this comment.
@valadaptive, are you satisfied that clipping happens efficiently in the Cairo pathway?
There was a problem hiding this comment.
Clipping has never happened in the Cairo pathway (on our side at least; Cairo probably does it internally). I haven't changed that in this PR.
There was a problem hiding this comment.
I understand that. My question is whether efficient rendering requires clipping geometry before passing it to Cairo and, if so, whether we do that. I have done very little work on the rendering pipeline, so I do not know this off-hand.
There was a problem hiding this comment.
I would assume it doesn't, considering we've never done so. I haven't delved too deeply into 2D vector graphics rendering either, but my understanding is that clipping is moreso a correctness thing than a performance one (e.g. if a shape is displayed clipping the left edge of the screen, you need to know to start the scanline with a fill). Cairo definitely handles that for us.
I may not fully understand the question--if you understand that the Cairo backend has never done clipping on the FontForge side, then why would it start being a performance issue now? Is it just something you noticed and are curious about?
There was a problem hiding this comment.
Yes. Just trying to maximize the value of whatever knowledge you have acquired working on this. It works fine, so there is no reason to dig into this further.
Ubuntu 20.04 will always ship the 2019 version though. That won't change no matter what we do here because any Debian-based distro release will (AFAIK) ship the same major versions of packages for its entire lifecycle, backporting fixes from newer versions if enterprise customers pay them to do so. In 22.04 onwards, |
|
@iorsh, I was originally proposing to leave out the |
|
Any volunteers to check for |
I valground some simple actions, like opening font, opening char view, copy-paste. The logs appear similar, I couldn't see any new issues. |
|
I sent the following message to the FontForge users mailing list last Saturday. There are no responses so far. Are there any other checks or adjustments that we need to make before merging?
|
|
I'm clear with the merge. There is an unrelated CI failure on |
|
Okay. It sounds like everybody is satisfied. Let's squash and merge. |
Resolves #5611 by just removing support for the X11 backend entirely.
If the eventual goal is to implement everything in GTK, we can remove some X11-exclusive code and get rid of quite a bit of cruft. In my testing, the X11 backend seems a bit broken--it doesn't actually exit the process when you close the windows. The non-Cairo drawing backend renders handles below outlines, and randomly decides to zoom into an arbitrary point:
Peek.2025-09-05.02-39.mp4
This PR is split into a few different parts. I can split them into separate PRs, but as a new contributor, I don't want to spam this repo with a bunch of them at once:
unifdefto remove the_NO_LIBCAIROifdefs. This flag was already never being set, and not much substantive code was gated behind it.FONTFORGE_CAN_USE_GDK. This removes all the gxdraw code, which was gated behind#ifndef FONTFORGE_CAN_USE_GDK.ENABLE_X11CMake option, which now does nothing.ENABLE_X11is off (which it is by default). Also remove the gxdraw source files (now just stubs) entirely.GDrawHasCairo, which now always returns true, and all code paths in the charview used for non-Cairo rendering. This gets rid of a lot of cruft. This should be reviewed carefully.Type of change