matplotlib/napari-matplotlibPublic

NotificationsYou must be signed in to change notification settings
Fork22
Star40

Match plugin text/axis/icon colours to the napari theme.#138

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

samcunliffe merged 8 commits intomatplotlib:mainfromsamcunliffe:86-handling-of-light-themes

Jun 1, 2023

Merged

Match plugin text/axis/icon colours to the napari theme.#138

samcunliffe merged 8 commits intomatplotlib:mainfromsamcunliffe:86-handling-of-light-themes

Jun 1, 2023

Conversation

Copy link

Collaborator

samcunliffe commentedMay 31, 2023•
edited
Loading

Change the text, axes, and icon colour to contrast with the background. I hear white text on an off-white background is not ideal.

Solves

Hard-coded icons and axes looks bad with non-default napari themes #86

Testing:

The theme changing via theviewer.events.theme callback is currently only tested manually. It works better than the currentmain but is still not perfect. (I'm going to try and make a gif to show what I mean.)
The icon path stuff is unit tested ✅

Note for reviewer(s):

This somewhat supersedes my plan (and what I wrote in the PR title) forGeneralise stylesheet parsing to extract colours for light/dark theme callback. #105 but doesn't actually supersede the code changes.
- ...so I'm going to keep that open for a bit longer (sorry for polluting the PR list). Will close when I decide what to do with it.
Are you OK with two face colours? I use boththeme.text andtheme.foreground.
The icons are black and not the subtle brownish grey used by napari's light theme.
Thenapari.Viewer gets moved up the family tree and becomes a member of theBaseNapariMPLWidget.

Solvesmatplotlib#86.

499c0a7

samcunliffe added the BugSomething isn't working label

May 31, 2023

samcunliffe requested review fromdstansby andruaridhg

May 31, 2023 14:00

Pre-commit and some docstring formatting.

4f8aa6d

samcunliffe linked an issue

May 31, 2023

that may beclosed by this pull request

Hard-coded icons and axes looks bad with non-default napari themes#86

Closed

samcunliffe commented

May 31, 2023

View reviewed changes

src/napari_matplotlib/base.py

		"""
		self._replace_toolbar_icons()
		if self.figure.gca():
		self.apply_napari_colorscheme(self.figure.gca())

Copy link

CollaboratorAuthor

samcunliffeMay 31, 2023•
edited
Loading

Choose a reason for hiding this comment

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

Not sure if this is the correct line, but I still don't quite have a perfect UX when switching the theme whilst the widgets are open...

Screen.Recording.2023-05-31.at.16.51.00.mov

Re-opening or doing something to the plot seems to fix.

Copy link

Member

dstansbyMay 31, 2023

Choose a reason for hiding this comment

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

A bit of a dive into thenapari source, and you might want to use something like this line to trigger something when the theme is changed?

https://github.com/napari/napari/blob/46dcc9c0976c35ec9dc64be60235f4addcadaa5c/napari/_qt/qt_main_window.py#L578

Copy link

CollaboratorAuthor

samcunliffeMay 31, 2023•
edited
Loading

Choose a reason for hiding this comment

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

😞

napari.settings.get_settings().appearance.events.theme.connect(self._on_theme_change)

gives the same behaviour. I wonder if it might be a different way to do the same thing as

self.viewer.events.theme.connect(self._on_theme_change)

The theme-change event isundocumented* (scroll a bit from the start of § Viewer events) so I'd be up for asking and/or fixing the docs for napari.

*) It's also duplicated: that page is made via ascript in napari/docs.

ruaridhg approved these changes

May 31, 2023

View reviewed changes

Copy link

Collaborator

ruaridhg left a comment

Choose a reason for hiding this comment

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

LGTM. Could switching the theme whilst widgets are open be something to do with the napari bug Iraised

samcunliffe commented

May 31, 2023

View reviewed changes

src/napari_matplotlib/base.pyShow resolvedHide resolved

Copy link

CollaboratorAuthor

samcunliffe commentedMay 31, 2023

Could switching the theme whilst widgets are open be something to do with the napari bug Inapari/napari#5847

Ithink not. Although something missing in the callbacks for a widget being loaded .... maybe.

Copy link

CollaboratorAuthor

samcunliffe commentedMay 31, 2023•
edited
Loading

Also needs a note in the changelog.

@samcunliffe : add a note in the changelog.

samcunliffe added3 commits

May 31, 2023 18:59

Have to regenerate the baseline figures.

4e198e7

Since we're now setting the axes to off-white.

Merge remote-tracking branch 'upstream/main' into 86-handling-of-ligh…

b6d2894

…t-themes

One more reference to update.

d5c05c6

samcunliffe force-pushed the86-handling-of-light-themes branch from276d6ac tod5c05c6Compare

May 31, 2023 18:01

samcunliffe commented

May 31, 2023

View reviewed changes

src/napari_matplotlib/tests/baseline/test_example_q_widget.png OutdatedShow resolvedHide resolved

samcunliffe added2 commits

May 31, 2023 19:05

Note to the changelog.

f5b4c2e

No intersphinx for you. Fine.

91d6937

samcunliffeenabled auto-merge

June 1, 2023 08:26

Copy link

CollaboratorAuthor

samcunliffe commentedJun 1, 2023

Err if this is ok to merge, I think it needs a reapproval.

I pushed a release note and updated the pytest-mpl plots. Sorry for the noise.

Copy link

Member

dstansby commentedJun 1, 2023•
edited
Loading

Happy to merge this (once I've checked the code), but things that need issues opening to be fixed after this:

If I switch to the light theme, the plot looks fine but the whole widget still has a dark background.
We couldn't get the theme changing event to work. Probably upstream issue, but also we should open an issue here so we don't forget.