Unify icon contributions and clarify supported formats#450
Conversation
willingc
left a comment
willingc
left a comment
There was a problem hiding this comment.
Looks good. Seems to have a circular import that in the tests.
|
Since it's not reused, I just moved the validator to the model where it's needed. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #450 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 37 37
Lines 2795 2802 +7
=========================================
+ Hits 2795 2802 +7 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| PluginManifest(name="myplugin", icon="my_plugin:myicon.png") | ||
| pm = PluginManifest(name="myplugin", icon="my_plugin:myicon.png") | ||
| assert pm.icon == "my_plugin:myicon.png" | ||
| pm = PluginManifest(name="myplugin", icon="https://example.com/icon.png") |
There was a problem hiding this comment.
I don't think that we should allow remote icons in plugins. This is a big privacy issue.
There was a problem hiding this comment.
Actually, this is a really important point, it let's someone track things, like the 1px by 1px white image in your emails.
There was a problem hiding this comment.
Ok, I agree. I was just trying to keep previous "behaviour" :)
|
Ok, urls no longer accepted. |
There was a problem hiding this comment.
Implementation looks good to me
Can we get some documentation in napari/docs about the "ship with sdist" bit. I think src/plugin/some_module/icon.svg is not by default included in the sdist because it is not a .py
This will result in plugin developers locally seeing everything as ok (because their -e install contains it) but then the wheel installed version would be wrong.
But also, maybe we shouldn't reference sdist here? Wheels are always built with the sdist (AFAIK), but that's still managed via pyproject.toml not MANIFEST.in. So maybe we just link to a good packaging resource that talks about how to include files that are not .py files in builds Ok now I'm a bit confused, has this changed? In ndevio I have .tiff and .png's as sample data but looking closely I did absolutely nothing to include them in the wheel... hmmm, no MANIFEST and nothing in pyproject
Have things changed?
It is because you use |
TimMonko
left a comment
TimMonko
left a comment
There was a problem hiding this comment.
Was about to merge and wanted to double check the logic because I was trying to update the PR description properly. Looks like maybe we dropped fonticon compatibility? And, I guess I'm not seeing where we block urls now?
I could just be having a brain fart...
| assert isinstance(v, str), f"{v} must be a string" | ||
| return v | ||
| def _ensure_valid_resource(value): | ||
| if ":" not in value: |
There was a problem hiding this comment.
does this change not invalidate using the fonticon key method? AFAICT, this makes it so only package resources would not raise the value error.
|
Mmm you're right... The problem is that validating the manifest should not necessarily require that the resources are present or the fonticon plugin is isntalled, so it's hard to properly check this... I guess we can check that a |
|
I think it makes sense to validate still, especially to intentionally block urls. |
|
So, good news is I updated the validator and now it should handle all the cases... bad news is: we actually have url icons in tests, which made me realize: this is a breaking change in theory... do we need to migrate? Do we need to check if plugins in the wild are setting the icon? |
I will check manifests in npe2api. If you don't hear from me by the time you wake up tomorrow send me a DM |
resolves some questions that I had about submodules and commands/submenus in validator
|
Ok, so I fixed the tests (the sample napari.yaml was still using a url) and I added some more things to Checking on the icon usage in npe2api now |
|
Not a single plugin scraped by npe2api declares an icon except napari-trackastra, and it uses There are five other instances that meet this regex, but all are colors used for the three theme plugins that exist: napari-gruvbox, napari-solarized, ndev-themes So as long as our conda-only plugins don't do this we are good to go. But the conda-only plugin authors seem to be quote responsive these days. |
TimMonko
added
the
ready to merge
Hold on, this won't work now! It needs to be Maybe we can contact them and open a PR. |
|
What is currrent status. Three plugins that are listed by @TimMonko are @brisvag, @aganders3 and @TimMonko plugin? Could we merge this PR and make release, or should we wait? |
The only one of concern is napari-trackastra `"icon": "resources/icon.png", as @brisvag says #450 (comment) The theme plugins are unaffected by this, I was mostly just showing that I did do diligence to look for "icon" (which themes change but are unaffected by this issue) I think our best way forward is contact napari-trackastra and merge this PR |
|
I'll do the contacting, feel free to merge! |
5 participants
This PR aims to unify a bit the manifest regarding icons, and to bring it more in line with what the underlying app-model accepts.
We actually "officially" support more things than appmodel (e.g: https urls), and I'm not sure whether we plan to keep these and handle the extra logic ourselves or what else to do. At least with this we start supporting dark/light for all icon fields.