Skip to content

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

samcunliffe
Copy link
Collaborator

@samcunliffe samcunliffe commented May 31, 2023

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

Screenshot 2023-05-31 at 14 49 45

Testing:

  • The theme changing via the viewer.events.theme callback is currently only tested manually. It works better than the current main 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) for Generalise 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 both theme.text and theme.foreground.
  • The icons are black and not the subtle brownish grey used by napari's light theme.
  • The napari.Viewer gets moved up the family tree and becomes a member of the BaseNapariMPLWidget.

@samcunliffe samcunliffe added the Bug Something isn't working label May 31, 2023
@samcunliffe samcunliffe requested review from dstansby and ruaridhg May 31, 2023 14:00
@samcunliffe samcunliffe linked an issue May 31, 2023 that may be closed by this pull request
"""
self._replace_toolbar_icons()
if self.figure.gca():
self.apply_napari_colorscheme(self.figure.gca())
Copy link
Collaborator Author

@samcunliffe samcunliffe May 31, 2023

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

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 the napari 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
Collaborator Author

@samcunliffe samcunliffe May 31, 2023

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 is undocumented* (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 a script in napari/docs.

Copy link
Collaborator

@ruaridhg 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 I raised

@samcunliffe
Copy link
Collaborator Author

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

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

@samcunliffe
Copy link
Collaborator Author

samcunliffe commented May 31, 2023

Also needs a note in the changelog.

@samcunliffe samcunliffe force-pushed the 86-handling-of-light-themes branch from 276d6ac to d5c05c6 Compare May 31, 2023 18:01
@samcunliffe samcunliffe enabled auto-merge June 1, 2023 08:26
@samcunliffe
Copy link
Collaborator Author

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.

@dstansby
Copy link
Member

dstansby commented Jun 1, 2023

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.
    Screenshot 2023-06-01 at 09 35 08
  • 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.

Copy link
Member

@dstansby dstansby left a comment

Choose a reason for hiding this comment

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

Some minor comments, but overall looking 👍

@samcunliffe
Copy link
Collaborator Author

but things that need issues opening to be fixed after this

Yeah. There are a few minor peripheral things that I noticed too...

  • Axis titles are always black.
  • Icons are black (and should properly be greyish brown to match the theme).

Are you ok with 3 individual minor issues?

@dstansby
Copy link
Member

dstansby commented Jun 1, 2023

Yep, three issues sounds good

@samcunliffe samcunliffe added this pull request to the merge queue Jun 1, 2023
Merged via the queue into matplotlib:main with commit 7bf5db1 Jun 1, 2023
@samcunliffe samcunliffe deleted the 86-handling-of-light-themes branch June 1, 2023 10:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Hard-coded icons and axes looks bad with non-default napari themes
3 participants