Skip to content
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

Merged
merged 12 commits into from
Feb 19, 2021

Conversation

tlambert03
Copy link
Contributor

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. While QIcon 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:

from napari._qt.qt_resources import QColoredSVGIcon
from qtpy.QtWidgets import QLabel

# can also use theme arguments, like: `theme='light'`
icon = QColoredSVGIcon.from_resources('new_points', color='#0934e2')

label = QLabel()
label.setPixmap(icon.pixmap(250, 250))
label.show()

Untitled

Type of change

  • Bug-fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

References

How has this been tested?

  • example: the test suite for my feature covers cases x, y, and z
  • example: all tests pass with my change

Final checklist:

  • My PR is the minimum possible work for the desired functionality
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works

@codecov
Copy link

codecov bot commented Feb 16, 2021

Codecov Report

Merging #2279 (497cc52) into master (78abf1f) will decrease coverage by 0.00%.
The diff coverage is 66.34%.

Impacted file tree graph

@@            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     
Impacted Files Coverage Δ
napari/_qt/qt_resources/__init__.py 51.72% <0.00%> (-0.91%) ⬇️
napari/_vispy/vispy_canvas.py 45.71% <45.83%> (+0.25%) ⬆️
napari/_qt/qt_resources/_svg.py 56.09% <56.09%> (ø)
napari/_qt/qt_viewer.py 53.40% <66.66%> (-0.38%) ⬇️
napari/layers/labels/labels.py 71.76% <66.66%> (-0.09%) ⬇️
napari/_qt/qt_resources/_tests/test_svg.py 100.00% <100.00%> (ø)
napari/layers/labels/_tests/test_labels.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 78abf1f...71adf01. Read the comment docs.

Copy link
Contributor

@Carreau Carreau left a 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.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
will take precedence over ``theme``, by default None.
will take precedence over `theme`, by default None.

Copy link
Contributor Author

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?

Copy link
Contributor

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.

Copy link
Contributor

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)

Copy link
Contributor Author

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

created #2286

napari/_qt/qt_resources/_svg.py Show resolved Hide resolved
napari/_qt/qt_resources/_svg.py Show resolved Hide resolved
napari/_qt/qt_resources/_svg.py Outdated Show resolved Hide resolved
napari/_qt/qt_resources/_svg.py Outdated Show resolved Hide resolved
napari/_qt/qt_resources/_svg.py Outdated Show resolved Hide resolved
napari/resources/_icons.py Show resolved Hide resolved
napari/resources/_icons.py Outdated Show resolved Hide resolved
@sofroniewn sofroniewn added this to the 0.4.6 milestone Feb 18, 2021
@sofroniewn sofroniewn added the feature New feature or request label Feb 18, 2021
@tlambert03 tlambert03 mentioned this pull request Feb 18, 2021
10 tasks
Copy link
Contributor

@sofroniewn sofroniewn left a 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

@sofroniewn sofroniewn merged commit d588f45 into napari:master Feb 19, 2021
@tlambert03 tlambert03 deleted the dynamic-svg branch March 19, 2021 13:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants