-
-
Notifications
You must be signed in to change notification settings - Fork 422
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
QColoredSVGIcon class for dynamic colorized svg icons #2279
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2279 +/- ##
==========================================
- Coverage 67.06% 67.05% -0.01%
==========================================
Files 400 402 +2
Lines 33648 33744 +96
==========================================
+ Hits 22565 22628 +63
- Misses 11083 11116 +33
Continue to review full report at Codecov.
|
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.
Looks good to me, a few question for my personal culture.
---------- | ||
color : str, optional | ||
A valid CSS color string, used to colorize the SVG. If provided, | ||
will take precedence over ``theme``, by default None. |
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.
will take precedence over ``theme``, by default None. | |
will take precedence over `theme`, by default None. |
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 was my understanding that when sphinx renders our api docs, it uses ReST (even if we're using jupyter-book)... so all of these single backticks will render as italicized, rather than code
. See, for example, all of the italicized "True" in this docstring ... whereas this function uses double backticks and renders correctly. Thoughts?
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.
italicised vs verbatim is a style discussion. Double backticks vs single backticks is semantically distinctions.
If you use single backticks sphinx should be able to infer that theme
is a parameter (potentially adding scrolling anchors to hat parameter); not if you use double.
If italic is wrong, then IMHO the CSS/Rendering/Sphinx should be changed, not the docstrings.
I personally care as the terminal renderer/docstring fixer I write need the correct semantic to find typos. It will ignore double-backticks as it's supposed to be verbatim. (well i could /should inspect verbatim and tell user when they might be wrong... but that's another discussion).
If you still like double backticks I'm fine but you should likely change `theme_key`
to double backticks line 71.
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.
(note, I personally also hate all those things in italic in this theme and in numpy documentation in general, and agree with you that monospace font is better suited, but I believe this to not be relevant for the argument)
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.
I 100% agree with your arguments here. Semantics and syntax are definitely being muddied here, and that poses a problem for the larger goal of having truly parseable docstrings. I looked into it a bit and it looks to be an issue that comes up in a lot of repos. Looks like our current sphinx docs will render single backticks as <cite>content</cite>
(whereas true italics is <em>
as expected) and double backticks with the <span class="pre">content</span>
, so we can definitely attack this on the CSS side. I'll make a new issue for this, since this is all over the repo and much larger than this PR
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.
created #2286
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 looks great to me @tlambert03
We can't quite use this to drop the whole build_icons thing... because we can't use these icons in our qss stylesheets without registering them as resources with the Qt Resource system (I looked into it, but it's trickier). It could come very much in handy if we wanted to allow plugins to provide un-compiled SVGs that we can match to the theme.
I very much like this idea!! Would be very nice if we could achieve that
Description
This is a handy subclass of
QIcon
that will let us create colored SVG icons on the fly, using any specified color, or using colors from the theme. WhileQIcon
already directly supports reading from SVG, it requires a file and not memory buffer... so this lets us modify and colorize the SVG string without intermediate files. It caches previous calls so it doesn't reproduce a colored icon twice.It's not hooked up to anything, but I came across the need for this when working on layergroups, and I think it could generally come in handy. We can't quite use this to drop the whole
build_icons
thing... because we can't use these icons in our qss stylesheets without registering them as resources with the Qt Resource system (I looked into it, but it's trickier). It could come very much in handy if we wanted to allow plugins to provide un-compiled SVGs that we can match to the theme.Basic usage:
Type of change
References
How has this been tested?
Final checklist: