-
Notifications
You must be signed in to change notification settings - Fork 38
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
feat: add QIcon backed by iconify #209
Conversation
Codecov ReportAll modified lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #209 +/- ##
==========================================
+ Coverage 87.38% 87.42% +0.03%
==========================================
Files 45 46 +1
Lines 3346 3356 +10
==========================================
+ Hits 2924 2934 +10
Misses 422 422
☔ View full report in Codecov by Sentry. |
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 looks ok. Few minor remarks.
Did you plan to ship pyconify to conda?|
Why not allow to cache icons svg between sessions?
They are cached between sessions: The main difference between temp_svg and svg is that temp_svg returns a file path and svg returns bytes. But they both use the same persistent cache yes I’ll put it on conda |
Co-authored-by: Grzegorz Bokota <[email protected]>
Co-authored-by: Grzegorz Bokota <[email protected]>
Hm. Did you try to play with |
Yep, I did quite a bit. I originally started with a similar strategy to what i did in napari: https://github.com/napari/napari/blob/main/napari/_qt/qt_resources/_svg.py However, after looking into more in this round, I'm less of a fan of that approach. If you look into the c++ code of the builtin QIconEngine, you'll find that it actually does quite a bit more than a naive approach might, it handles caching of pixmaps for various sizes, and better handles rendering svgs for different states (enabled/disabled, active/inactive). So, after a fair amount of experimentation, I decided that simply writing a file to disk and letting the builtin QIconEngine handle it is well worth the cost. It still all loads in about 10µs (after the original caching from the API), so it's hardly a performance limitation. And there's a large amount of code that no longer needs to be maintained. So that's why I went with this approach. I will, however, update pyconify to be able to give you the path to the actual cached file in the pyconify cache (rather than this current approach, which costs one additional local file copy from the pyconify cache to the tempdir) |
@Czaki, will likely merge this soon. Would welcome your thoughts on |
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.
As far as I check the svg_path
is improperly uses cache. I have created PR to fix this: pyapp-kit/pyconify#5
I have two small remarks (one typo, and one for test). But general image is good.
Co-authored-by: Grzegorz Bokota <[email protected]>
thanks @Czaki! merged your PR, and bumped the dependency here. |
Iconify is a pretty great API unifying pretty much all of the major font-icon libraries into a single API from which you can request standardized SVGs. I wrapped the API in library
pyconify
that makes it easier to work with in python, and caches all svgs locally for faster/offline usage (after first using online once). This PR makes a simple QIcon wrapper:A downside of using this is that it's SVG based, which makes it a bit harder than font libraries to change colors simply using style sheets (like napari does for its themes... though even that may be possible). A big upside is that you no longer need to include the whole font-family file just to grab a couple icons, and you can pick and choose from all of the libraries without having to bring in a new plugin for each one. Also, for reasons beyond just color changing, SVGs are preferable in many cases to fonts (animations are possible, for example)