napari / napari

napari: a fast, interactive, multi-dimensional image viewer for python
https://napari.org
BSD 3-Clause "New" or "Revised" License
2.1k stars 410 forks source link

Add hook specification to add new icons and stylesheets #2900

Closed lukasz-migas closed 2 years ago

lukasz-migas commented 2 years ago

This PR adds new (experimental) hook specification napari_experimental_provide_theme to provide a way to add new themes, new icons (or replace existing ones) and add extra stylesheets. Looking through the codebase I could not find an easy way to modify stylesheets or easily add extra icons (I need this for the 1d plugin I am working on to add new layers) so I thought adding plugin specification might be one way to do it.

The new hook simply returns a list of stylesheet paths, list of svg icon paths and dictionary of theme(s) which are then added in appropriate places and used to generate the _qt_resources_...py file.

Type of change

How has this been tested?

Some tests are failing on my machine but I think it's unrelated to my changes.

Final checklist:

Somewhat addresses #936.

codecov[bot] commented 2 years ago

Codecov Report

Merging #2900 (fb9bcee) into main (ca2595d) will increase coverage by 0.16%. The diff coverage is 97.53%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2900      +/-   ##
==========================================
+ Coverage   82.50%   82.66%   +0.16%     
==========================================
  Files         542      546       +4     
  Lines       45187    45683     +496     
==========================================
+ Hits        37282    37765     +483     
- Misses       7905     7918      +13     
Impacted Files Coverage Δ
napari/_qt/qt_resources/_icons.py 67.40% <66.66%> (-5.67%) :arrow_down:
napari/plugins/_plugin_manager.py 88.20% <97.01%> (+2.17%) :arrow_up:
napari/_qt/_tests/test_plugin_qss.py 100.00% <100.00%> (ø)
napari/_qt/qt_event_loop.py 71.11% <100.00%> (+0.65%) :arrow_up:
napari/_qt/qt_resources/_tests/test_icons.py 100.00% <100.00%> (ø)
napari/plugins/_tests/test_provide_icons.py 100.00% <100.00%> (ø)
napari/plugins/hook_specifications.py 100.00% <100.00%> (ø)
... and 3 more

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 ca2595d...fb9bcee. Read the comment docs.

tlambert03 commented 2 years ago

Awesome! Definitely something we've wanted. Will have a look soon!

lukasz-migas commented 2 years ago

more work is needed to integrate with preferences. currently, the new theme won't show up in the preferences window... and if you set it programatically... then the preferences window fails to open. (this may well be due to some poor assumptions in the preferences window... but needs to be addressed).

This is true and I've noticed this happened. Its actually true to one of the examples. Perhaps it would be beneficial to refactor napari's theme from dictionary to an evented model with more standardized schema. For one, it could make changing theme colors reactive and help with theme validation (make sure that correct keys/values are provided).

It probably would also help avoid some future problems like here where the dark/light theme is hardcoded but that might not be appropriate if the user has completely different selected.

sofroniewn commented 2 years ago

Perhaps it would be beneficial to refactor napari's theme from dictionary to an evented model with more standardized schema. For one, it could make changing theme colors reactive and help with theme validation (make sure that correct keys/values are provided).

It probably would also help avoid some future problems like here where the dark/light theme is hardcoded but that might not be appropriate if the user has completely different selected.

I'd be supportive of this. I know that at the time @jni had a preference for keeping it just a dict, but I think we've got more experience now with the evented model and it might well be worth revisiting this idea. Curious what others think

jni commented 2 years ago

I'd be supportive of this. I know that at the time @jni had a preference for keeping it just a dict, but I think we've got more experience now with the evented model and it might well be worth revisiting this idea. Curious what others think

Yup, I've enjoyed Pydantic stuff, happy to make it an evented model internally and use from_dict or similar to still accept dicts as input.

lukasz-migas commented 2 years ago

This is looking great! Thanks for adding the evented dict too!! I just left a couple minor comments. Let us know if this is ready for full review

