Skip to content

Fix semantics of MEP22 image names.#27492

Merged
timhoffm merged 1 commit intomatplotlib:mainfrom
anntzer:mep22imagename
Mar 14, 2024
Merged

Fix semantics of MEP22 image names.#27492
timhoffm merged 1 commit intomatplotlib:mainfrom
anntzer:mep22imagename

Conversation

@anntzer
Copy link
Copy Markdown
Contributor

@anntzer anntzer commented Dec 10, 2023

See writeup in changelog note. Closes #27400.

@tacaswell I think the behavior here also handles the use case you mention at #27400 (comment).

PR summary

PR checklist

@anntzer anntzer added the MEP: MEP22 tool manager label Dec 10, 2023
@tacaswell
Copy link
Copy Markdown
Member

Seems a bit overkill, but 👍🏻

I think we should always return an absolute path. Minor preference we move to pathlib internally but 🤷🏻 either way.

@tacaswell tacaswell added this to the v3.9.0 milestone Dec 11, 2023
See writeup in changelog note.
@anntzer anntzer force-pushed the mep22imagename branch 2 times, most recently from a205ac3 to 8470189 Compare December 11, 2023 22:19
@anntzer
Copy link
Copy Markdown
Contributor Author

anntzer commented Dec 11, 2023

Switched to returning an absolute path. Like most people(?) I also prefer pathlib, but the return value immediately gets passed to add_toolitem, which is documented to take a str as input (and is technically something overridable by third-party backends) and I don't really want to bother changing that (especially as the filename will typically then get passed to a gui toolkit code to load the icon, and these typically don't actually support pathlib but plain str paths). So overall in this specific case I'm not convinced it's worth switching to returning a Path that'll immediately get converted back to a str (but I don't feel strongly either way).

Copy link
Copy Markdown
Member

@QuLogic QuLogic left a comment

Choose a reason for hiding this comment

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

This will break webagg, which expects an image basename (which is mapped to a FontAwesome icon as a class name), which doesn't appear to be stripped of the new prefix here. It may also affect ipympl.

@anntzer
Copy link
Copy Markdown
Contributor Author

anntzer commented Dec 12, 2023

I don't think it does? Both backends only implement the classic toolbar2, not mep22 toolmanager.

Copy link
Copy Markdown
Member

@QuLogic QuLogic left a comment

Choose a reason for hiding this comment

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

Indeed, you are correct about that. Looks good to me then.

@timhoffm timhoffm merged commit fef38dd into matplotlib:main Mar 14, 2024
@drmcnelson
Copy link
Copy Markdown

Thank you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

MEP: MEP22 tool manager

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: tk backend confused by presence of file named "move" in current working directory

5 participants