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

Xft2 with fallback #403

Merged
merged 2 commits into from
Jan 10, 2021
Merged

Conversation

mikeandmore
Copy link
Contributor

This PR removes Xft1 support and add font fallback to Xft2 support. With font fallback, fvwm can now render unicode strings.

s

Copy link
Member

@ThomasAdam ThomasAdam left a comment

Choose a reason for hiding this comment

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

Hi @mikeandmore

Really good work. A few stylistic changes, but otherwise seems to look good to me on a first pass!

libs/Fft.c Outdated
@@ -332,6 +332,7 @@ static void MatchFont(Display *dpy,
*yoff += info.yOff;

if (render_char) render_char(user_data, &sp);
else if (sp.font != font) XftFontClose(dpy, sp.font);
Copy link
Member

Choose a reason for hiding this comment

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

Ugh -- the style here is terrible. Not your fault though, you're just in-keeping with what's already there.

We should really get something like clang-format to handle this for us. (Hint, hint. :P)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ouch, that's a pure typo though.

libs/Fft.c Outdated Show resolved Hide resolved
libs/Fft.c Outdated Show resolved Hide resolved
libs/Fft.c Outdated Show resolved Hide resolved
libs/Fft.c Outdated
Comment on lines 231 to 232
// Searching 64 shorts doesn't takes a full hash table. That's only two
// cache lines.
Copy link
Member

Choose a reason for hiding this comment

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

Comment style needs changing.

libs/Fft.c Outdated
unsigned short hash = FcPatternHash(src);
hash ^= code; // Because FcPatternHash() does not hash charset due to its cost.

for (int i = font_cache.fifo_start; i < font_cache.fifo_end; i++) {
Copy link
Member

Choose a reason for hiding this comment

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

C89 loop delclaration, please.

libs/Fft.c Outdated Show resolved Hide resolved
libs/Fft.c Outdated Show resolved Hide resolved
@mikeandmore
Copy link
Contributor Author

Let me know if I miss anything.

Copy link
Member

@ThomasAdam ThomasAdam left a comment

Choose a reason for hiding this comment

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

Hi @mikeandmore

Thanks. This looks good to me. Will likely merge tomorrow.

@ThomasAdam
Copy link
Member

ThomasAdam commented Jan 10, 2021

Hey @mikeandmore

This looks good now. Just one final thing to do before I merge this. Please can you squash:

2894e3cad fix XftFont memory leak
2894e3cad fix XftFont memory leak

into:

e17b8a509 coding styles

Best way to do this is on your xft2-with-fallback branch, do the following:

git rebase -i master

Then in your editor, where it litsts:

pick 2894e3cad fix XftFont memory leak
pick e17b8a509 coding styles

Change pick for fixup on both lines, and then save, and push the result out (git push -f) and that way, we won't have these intermediary commits which show style changes.

Hope that makes sense.

Cheers,
Thomas

1. We don't need CombineChars for Xft2, we can convert using
Fontconfig on the fly.

2. XftFontMatch() is slow and so we need a cache. This cache works
well for menus because same characters are rendered repeatly when
the cursor moves in/out a menu item.
@mikeandmore
Copy link
Contributor Author

pushed.

@ThomasAdam ThomasAdam merged commit 070f355 into fvwmorg:master Jan 10, 2021
@mikeandmore mikeandmore deleted the xft2-with-fallback branch January 10, 2021 18:48
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.

2 participants