-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
dynamically finding the backend preferred format for button images #9811
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
lib/matplotlib/backend_bases.py
Outdated
""" | ||
Get the preferred image extension | ||
|
||
Return |
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.
Returns.
lib/matplotlib/backend_bases.py
Outdated
if os.path.isfile(fname): | ||
return fname | ||
|
||
def btn_image_extension(self): |
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.
"preferred_image_extension"?
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.
prone to confusion?
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.
It is not actually a preference, it is matter of "does it work?"
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.
supported_...
(it's not as if you have to repeat this name again and again many places through the codebase so you may as well avoid abbreviating it).
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.
could we get away with just having a class-level attribute of _icon_extension
?
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.
probably would work too...
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.
That was my first idea, but I didn't like creating a property with a raise AttributeError
in the base class.
But i guess you're right, it is alot easier and cleaner for the final implementation of the backend
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.
Make it None on the base class which will make it raise a TypeError when fetching the attribute if not overridden?
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.
Or default to 'png'
?
FWIW recent tkinters should support png out of the box (https://stackoverflow.com/a/27601573/378080). |
@tacaswell I changed it to a class attribute as suggested, I think it is cleaner this way |
Repro'ing my comment that got folded when the code was patched away:
|
I think it is ok to leave it as |
Done!, added a default format |
I think this will need a manual rebase against current |
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.
Rebasing on current master so that circle bulids would be good, but not going to hold the PR over that.
PR Summary
The Tools used by Toolmanager assumed every backend understood
.png
files for the button images.This is not the case, for example tk works with png in linux but not on mac
This pr fixes this by asking each backend for it's preferred image format
PR Checklist