Thanks! It's not quite ready for review yet as I am still working out what should happen when user installs/uninstalls plugins (e.g. addition of new theme, removal of stylesheet etc) and I also consistently get segfault whenever I update the icon value of the theme data (other keys are fine). Whenever the color of icon is changed, we have re-generate icons with new, updated color and then update the resources which seems to cause the crash. It works the first time but crashes napari the second time.

lukasz-migas commented 2 years ago

@sofroniewn @tlambert03 I think this PR is in much better place now so please feel free to review. The scope has changed a little since now there are three separate hook specs: one for providing theme colors (historic), svg icons and qss stylesheets.

All themes provided by the napari_experimental_provide_theme are now discovered as napari starts-up. The color theme is now an evented model with color attribute validation using Pydantic's Color class. Nearly all Theme attributes are now reactive, meaning that you can rapidly adjust the style of napari. Majority of the attributes simply require update to the stylesheet with the exception of icon which needs to regenerate all icons with the new color. Changes to canvas and console are now also reflected. Obviously, theme data is not yet saved in the settings file so these changes are not permanent. Whenever a new plugin is added, any theme provided by that plugin is added to the _themes dictionary. Anytime the plugin is uninstalled or disabled, it is removed from there.

The napari_experimental_provide_qss and napari_experimental_provide_icons hooks provide means of adding stylesheets and icons. These are not historic, meaning that they can be discovered in the plugin sorter. My reasoning for this is that it would be quite nice to install several plugins that provide icons/stylesheets/themes but then able to reorganize them in the sorter so the plugins can overwrite existing look.

For stylesheets, this turned out to be a bit harder than I anticipated due to the way napari uses stylesheets. By default, there are three stylesheets with names such as 00_base.qss or 02_custom.qss which are always sorted before they are compiled. Whatever comes last will overwrite previous style but since plugins could provide any name such as 99_custom the order in the plugin sorter might have no effect. I am not entire sure how to solve this yet.

For icons, this is slightly easier since there is a ICONS dictionary that provides mapping of icon name -> icon path and if two plugins provide a new icon for the same icon name then the last one will be the one that is used.

A consequence of moving to a pydantic-based model is that the get_theme method would now return a Theme instance which is not expected in many places and plugins (e.g. napari-console). In order to not cause havoc, the get_theme function will still return a dictionary which can be turned off by specifying as_dict=False when calling the method. I reckon we could put a FutureWarning here.

tlambert03 commented 2 years ago

amazing stuff, thanks! will have a look soon

lukasz-migas commented 2 years ago

FYI this is the new theme behaviour.

evented-theme

tlambert03 commented 2 years ago

so slick!!! 😍

sofroniewn commented 2 years ago

All themes provided by the napari_experimental_provide_theme are now discovered as napari starts-up. The color theme is now an evented model with color attribute validation using Pydantic's Color class.

This is incredible! Love the new behaviour. I'd probably be ok with calling the hookspec napari_provide_theme too. I think this hookspec will probably be pretty stable.

The napari_experimental_provide_qss and napari_experimental_provide_icons hooks provide means of adding stylesheets and icons. These are not historic, meaning that they can be discovered in the plugin sorter. My reasoning for this is that it would be quite nice to install several plugins that provide icons/stylesheets/themes but then able to reorganize them in the sorter so the plugins can overwrite existing look.

Hmm ok, I do have some slight concerns here that this could lead to incompatibilities between plugins. For the icons in particular, it's not clear to me that we actually want a plugin to overwrite builtin icons. I think I'd be ok if things were additive and namespaced, but not sure it will be a good long term user experience if things can be overwritten.

This PR is coming along really well though @lukasz-migas and will be so exciting to get in. If this is a huge pain, then you don't have to do it, but one thought is that we split this PR into two, one with the napari_provide_theme hookspec and all the changes to the theme as an EventedModel, and then one with the icon and stylesheet hookspecs. It might be easier for us to merge and review as such. Let me know your thoughts? As always great work!!

