Skip to content

ENH: expose font_file in brain.add_text (#12543)#13778

Open
PragnyaKhandelwal wants to merge 6 commits intomne-tools:mainfrom
PragnyaKhandelwal:enh-brain-add-text-font-file-12543
Open

ENH: expose font_file in brain.add_text (#12543)#13778
PragnyaKhandelwal wants to merge 6 commits intomne-tools:mainfrom
PragnyaKhandelwal:enh-brain-add-text-font-file-12543

Conversation

@PragnyaKhandelwal
Copy link
Copy Markdown
Contributor

Reference issue (if any)

Fixes #12543.

What does this implement/fix?

This PR exposes a font_file parameter in brain.add_text() and forwards it to the underlying PyVista add_text() call.

This allows users to pass a .ttf/.ttc font file for Unicode glyph rendering (for example, CJK characters and symbols), while preserving current behavior when font_file is not provided.

Additional information

Verification context:

  1. Aligns with issue discussion where specifying font_file in PyVista resolves Unicode rendering for add_text().
  2. Keeps scope minimal by exposing existing backend capability in the MNE API.

Copy link
Copy Markdown
Contributor

@wmvanvliet wmvanvliet left a comment

Choose a reason for hiding this comment

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

Really nice work! It would be nice to also test setting font_file as part of one of the unit tests.

@PragnyaKhandelwal
Copy link
Copy Markdown
Contributor Author

PragnyaKhandelwal commented Mar 23, 2026

Thanks for the review @wmvanvliet . I updated the backend to always pass font_file to Plotter.add_text, including None and added a unit test that checks both cases: font_file set to a non-None value and font_file=None.

@PragnyaKhandelwal
Copy link
Copy Markdown
Contributor Author

I verified the checks failed....looks like the build_docs failure is due to the ci merge step issue not a doc/content failure from this pr

@PragnyaKhandelwal PragnyaKhandelwal marked this pull request as ready for review March 24, 2026 12:30
@PragnyaKhandelwal
Copy link
Copy Markdown
Contributor Author

Hi @wmvanvliet and @drammock, just a quick note that I've addressed the feedback regarding the font discovery logic and documentation clarifications.
I've verified the font passthrough locally. Whenever you have a moment, I’m standing by for a final review or any further adjustments needed to get this merged!

@wmvanvliet
Copy link
Copy Markdown
Contributor

One comment left unaddressed. Why are you using a kwargs dict instead of just passing the parameters to the function? Also, could you please disclose AI usage for this PR?

@PragnyaKhandelwal
Copy link
Copy Markdown
Contributor Author

One comment left unaddressed. Why are you using a kwargs dict instead of just passing the parameters to the function? Also, could you please disclose AI usage for this PR?

Well good catch, @wmvanvliet. I’ve switched from kwargs passthrough to explicit parameter passing to match MNE style.
Also, yes, I used Gemini as drafting assistance for parts of this PR, then manually reviewed/edited the final changes. I should have disclosed this earlier and will do so clearly in future PRs.

@PragnyaKhandelwal
Copy link
Copy Markdown
Contributor Author

At this point, only ci/circleci: build_docs is failing (looks like a CI timeout during docs build), while the other checks are passing. Could you please re-run that job when convenient?

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.

Unicode support in the method add.text() of mne.viz.brain

3 participants