Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Fix semantics of MEP22 image names.#27492
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Seems a bit overkill, but 👍🏻 I think we should always return an absolute path. Minor preference we move to pathlib internally but 🤷🏻 either way. |
See writeup in changelog note.
a205ac3
to8470189
CompareSwitched 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). |
QuLogic left a comment• 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.
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 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 affectipympl
.
I don't think it does? Both backends only implement the classic toolbar2, not mep22 toolmanager. |
Uh oh!
There was an error while loading.Please reload this page.
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.
Indeed, you are correct about that. Looks good to me then.
drmcnelson commentedMar 14, 2024
Thank you |
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