tlambert03 commented 2 years ago

For the icons in particular, it's not clear to me that we actually want a plugin to overwrite builtin icons. I think I'd be ok if things were additive and namespaced, but not sure it will be a good long term user experience if things can be overwritten.

@sofroniewn, by "overwrite" do you mean here that you only want plugins to be able to provide icons for their own buttons, but not modify the napari buttons? or something more subtle? I feel like icon packs are a pretty common extensions concept (for instance, I use and love non-default icons in vscode)

sofroniewn commented 2 years ago

@sofroniewn, by "overwrite" do you mean here that you only want plugins to be able to provide icons for their own buttons, but not modify the napari buttons? or something more subtle? I feel like icon packs are a pretty common extensions concept (for instance, I use and love non-default icons in vscode)

I was just worried about ending up in an odd mixed/ matched situations - but looking at how vscode does it for inspiration here seems wise, so allowing "icon packs" like that seems fine

tlambert03 commented 2 years ago

I was just worried about ending up in an odd mixed/ matched situations

I guess I still generally feel like, as long as the API for supplying icons is clear, and it's clear how a user can enable and disable plugins that override the icons, it's not for us to decide how they like their app to appear :) (though, we absolutely should slave away over the "default" look and feel of the app.)

edit: furthermore... if we're not exposing the ability to overwrite the builtin icons, then we really don't need a hookspec at all! Developers already have the ability to "add" icons using the compile_qt_svgs convenience function, and an example is provided here in our examples

sofroniewn commented 2 years ago

Poking around the vscode-icons there are definitely a lot of nice packages there that overwrite builtin icons, so I'm fine with that - but I think it's worth understanding how they deal with priority, custom icons, conflicts, overwriting, etc (right now I don't know how they do it, but i'm sure they have good patterns we can reuse).

it's not for us to decide how they like their app to appear :)

My concerns really are not about dictating how people want their app to appear - I'm more concerned that they want it to appear a certain way, but we then end up doing something strange with how we let icons (or stylesheets) get added/ overwrite each other that puts things in a state no one wants to use (some icons from plugin A, some from B, some from C, but really should only be from B) and that their only option is to turn certain things off which prevents them from using functionality they want (A is now missing) or both A and B have used same icon name for very different types of thing and so adding B makes it very confusing whenever anyone wants to use A if B overwrites A. Maybe we're not in danger of this and my concerns were more "theoretical", but it's not about us telling people how things "should" look, it's about ensuring that it does actually look the way everyone wants it too, which may be impossible though ;-)

if we're not exposing the ability to overwrite the builtin icons, then we really don't need a hookspec at all!

I don't agree with this - one of the major points of hookspecs is that they allow plugins to provide things without needing to depend on napari and do imports like from napari.qt import compile_qt_svgs, and being able to provide new icons through such a system would still be valuable, even if it was all that could be done.

I think though that this back and forth about icon overwritting has reached the end of its utility though. When I made my initial comments I really didn't know anything about how vscode did custom icons, and while I still think there could be some complexity here, looking through their packages has convinced me of that they are solvable (so thanks @tlambert03 for that initial link). All in all, I'm fine with icons being overwritten by plugins and if we run into odd behaviors / corner case situations we can address them as they come up.

I do still think it might be nice to split this PR into two @lukasz-migas for ease of review and merge, but really up to you if you don't want to do it.

tlambert03 commented 2 years ago

but we then end up doing something strange with how we let icons (or stylesheets) get added/ overwrite each other that puts things in a state no one wants to use

yeah, that's definitely undesireable... It's certainly up to us to implement this in a non-crappy way such that you can enable or disable a plugin. I'd vote for it being all-or-none, in the sense that you can either have a plugin icon pack enabled (with unprovided once falling back to builtins). Not some complicated cascade...

one of the major points of hookspecs is that they allow plugins to provide things without needing to depend on napari and do imports like from napari.qt import compile_qt_svgs, and being able to provide new icons through such a system would still be valuable, even if it was all that could be done.

