🌐 AI搜索 & 代理 主页
Skip to content

Conversation

@QuLogic
Copy link
Member

@QuLogic QuLogic commented Mar 22, 2025

PR summary

Along with #29695, this language parameter is an additional setting that may affect text layout. As with the other PR, this is not complete, but rather something to get the API decided upon (and likely will be rebased on raqm PR after it's done.)

There are two parts to the API:

  1. the language parameter attached to Text, and any other API that wraps it (i.e., Axes.text or Axes.annotate)
  2. the language parameter in backend's Renderer.draw_text

At the moment, for both, I have set these to str | list[tuple[str, int, int]] where the latter denotes a list of (language, start, end)-tuples. However, this was before I found out about the font feature machinery, and it is possible that we may wish to use that syntax instead, at least for API part 1. For part 2, it's a little nicer to have the explicit types, just for passing to the FT2Font extension, but this may not be relevant to other implementations.

PR checklist

@anntzer
Copy link
Contributor

anntzer commented Mar 22, 2025

re: adding language to draw_text: the general discussion about extensibility of renderer API applies, i.e.

  • if you're going to add language as a parameter to draw_text then it should be at least keyword-only.
  • it's likely that the more maintainable API would likely to just embed language as part of fontproperties (it's not really different from e.g. font features, I'd think) and tell the backend to read that info.
  • for draw_text we have actually more or less given up on having a "clean" renderer API because we pass the whole Text instance to draw_text (per Propagate mpl.text.Text instances to the backends and fix documentation #1081) anyways so perhaps backends that care could also just read that info off the Text instance. It feels a bit dirty, but heh... ("dirty" because we as well may just have a renderer API that's renderer.draw_text(textobj) to start with...)

@anntzer anntzer mentioned this pull request Mar 22, 2025
5 tasks
@anntzer
Copy link
Contributor

anntzer commented Mar 28, 2025

Side point regarding allowing list[tuple[str, int, int]] as language setting: this may be a problem if one uses somelanguage[idx:] to set the language from an index up to the end of the string (whose length could technically be modified later, even though it's not clear this necessarily makes much practical sense). Likely you need list[tuple[str, int, int | None]] (and then you may possibly support None (=0) for the start as well).

@QuLogic
Copy link
Member Author

QuLogic commented Apr 16, 2025

In the call a couple weeks ago, we decided to drop the new language parameter to draw_text, because that already had a mtext parameter which contained the whole Text object. This would avoid any signature introspection.

However, one followup is that Renderer.get_text_width_height_descent would also depend on font features/language settings, and really should get these passed through somehow as well.



def layout(string, font, *, kern_mode=Kerning.DEFAULT):
def layout(string, font, language, *, kern_mode=Kerning.DEFAULT):
Copy link
Contributor

Choose a reason for hiding this comment

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

These params should be kwonly? (throughout most of the PR, I think)

@QuLogic QuLogic force-pushed the text-language branch 2 times, most recently from 3c188df to 931423b Compare May 13, 2025 03:28
src/ft2font.cpp Outdated

if (languages.has_value()) {
for (auto & [lang_str, start, end] : languages.value()) {
if (!raqm_set_language(rq, lang_str.c_str(), start, end)) {
Copy link
Member Author

@QuLogic QuLogic Aug 27, 2025

Choose a reason for hiding this comment

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

This function actually takes a length, not an end location; I'm not sure which one we should accept (Harfbuzz feature tags use start/end, so maybe that?), but I'll have to update this to the correct naming (the test is setup to match what libraqm says, but the docs say the opposite.)

Copy link
Contributor

Choose a reason for hiding this comment

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

start-stop looks normal as API to me?

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree and have made that change.

@QuLogic QuLogic moved this from Waiting for other PR to Ready for Review in Font and text overhaul Sep 3, 2025
@QuLogic QuLogic force-pushed the text-language branch 3 times, most recently from 6fc2dee to 6ef55ed Compare September 3, 2025 03:22
@QuLogic
Copy link
Member Author

QuLogic commented Sep 3, 2025

Also extended the tests a bit to ensure that the results are as expected and fixed the fallback to the rcParam as well.

The test image is extracted to a separate commit as it should not be merged yet.

@QuLogic QuLogic force-pushed the text-language branch 2 times, most recently from d8051c2 to 886bf64 Compare September 19, 2025 05:55
@QuLogic QuLogic force-pushed the text-language branch 2 times, most recently from 0485aee to 3ffce36 Compare September 19, 2025 06:33
Comment on lines 35 to 36
fig.text(0, 0.1, f'English: {char}', fontsize=40, language='en')
fig.text(1, 0.1, f'Inari Sámi: {char}', fontsize=40, language='smn',
Copy link
Member

@tacaswell tacaswell Sep 19, 2025

Choose a reason for hiding this comment

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

Super picky change to match the example ordering to the text ordering:

Suggested change
fig.text(0, 0.1, f'English: {char}', fontsize=40, language='en')
fig.text(1, 0.1, f'Inari Sámi: {char}', fontsize=40, language='smn',
fig.text(0, 0.1, f'Inari Sámi: {char}', fontsize=40, language='smn')
fig.text(1, 0.1, f'English: {char}', fontsize=40, language='en',

@tacaswell
Copy link
Member

It looks like the appveyor tests hung.

@QuLogic
Copy link
Member Author

QuLogic commented Sep 20, 2025

OK, I fixed the doc note, and moved the test image to the text-overhaul-figures branch in preparation for merging.

@QuLogic QuLogic merged commit 44c17bc into matplotlib:text-overhaul Sep 20, 2025
46 of 47 checks passed
@QuLogic QuLogic deleted the text-language branch September 20, 2025 04:13
@github-project-automation github-project-automation bot moved this from Ready for Review to Done in Font and text overhaul Sep 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

4 participants