Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Add language parameter to Text objects#29794
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:text-overhaul
Are you sure you want to change the base?
Conversation
anntzer commentedMar 22, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
re: adding language to draw_text: the general discussion about extensibility of renderer API applies, i.e.
|
Side point regarding allowing |
In the call a couple weeks ago, we decided to drop the new However, one followup is that |
| @@ -65,7 +65,7 @@ def layout(string, font, *, kern_mode=Kerning.DEFAULT): | |||
| """ | |||
| x = 0 | |||
| prev_glyph_idx = None | |||
| char_to_font = font._get_fontmap(string) | |||
| char_to_font = font._get_fontmap(string) # TODO: Pass in language. | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Note, this function is getting rewritten with libraqm, so this is just a note for later.
lib/matplotlib/_text_helpers.py Outdated
| @@ -43,7 +43,7 @@ def warn_on_missing_glyph(codepoint, fontnames): | |||
| f"Matplotlib currently does not support {block} natively.") | |||
| def layout(string, font, *, kern_mode=Kerning.DEFAULT): | |||
| def layout(string, font,language,*, kern_mode=Kerning.DEFAULT): | |||
There was a problem hiding this comment.
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)
Uh oh!
There was an error while loading.Please reload this page.
3c188df to931423bCompareI've added a |
Also added a What's new note, though as with the other PR, it won't show the difference until |
| fig.text(0.5, 0.3, f'\\U{ord(char):08x}', fontsize=40, horizontalalignment='center') | ||
| 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', | ||
| horizontalalignment='right') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
can you add a (str, int, int) example just to cover all the forms?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think we're not ready to expose that just yet, so it's not been documented.
| with pytest.raises(TypeError): | ||
| font.set_text('foo', language=[1, 2, 3]) | ||
| with pytest.raises(TypeError): | ||
| font.set_text('foo', language=[(1, 2)]) | ||
| with pytest.raises(TypeError): | ||
| font.set_text('foo', language=[('en', 'foo', 2)]) | ||
| with pytest.raises(TypeError): | ||
| font.set_text('foo', language=[('en', 1, 'foo')]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
can you parameterize so there are labels to make it clear what's wrong with each case?
| font = ft2font.FT2Font(file, hinting_factor=1, _kerning_factor=0) | ||
| font.set_text('foo') | ||
| font.set_text('foo', language='en') | ||
| font.set_text('foo', language=[('en', 1, 2)]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
is this just testing it doesn't blow up? is there a .get_language to test that the setting didn't do anything strange?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yes, smoke testing only; full tests come with libraqm.
| with pytest.raises(TypeError, match='must be list of tuple'): | ||
| Text(0, 0, 'foo', language=[1, 2, 3]) | ||
| with pytest.raises(TypeError, match='must be list of tuple'): | ||
| Text(0, 0, 'foo', language=[(1, 2)]) | ||
| with pytest.raises(TypeError, match='start location must be int'): | ||
| Text(0, 0, 'foo', language=[('en', 'foo', 2)]) | ||
| with pytest.raises(TypeError, match='end location must be int'): | ||
| Text(0, 0, 'foo', language=[('en', 1, 'foo')]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
parameterize on match and language?
| _api.check_isinstance((list, str, None), language=language) | ||
| language = mpl._val_or_rc(language, 'text.language') | ||
| if isinstance(language, list): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
restricted to list or is any iterable fine?
| with pytest.raises(TypeError, match='must be list of tuple'): | ||
| Text(0, 0, 'foo', language=[1, 2, 3]) | ||
| with pytest.raises(TypeError, match='must be list of tuple'): | ||
| Text(0, 0, 'foo', language=[(1, 2)]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
this is a list of tuple
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yes, but the tuple is the wrong length (I just didn't write out the whole message in thematch.)
| _api.check_isinstance((list, str, None), language=language) | ||
| language = mpl._val_or_rc(language, 'text.language') | ||
| if isinstance(language, list): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
so does this mean that if language is a list it has to be a list where each element is a 3 tuple? so you can't do language = ['en', 'sr', 'ru']? which what would the list of tuple be used for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
No, that would not make sense, but it's understandable that it's not clear since it's purposely undocumented. If you provide multiple languages, then they would need to specify a range over which they apply.
PR summary
Along with#29695, thislanguage 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:
languageparameter attached toText, and any other API that wraps it (i.e.,Axes.textorAxes.annotate)languageparameter in backend'sRenderer.draw_textAt 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 aboutthe 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