sorry, don't want to belabor this, but I do think this is a pretty interesting/deep point. I think of hooks like the name implies: we offer you a hook into some of the internal workings of napari. you can hook into the image io process, you can hook into adding something to our menus, etc... I think it's a nice design goal to generally avoid forcing a napari dependency (particularly with something as generic as file IO). but once we get to the point of compiling SVGs for use in a Qt-only program, where those SVGs correspond to specific functions in napari ... then I feel like it's possible to take the concept of not using napari to an illogical extreme. compile_qt_svgs is just a convenient way to dynamically compile SVGs (it could go in superqt if you really hated them needing napari)... but if they're not overriding our icons, they're not "hooking" into anything. (i.e., it's just a regular python function)

sofroniewn commented 2 years ago

I'd vote for it being all-or-none, in the sense that you can either have a plugin icon pack enabled (with unprovided once falling back to builtins). Not some complicated cascade...

Yes that could be very nice, though other plugins should maybe still be able to add their own in a name-spaced way (like their logo for example, they will likely be the only people to provide it, but it might live at my_name_name/logo.svg

but once we get to the point of compiling SVGs for use in a Qt-only program, where those SVGs correspond to specific functions in napari ... then I feel like it's possible to take the concept of not using napari to an illogical extreme. compile_qt_svgs is just a convenient way to dynamically compile SVGs (it could go in superqt if you really hated them needing napari)... but if they're not overriding our icons, they're not "hooking" into anything.

I still disagree, first off imagine one day - we split the repo into a pydantic evented model part with our layers and components and then allow multiple front ends, one based on Qt, and one based on web technology then having just icons provided by a hookspec would allow those icons to keep on getting used, but if people are using compile_qt_svgs then that won't be possible. Maybe we should accelerate the splitting of the repo in this way so it becomes less of a theoretical concern and something that is actually possible. For now my attitude though has been to favor the more general approach where possible, but maybe it will be cleaner to just make the split.

Secondly, it will likely always be much cleaner from a dependency management standpoint if plugins don't depend on napari - even if they will only ever be used by napari - to ensure things like robust installation into a bundled app. I don't want us to get into situations where plugins pin to different versions of napari and then can't be installed into the bundle together, so reducing the need for dependency on napari always seems useful irrespective of how that plugin will get used.

Maybe we should open a new issue on "whether plugins should depend on napari" if you want to discuss this more.

tlambert03 commented 2 years ago

fair enough! :)

lukasz-migas commented 2 years ago

This PR had stimulated a lot more discussion than I anticipated!

[sofroniewn] Poking around the vscode-icons there are definitely a lot of nice packages there that overwrite builtin icons, so I'm fine with that - but I think it's worth understanding how they deal with priority, custom icons, conflicts, overwriting, etc (right now I don't know how they do it, but i'm sure they have good patterns we can reuse).

I believe vscode allows selection of a single icon pack at a time. The extension developers define their icon packs using pre-defined json file described here. Perhaps my initial plan of being able to have priority-like behaviour was not so wise after all since it could hypothetically create a real mess without necessarily knowing where it's coming from.

[tlambert03] yeah, that's definitely undesireable... It's certainly up to us to implement this in a non-crappy way such that you can enable or disable a plugin. I'd vote for it being all-or-none, in the sense that you can either have a plugin icon pack enabled (with unprovided once falling back to builtins). Not some complicated cascade...

I agree, having thought about this, perhaps the qss and icons hooks should be made historic, meaning that they will be able to provide an easy way for plugin devs to add their icons/stylesheets without necessarily overriding the default look and feel of napari. Then we would have an easy way of keeping track of what each plugin is providing and they could have their own namespace. And then in the future, we could develop another hook spec for iconpacks?

[sofroniewn] I do still think it might be nice to split this PR into two @lukasz-migas for ease of review and merge, but really up to you if you don't want to do it.

