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

Use supported codepoints instead of scripts in Monospace fallback #215

Merged
merged 1 commit into from
Jan 19, 2024

Conversation

MoSal
Copy link
Contributor

@MoSal MoSal commented Jan 18, 2024

This should/could improve fallback order.

This could also probably be utilized for non-Monospace fallback too.
But I didn't want to touch that code to avoid accidentally breaking
anything.

@jackpot51
Copy link
Member

Make sure to format with cargo fmt and then run the tests with ./test.sh

 This should/could improve fallback order.

 This could also probably be utilized for non-Monospace fallback too.
 But I didn't want to touch that code to avoid accidentally breaking
 anything.

Signed-off-by: Mohammad AlSaleh <[email protected]>
@MoSal MoSal force-pushed the fallback_codepoints branch from 54918b6 to a4a0c94 Compare January 18, 2024 20:43
@MoSal
Copy link
Contributor Author

MoSal commented Jan 18, 2024

Formatting done.

As for tests, these 6 fail on my system with and without this change. They trigger the "no default font found" expect() from src/shape.rs:

failures:
    test_arabic_paragraph_rendering
    test_arabic_word_rendering
    test_english_mixed_with_arabic_paragraph_rendering
    test_english_mixed_with_hebrew_paragraph_rendering
    test_hebrew_paragraph_rendering
    test_hebrew_word_rendering

@MoSal
Copy link
Contributor Author

MoSal commented Jan 18, 2024

Pushed the Emoji commit here because the same code block is touched.

@MoSal MoSal marked this pull request as draft January 18, 2024 21:43
@MoSal
Copy link
Contributor Author

MoSal commented Jan 18, 2024

Converted to draft until wide Monospace cases are sorted out properly.

@MoSal MoSal marked this pull request as ready for review January 18, 2024 22:50
@MoSal MoSal force-pushed the fallback_codepoints branch from cfcfdda to a4a0c94 Compare January 19, 2024 16:09
@MoSal
Copy link
Contributor Author

MoSal commented Jan 19, 2024

Removed the emoji change in favor of #216.

@jackpot51 jackpot51 merged commit db1530c into pop-os:main Jan 19, 2024
2 of 4 checks passed
@vorporeal
Copy link
Contributor

vorporeal commented Apr 2, 2024

Was any performance analysis done on this change? I'm noticing in CPU profiles for our application that a lot of time is spent iterating over codepoints in subtables during Font construction - for extensive unicode fonts, this could end up doing significantly more work than the previous code.

@MoSal
Copy link
Contributor Author

MoSal commented Apr 2, 2024

Was any performance analysis done on this change? I'm noticing in CPU profiles for our application that a lot of time is spent iterating over codepoints in subtables during Font construction - for extensive unicode fonts, this could end up doing significantly more work than the previous code.

After #242, performance should be fine. There is bit more work done in font system initialization. But after that, performance shouldn't be an issue. I perf'ed this in cosmic-terminal. Feedback from other use cases would definitely be appreciated.

@vorporeal
Copy link
Contributor

We're using the latest published version (0.11.2), which doesn't include #242.

Should there be a new release cut and pushed to crates.io that contains the performance fix?

@MoSal
Copy link
Contributor Author

MoSal commented Apr 2, 2024

We're using the latest published version (0.11.2), which doesn't include #242.

Can you check with a version that includes it?

Should there be a new release cut and pushed to crates.io that contains the performance fix?

That decision resides with @jackpot51. I'm just a random contributor.


Still interested in learning more about the use-case where this caused noticeable regression. Is monospacity required for the use-case, or was it just a Monospace font that happened to be selected? Are very long lines involved? Are continually updating text buffers involved? ...

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.

3 participants