This is certainly doable – it’s a bit of work but I doubt it will take much time. I will split it this afternoon.

sofroniewn commented 2 years ago

I believe vscode allows selection of a single icon pack at a time. The extension developers define their icon packs using pre-defined json file described here. Perhaps my initial plan of being able to have priority-like behaviour was not so wise after all since it could hypothetically create a real mess without necessarily knowing where it's coming from.

I agree, having thought about this, perhaps the qss and icons hooks should be made historic, meaning that they will be able to provide an easy way for plugin devs to add their icons/stylesheets without necessarily overriding the default look and feel of napari. Then we would have an easy way of keeping track of what each plugin is providing and they could have their own namespace. And then in the future, we could develop another hook spec for iconpacks?

Ok, good thoughts @lukasz-migas that makes sense and I'm very open to that approach, but I still honestly don't know what's best here - but somehow plugin A should be able to contribute a custom icon that will only be used inside it's own widget, plugin B should also be able to overwrite all existing icons into some cool style and a user that wants to use plugin B's icons should still be able to use the dock widget from plugin A with it's custom icon.

The thing I didn't know how to handle is if plugin A also overwrites some icons then what happens, do you get a mix of plugin B icons and plugin A icons? Maybe that's ok, but it sort of means that if you provide new icons then and overwrite icons you might get into a funny state.

I still can't figure out how vscode does this - do they distinguish between "new icons" and "builtin icons"?

I guess if we had two hookspecs, one for adding new, one for iconpacks we could solve this? Or we just have the one and if weird things happen its ok, but we should advise devs on how to avoid those cases as I'm worried people might do things without realizing

Really apologies though if I'm just not thinking about this right, I don't think what I'm describing is that pathological, but I could just be missing something basic here. What do you think @tlambert03? - I really trust your opinion here, you've thought more about this in VSCode and other places than I have.

tlambert03 commented 2 years ago

I suspect namespaces will likely be handy here (as usual). The Qt resource system already has a builtin concept of namespaces with the "prefix" in the qrc file. The compile_qt_svgs function that I wrote also accepts a prefix argument (and will compile the provided SVGs under that prefix). What the prefix means in practice is that, in a QSS file or in the path passed to a QIcon constructor, your file path must be accessed as if it was in the folder prefix. For instance, if prefix='plugin_name', and one of the icons is "my_icon.svg", then you could access the compiled resources at :/plugin_name/my_icon.svg. The napari theme icons are currently (historically) prefixed with themes. But now might be a good time to step back and plan out namespaces here. When it's been just us, it's easy enough to just add an icon to the SVG folder and then use in QSS, but if we want to be careful here (and I think we should), we could declare a list of icons, and their description, making it clear what an "icon pack" should provide...

zoom.svg - button that selects pan/zoom mode in the viewer
new_points.svg - the icon representing a Points layer.

and then, somehow, the hook implementation would need to indicate that this was indeed an icon intended to override the napari icon:

napari_icons = {
    'napari.icons.zoom' : 'my_zoom.svg',
    'napari.icons.points' : 'my_points.svg'
}

icons that we're just compiling for them as a service, that aren't actually altering internal napari behavior, I'm less clear about. but I guess they could just give us a list of icon names,

svgs = ['icon_a.svg', 'icon_b.svg']

and then they'd be allowed to access them under their plugin namespace (but note: this is still more complicated because of the way that we also provide "coloration" services for the icon... so that also has to somehow be a part of the path)

icon =QIcon(":/my_plugin/icon_a.svg")

anyway, I'm not particularly thrilled with the actual namespaces I used here, or the implementation... but I think that namespaces in general are probably helpful here

sofroniewn commented 2 years ago

The napari theme icons are currently (historically) prefixed with themes. But now might be a good time to step back and plan out namespaces here.

Yup for sure, happy to revisit that one

anyway, I'm not particularly thrilled with the actual namespaces I used here, or the implementation... but I think that namespaces in general are probably helpful here

Ok yeah, agreed this direction is probably a good one. We can also look @liaprins-czi into this convo too (it's quite long @liaprins-czi so the tldr is that we're thinking about how best to let plugins provide either new icons or overwrite existing icons. There is some good precedent for this stuff in vscode extensions, but definitely still some tricky things we might need to resolve!!

liaprins-czi commented 2 years ago

Hmm so is the question whether we would allow them to do that? Or how they would supply the icons? Just curious hwo I cna best weigh in from design POV. Thanks!

sofroniewn commented 2 years ago

Hmm so is the question whether we would allow them to do that? Or how they would supply the icons? Just curious hwo I cna best weigh in from design POV. Thanks!

I think we probably need to let people both provide new icons and have some ability to overwrite builtin icons, but open questions seem to be around if at any one moment in time you're just using one set of icons that will be overwriting the existing ones - I believe we're referring to this as an "iconpack" and at anyone time it sounds like you just have one "iconpack" active. But for people extending the UI do we still need to let them add their own icons that only they want to use? Or should we think about these two things - "iconpacks" and "custom icons" as going through two different paths, serving two different functions etc

liaprins-czi commented 2 years ago

Hmm so is the question whether we would allow them to do that? Or how they would supply the icons? Just curious hwo I cna best weigh in from design POV. Thanks!

I think we probably need to let people both provide new icons and have some ability to overwrite builtin icons, but open questions seem to be around if at any one moment in time you're just using one set of icons that will be overwriting the existing ones - I believe we're referring to this as an "iconpack" and at anyone time it sounds like you just have one "iconpack" active. But for people extending the UI do we still need to let them add their own icons that only they want to use? Or should we think about these two things - "iconpacks" and "custom icons" as going through two different paths, serving two different functions etc

Yeah, there are definitely complications around letting people use their own custom icons, and I don't see it being a good choice to allow so from a usability perspective. The icons we have should (and will be eventually, there is a heuristic issue for this) be optimized to communicate the meaning. And if we add new buttons and / or icons in the future, after a custom theme has been made, it will not be able to have the new icon or else it will use the one core napari has anyway (which would be better than not having one, even if it doesn't "match" their theme).

sofroniewn commented 2 years ago

Yeah, there are definitely complications around letting people use their own custom icons, and I don't see it being a good choice to allow so from a usability perspective. The icons we have should (and will be eventually, there is a heuristic issue for this) be optimized to communicate the meaning. And if we add new buttons and / or icons in the future, after a custom theme has been made, it will not be able to have the new icon or else it will use the one core napari has anyway (which would be better than not having one, even if it doesn't "match" their theme).

Yeah, these are some of my concerns too. We've got some immediate need for them now to get our 1D plotting in, but we might need to refine this as we get further along.

As is done in this PR I think it makes sense to use the experimental word in the hookspec to indicate that this is part of the plugin interface that might change as we go. So my attitude would be to press on as an experimental feature, but keep all these considerations in mind as we go!

liaprins-czi commented 2 years ago

Yeah the experimental version sounds good to me, also I hadn't considered from POV of pulgins needing their own custom specific buttons to use... that would make sense to allow I think, actually! I had only been thinking of theming plugins that change the "skin" of napari viewer...

lukasz-migas commented 2 years ago

Yeah the experimental version sounds good to me, also I hadn't considered from POV of pulgins needing their own custom specific buttons to use... that would make sense to allow I think, actually! I had only been thinking of theming plugins that change the "skin" of napari viewer...

@liaprins-czi that was actually the original idea behind this PR to give plugin devs an easy way to provide style sheets for their ui elements and icons for their buttons etc. I might have overcomplicated it a little with the overwrite/priority behaviour which would make it very complex and very messy.

liaprins-czi commented 2 years ago

@lukasz-migas Okay great, thanks for clarifying!

tlambert03 commented 2 years ago

reading more about how vscode does this. They have "product icons", these are

a set of built-in icons that are used in views and the editor, but can also be used in hovers, the status bar, and by extensions.

https://code.visualstudio.com/api/references/icons-in-labels#icon-listing

Untitled 2

so basically, the identifier is a specific usage of an icon (like, "account icon in the view bar"). VScode provides a default icon set ("codicons"), each of which have a id. These are the ones that are "optimized to communicate the meaning", but icon packs can offer up a different interpretation (which, again, I think is desirable. I don't think we should assume we can always pick/provide the optimal icons for everyone's use case).

neuromusic commented 2 years ago

(aside: I can't wait for the first "winamp" theme for napari)

lukasz-migas commented 2 years ago

I've made a couple of changes to this PR to now use namespaces when adding icons and stylesheets. Plugins that use the napari_experimental_provide_icons hook will now register icons under the PluginName:IconName.svg and therefore won't override any existing icons. Same situation applies to stylesheets where they are also registered with the PluginName: in their name.

tlambert03 commented 2 years ago

Thanks @lukasz-migas. I'll take a look soon.

I hope this won't be too frustrating, but I just want to link the issue proposing that we move away from supporting SVG icons altogether, in favor of using font-icons (see https://github.com/napari/napari/issues/3136) ... So, I'm a little apprehensive about adding a whole new hook spec for something that I'm kind of hoping won't be relevant in a couple months.
for napari, using fonts instead of svgs will reduce a ton of code, and avoid the need for the compilation step altogether. For plugins, it will let them declare an icon using a simple namespaced string (see example) ... provided that there is some font library out there that has an icon they want to use (generally a safe bet with the thousands available).

lukasz-migas commented 2 years ago

@tlambert03 Thanks for point me to #3136. I haven't looked at it but I love the idea. The compilation step is a bit of a pain and does add a bit of overhead so it would be great to remove it altogether.

nclack commented 2 years ago

HI @lukasz-migas, this PR came up during a review of possibly stale PRs. There's been a lot of discussion here!

Also, we've been working a new plugin engine at tlambert03/npe2. There's a theme section there inspired by the theme hook spec here. I'd love to get your thoughts.

I'm wondering what's left to do with this PR. Should the remaining work be captured in an issue? Should we think about doing it with npe2?

lukasz-migas commented 2 years ago

Hi @nclack!

Indeed this PR had received a lot of discussion and several off-shoot PRs to separate some of the functionality. I am very excited for npe2 when it's available. I've had a look at the link you posted plus the npe2-tester repo and I like what I see! I think the current schema will work well for themes.

With regards to this PR, it provides two specifiactions - one for stylesheets (napari_experimental_provide_qss) and one for providing icons (napari_experimental_provide_icons). As @tlambert03 suggested above, the svg icons will be abandonded in favour of text icons (#3136).

Technically, this PR is ready for review as it implements the functionality and has full suite of tests to cover the changes. I think all (or most) of points of concern from the discussions above were addressed (e.g. namespaces), however, I am not sure if it's beneficial to merge this into the main branch and then have to remove it.

Perhaps the stylesheet spec could be implemented in npe2 (I could give it a go next week) and once the text icons are implemented, there will no need for icon spec since icons will be easily accessible. @tlambert03 would you agree?

tlambert03 commented 2 years ago

Perhaps the stylesheet spec could be implemented in npe2

can you remind me why we need to accept stylesheets in addition to theme colors and icons?

the theme stuff is implemented (but rather different from npe1) here: https://github.com/tlambert03/npe2/blob/main/npe2/manifest/themes.py basically, it's strictly static! no python needed. once we have icon fonts, we can do the same for icons... they'll just be a string pointer to a font char

nclack commented 2 years ago

I am not sure if it's beneficial to merge this into the main branch and then have to remove it

I agree. I'll close this PR and open a separate issue (#3557) around stylesheet support in napari. I suspect the discussion around customized stylesheets may be pretty involved. I'd love to get your comments on that issue motivating custom qss specifically.