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

Decouple changing viewer.theme from changing theme settings/preferences #5143

Merged

Conversation

psobolewskiPhD
Copy link
Member

Description

This PR decouples viewer.theme and the Toggle theme keybind (default: shift-command-T) from the settings: when the current viewer theme is changed, the setting is no longer altered. This way, one can change the current viewer theme, but not alter ones preferences.
If one wants to alter settings/preferences, then that can and should be done explicitly either using get_settings().appearance.theme or using the Preferences GUI.
See #5062
Note: the live behavior is really annoying when testing things, because toggling theme requires then fixing preferences when done. It means one can change the viewer.theme in examples/scripts without altering the users preferences, see: #4875 (comment)

Here's the console output when changing viewer.theme (note it's daytime, so system == 'light')

from napari.settings import get_settings
get_settings().appearance.theme
Out[1]: 'system'

viewer.theme
Out[2]: 'system'

viewer.theme = 'dark'

get_settings().appearance.theme
Out[4]: 'system'

Here's the console output when using the keybind to toggle themes:

viewer.theme
Out[1]: 'system'

viewer.theme
Out[2]: 'dark'

viewer.theme
Out[3]: 'light'

from napari.settings import get_settings
get_settings().appearance.theme
Out[4]: 'system'

You can see that the initial theme is system then it's toggled using the keybind a few times, but the setting remains system.

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: the existing example and viewer tutorial do not mention nor assume that changing viewer.theme will change settings, so no change is needed. The new behavior is consistent with the existing documentation.

Depending on point of view, this is either a bug fix or breaking change. I personally think viewer.theme and the Toggle theme keybind changing settings is an annoying bug. The fact that the new behavior is consistent with docs also suggests to me that the old behavior was a bug/unintended.

References

closes #5062

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
  • example: I check if my changes works with both PySide and PyQt backends
    as there are small differences between the two Qt bindings.
    As far as I can tell, existing tests are consistent with the new behavior rather than the old behavior.

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
  • If I included new strings, I have used trans. to make them localizable.
    For more information see our translations guide.

@github-actions github-actions bot added the qt Relates to qt label Sep 26, 2022
@github-actions
Copy link
Contributor

Click here to download the docs artifacts
docs
(zip file)

@Czaki Czaki added design Design discussion preferences Issues relating to the creation of new preference fields/panels labels Sep 26, 2022
@github-actions github-actions bot added tests Something related to our tests and removed design Design discussion preferences Issues relating to the creation of new preference fields/panels labels Sep 26, 2022
@github-actions
Copy link
Contributor

Click here to download the docs artifacts
docs
(zip file)

@codecov
Copy link

codecov bot commented Sep 26, 2022

Codecov Report

Merging #5143 (c19ed4e) into main (5e0aebe) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #5143      +/-   ##
==========================================
+ Coverage   89.08%   89.10%   +0.02%     
==========================================
  Files         595      596       +1     
  Lines       50579    50639      +60     
==========================================
+ Hits        45058    45123      +65     
+ Misses       5521     5516       -5     
Impacted Files Coverage Δ
napari/_qt/_tests/test_qt_provide_theme.py 100.00% <100.00%> (ø)
napari/_qt/qt_main_window.py 76.21% <100.00%> (ø)
...apari/components/_tests/test_viewer_keybindings.py 100.00% <100.00%> (ø)
napari/components/_viewer_key_bindings.py 100.00% <100.00%> (+1.63%) ⬆️
napari/utils/_tests/test_theme.py 100.00% <100.00%> (ø)
napari/utils/theme.py 92.68% <0.00%> (+0.60%) ⬆️
napari/utils/info.py 81.44% <0.00%> (+1.03%) ⬆️
napari/_qt/__init__.py 56.66% <0.00%> (+6.66%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@Czaki Czaki added design Design discussion preferences Issues relating to the creation of new preference fields/panels labels Sep 26, 2022
@psobolewskiPhD
Copy link
Member Author

psobolewskiPhD commented Sep 27, 2022

This PR also addresses the fact that if you use multiple viewers, then changing the theme of one changes them all.
See: #5062 (comment)

Try:

import napari

viewer1 = napari.Viewer()
viewer2 = napari.Viewer()

viewer2.theme = 'dark' # or light, whatever is opposite of your current theme

(or from the napari console just the last 2 lines)
Only viewer2 will switch theme, the other viewer will keep the previous theme.

Edit: let me expand on this:
When opened all viewers use the setting from napari settings.
If the user then uses viewer2.theme = 'dark' # or light only viewer2 should change themes and this PR makes that happen, for example to compare minimum blending in a Light viewer vs. additive in a Dark one.
If the user changes the napari setting of the theme, then both viewers will change—this is the same as live behavior.

Copy link
Contributor

@brisvag brisvag left a comment

Choose a reason for hiding this comment

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

Seems good. I wonder if it would be a good idea to use a notification pop-up to indicate what theme is the newly selected?

napari/components/_viewer_key_bindings.py Outdated Show resolved Hide resolved
@psobolewskiPhD
Copy link
Member Author

Seems good. I wonder if it would be a good idea to use a notification pop-up to indicate what theme is the newly selected?

You mean beyond the visual? Also, in which case? I assume, the keybind toggle?
I guess seeing the name pop after the toggle can be useful if there are a lot of themes to choose from, so it's hard to keep track. I'm not sure napari is as this stage yet, but it's certainly not hard to implement, perhaps using: napari.utils.notifications show_info?

@brisvag
Copy link
Contributor

brisvag commented Sep 27, 2022

You mean beyond the visual? Also, in which case? I assume, the keybind toggle?
I guess seeing the name pop after the toggle can be useful if there are a lot of themes to choose from, so it's hard to keep track. I'm not sure napari is as this stage yet, but it's certainly not hard to implement, perhaps using: napari.utils.notifications show_info?

Yup to all the questions :) I agree that it might be premature, I mostly thought about it as a way to get around you issue above (I don't love skipping the system theme altogether and forcing people to go in the settings for that "special" case).

@psobolewskiPhD
Copy link
Member Author

psobolewskiPhD commented Sep 27, 2022

You mean beyond the visual? Also, in which case? I assume, the keybind toggle?
I guess seeing the name pop after the toggle can be useful if there are a lot of themes to choose from, so it's hard to keep track. I'm not sure napari is as this stage yet, but it's certainly not hard to implement, perhaps using: napari.utils.notifications show_info?

Yup to all the questions :) I agree that it might be premature, I mostly thought about it as a way to get around you issue above (I don't love skipping the system theme altogether and forcing people to go in the settings for that "special" case).

system is just napari Light or napari Dark depending on your OS settings:

def get_system_theme() -> str:
"""Return the system default theme, either 'dark', or 'light'."""
try:
name = darkdetect.theme().lower()
except Exception:
name = "dark"
return name

It's not actually a different theme. So if you want to toggle between one or the other (or other themes you install) then hitting system is annoying because it frequently does nothing.

Edit: extra clarification: with this PR the toggle keybind only changes the theme of the current viewer, the user can always use settings to change to system.

@brisvag
Copy link
Contributor

brisvag commented Sep 27, 2022

It's not actually a different theme. So if you want to toggle between one or the other (or other themes you install) then hitting system is annoying because it frequently does nothing.

Ok; I don't feel strongly about this, so I'll let you and others decide :)

@github-actions github-actions bot removed design Design discussion preferences Issues relating to the creation of new preference fields/panels labels Sep 27, 2022
@github-actions
Copy link
Contributor

Click here to download the docs artifacts
docs
(zip file)

psobolewskiPhD and others added 2 commits September 27, 2022 22:28
Co-authored-by: Grzegorz Bokota <bokota+github@gmail.com>
@github-actions
Copy link
Contributor

Click here to download the docs artifacts
docs
(zip file)

@Czaki
Copy link
Collaborator

Czaki commented Dec 11, 2022

Another option is to drop the menu item from this PR and make it a followup.

It is a good idea for me.

@psobolewskiPhD psobolewskiPhD force-pushed the decouple_viewer_theme_from_settings branch from ff6ef55 to 59920f8 Compare December 11, 2022 21:21
@psobolewskiPhD
Copy link
Member Author

I think I messed up the reverting (because of the previous merge?) but I've managed to force things to be before the View menu additions.
So now we're back to just the change in the behavior of viewer.theme = 'xx' not changing settings and toggle also not changing settings.

@DragaDoncila
Copy link
Contributor

I think removing the menu addition from this one is a good move @psobolewskiPhD. I will review asap, but probably not until tomorrow so if others have already approved, go for the merge. I'll also take a look at adding the menu item, I think it'll be a small change, but just haven't had a chance to play with it yet.

@psobolewskiPhD psobolewskiPhD marked this pull request as ready for review December 11, 2022 22:06
@psobolewskiPhD
Copy link
Member Author

FYI here's the blending/canvas docs PR: napari/docs#69

@psobolewskiPhD psobolewskiPhD force-pushed the decouple_viewer_theme_from_settings branch from 8cae6a1 to c19ed4e Compare December 17, 2022 11:45
Copy link
Contributor

@DragaDoncila DragaDoncila left a comment

Choose a reason for hiding this comment

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

Pulled this down locally and confirmed it all works as expected. Had a quick look at the code and nothing seems out of the ordinary so let's get this in 🎉

@psobolewskiPhD psobolewskiPhD merged commit daa8afe into napari:main Jan 5, 2023
@psobolewskiPhD psobolewskiPhD deleted the decouple_viewer_theme_from_settings branch January 5, 2023 17:10
kne42 added a commit to kne42/napari that referenced this pull request Jan 9, 2023
* main:
  Bugfix: Ensure layer._fixed_vertex is set when rotating (napari#5449)
  Bugfix: Fix test_get_system_theme test for `name` to `id` change (napari#5456)
  Add correct `enablement` kwarg to `Split Stack` action, `Convert data type` submenu and `Projections` submenu (napari#5437)
  Add error color to themes and change application close/exit dialogs (napari#5442)
  Add auto canceling previous test after a new PR commit (napari#5453)
  Fix theme id reference to get image for 'success_label' style (napari#5447)
  Decouple changing viewer.theme from changing theme settings/preferences (napari#5143)
  [pre-commit.ci] pre-commit autoupdate (napari#5422)
  Configure `fail_on_no_env` for `tox-gh-actions` (napari#5408)
melissawm pushed a commit to melissawm/napari that referenced this pull request Jan 11, 2023
…es (napari#5143)

* don't set theme setting on updating theme or keybind

* update test for explicit behavior

* fix idx logic vs. checking for `system`

* improve conciseness

Co-authored-by: Grzegorz Bokota <bokota+github@gmail.com>

* use modulo for system check too

* Add test for toggle_theme behavior

* fix test?

* Test toggle_theme is looping all avail themes.

* Clarify keybind in settings & docstring

* test get_theme and get_system_theme

* Use ViewerModel for test

* add test to check behavior when theme is system

* Break provide_theme_hook into two tests

* Use new get_theme for test

Co-authored-by: Grzegorz Bokota <bokota+github@gmail.com>

* Fix get_theme, need to pass False

* use ViewerModel for `toggle_theme` definition

Co-authored-by: Grzegorz Bokota <bokota+github@gmail.com>

* tweak _update_theme per @Czaki

* Fix test_canvas_color for system theme

* Assert initial theme not the one we toggle to

Co-authored-by: Andy Sweet <andrew.d.sweet@gmail.com>

* Clarify `get_theme` argument

Co-authored-by: Andy Sweet <andrew.d.sweet@gmail.com>

* Refactor per @andy-sweet: make viewer w/theme func

* if system, set stylesheet rather than theme

* Fix ViewerModel tests per @andy-sweet

* Revert "Fix test_canvas_color for system theme"

This reverts commit 58effe1.

* Fix tests

* fix test

* back to ViewerModel for toggle_theme

Co-authored-by: Grzegorz Bokota <bokota+github@gmail.com>
Co-authored-by: Andy Sweet <andrew.d.sweet@gmail.com>
@psobolewskiPhD psobolewskiPhD mentioned this pull request Mar 17, 2023
7 tasks
@Czaki Czaki modified the milestones: 0.5.0, 0.4.18 Jun 17, 2023
Czaki added a commit that referenced this pull request Jun 17, 2023
…es (#5143)

* don't set theme setting on updating theme or keybind

* update test for explicit behavior

* fix idx logic vs. checking for `system`

* improve conciseness

Co-authored-by: Grzegorz Bokota <bokota+github@gmail.com>

* use modulo for system check too

* Add test for toggle_theme behavior

* fix test?

* Test toggle_theme is looping all avail themes.

* Clarify keybind in settings & docstring

* test get_theme and get_system_theme

* Use ViewerModel for test

* add test to check behavior when theme is system

* Break provide_theme_hook into two tests

* Use new get_theme for test

Co-authored-by: Grzegorz Bokota <bokota+github@gmail.com>

* Fix get_theme, need to pass False

* use ViewerModel for `toggle_theme` definition

Co-authored-by: Grzegorz Bokota <bokota+github@gmail.com>

* tweak _update_theme per @Czaki

* Fix test_canvas_color for system theme

* Assert initial theme not the one we toggle to

Co-authored-by: Andy Sweet <andrew.d.sweet@gmail.com>

* Clarify `get_theme` argument

Co-authored-by: Andy Sweet <andrew.d.sweet@gmail.com>

* Refactor per @andy-sweet: make viewer w/theme func

* if system, set stylesheet rather than theme

* Fix ViewerModel tests per @andy-sweet

* Revert "Fix test_canvas_color for system theme"

This reverts commit 58effe1.

* Fix tests

* fix test

* back to ViewerModel for toggle_theme

Co-authored-by: Grzegorz Bokota <bokota+github@gmail.com>
Co-authored-by: Andy Sweet <andrew.d.sweet@gmail.com>
@Czaki Czaki mentioned this pull request Jun 17, 2023
Czaki added a commit that referenced this pull request Jun 18, 2023
…es (#5143)

* don't set theme setting on updating theme or keybind

* update test for explicit behavior

* fix idx logic vs. checking for `system`

* improve conciseness

Co-authored-by: Grzegorz Bokota <bokota+github@gmail.com>

* use modulo for system check too

* Add test for toggle_theme behavior

* fix test?

* Test toggle_theme is looping all avail themes.

* Clarify keybind in settings & docstring

* test get_theme and get_system_theme

* Use ViewerModel for test

* add test to check behavior when theme is system

* Break provide_theme_hook into two tests

* Use new get_theme for test

Co-authored-by: Grzegorz Bokota <bokota+github@gmail.com>

* Fix get_theme, need to pass False

* use ViewerModel for `toggle_theme` definition

Co-authored-by: Grzegorz Bokota <bokota+github@gmail.com>

* tweak _update_theme per @Czaki

* Fix test_canvas_color for system theme

* Assert initial theme not the one we toggle to

Co-authored-by: Andy Sweet <andrew.d.sweet@gmail.com>

* Clarify `get_theme` argument

Co-authored-by: Andy Sweet <andrew.d.sweet@gmail.com>

* Refactor per @andy-sweet: make viewer w/theme func

* if system, set stylesheet rather than theme

* Fix ViewerModel tests per @andy-sweet

* Revert "Fix test_canvas_color for system theme"

This reverts commit 58effe1.

* Fix tests

* fix test

* back to ViewerModel for toggle_theme

Co-authored-by: Grzegorz Bokota <bokota+github@gmail.com>
Co-authored-by: Andy Sweet <andrew.d.sweet@gmail.com>
Czaki added a commit that referenced this pull request Jun 19, 2023
…es (#5143)

* don't set theme setting on updating theme or keybind

* update test for explicit behavior

* fix idx logic vs. checking for `system`

* improve conciseness

Co-authored-by: Grzegorz Bokota <bokota+github@gmail.com>

* use modulo for system check too

* Add test for toggle_theme behavior

* fix test?

* Test toggle_theme is looping all avail themes.

* Clarify keybind in settings & docstring

* test get_theme and get_system_theme

* Use ViewerModel for test

* add test to check behavior when theme is system

* Break provide_theme_hook into two tests

* Use new get_theme for test

Co-authored-by: Grzegorz Bokota <bokota+github@gmail.com>

* Fix get_theme, need to pass False

* use ViewerModel for `toggle_theme` definition

Co-authored-by: Grzegorz Bokota <bokota+github@gmail.com>

* tweak _update_theme per @Czaki

* Fix test_canvas_color for system theme

* Assert initial theme not the one we toggle to

Co-authored-by: Andy Sweet <andrew.d.sweet@gmail.com>

* Clarify `get_theme` argument

Co-authored-by: Andy Sweet <andrew.d.sweet@gmail.com>

* Refactor per @andy-sweet: make viewer w/theme func

* if system, set stylesheet rather than theme

* Fix ViewerModel tests per @andy-sweet

* Revert "Fix test_canvas_color for system theme"

This reverts commit 58effe1.

* Fix tests

* fix test

* back to ViewerModel for toggle_theme

Co-authored-by: Grzegorz Bokota <bokota+github@gmail.com>
Co-authored-by: Andy Sweet <andrew.d.sweet@gmail.com>
Czaki added a commit that referenced this pull request Jun 21, 2023
…es (#5143)

* don't set theme setting on updating theme or keybind

* update test for explicit behavior

* fix idx logic vs. checking for `system`

* improve conciseness

Co-authored-by: Grzegorz Bokota <bokota+github@gmail.com>

* use modulo for system check too

* Add test for toggle_theme behavior

* fix test?

* Test toggle_theme is looping all avail themes.

* Clarify keybind in settings & docstring

* test get_theme and get_system_theme

* Use ViewerModel for test

* add test to check behavior when theme is system

* Break provide_theme_hook into two tests

* Use new get_theme for test

Co-authored-by: Grzegorz Bokota <bokota+github@gmail.com>

* Fix get_theme, need to pass False

* use ViewerModel for `toggle_theme` definition

Co-authored-by: Grzegorz Bokota <bokota+github@gmail.com>

* tweak _update_theme per @Czaki

* Fix test_canvas_color for system theme

* Assert initial theme not the one we toggle to

Co-authored-by: Andy Sweet <andrew.d.sweet@gmail.com>

* Clarify `get_theme` argument

Co-authored-by: Andy Sweet <andrew.d.sweet@gmail.com>

* Refactor per @andy-sweet: make viewer w/theme func

* if system, set stylesheet rather than theme

* Fix ViewerModel tests per @andy-sweet

* Revert "Fix test_canvas_color for system theme"

This reverts commit 58effe1.

* Fix tests

* fix test

* back to ViewerModel for toggle_theme

Co-authored-by: Grzegorz Bokota <bokota+github@gmail.com>
Co-authored-by: Andy Sweet <andrew.d.sweet@gmail.com>
Czaki added a commit that referenced this pull request Jun 21, 2023
…es (#5143)

* don't set theme setting on updating theme or keybind

* update test for explicit behavior

* fix idx logic vs. checking for `system`

* improve conciseness

Co-authored-by: Grzegorz Bokota <bokota+github@gmail.com>

* use modulo for system check too

* Add test for toggle_theme behavior

* fix test?

* Test toggle_theme is looping all avail themes.

* Clarify keybind in settings & docstring

* test get_theme and get_system_theme

* Use ViewerModel for test

* add test to check behavior when theme is system

* Break provide_theme_hook into two tests

* Use new get_theme for test

Co-authored-by: Grzegorz Bokota <bokota+github@gmail.com>

* Fix get_theme, need to pass False

* use ViewerModel for `toggle_theme` definition

Co-authored-by: Grzegorz Bokota <bokota+github@gmail.com>

* tweak _update_theme per @Czaki

* Fix test_canvas_color for system theme

* Assert initial theme not the one we toggle to

Co-authored-by: Andy Sweet <andrew.d.sweet@gmail.com>

* Clarify `get_theme` argument

Co-authored-by: Andy Sweet <andrew.d.sweet@gmail.com>

* Refactor per @andy-sweet: make viewer w/theme func

* if system, set stylesheet rather than theme

* Fix ViewerModel tests per @andy-sweet

* Revert "Fix test_canvas_color for system theme"

This reverts commit 58effe1.

* Fix tests

* fix test

* back to ViewerModel for toggle_theme

Co-authored-by: Grzegorz Bokota <bokota+github@gmail.com>
Co-authored-by: Andy Sweet <andrew.d.sweet@gmail.com>
Czaki added a commit that referenced this pull request Jun 21, 2023
…es (#5143)

* don't set theme setting on updating theme or keybind

* update test for explicit behavior

* fix idx logic vs. checking for `system`

* improve conciseness

Co-authored-by: Grzegorz Bokota <bokota+github@gmail.com>

* use modulo for system check too

* Add test for toggle_theme behavior

* fix test?

* Test toggle_theme is looping all avail themes.

* Clarify keybind in settings & docstring

* test get_theme and get_system_theme

* Use ViewerModel for test

* add test to check behavior when theme is system

* Break provide_theme_hook into two tests

* Use new get_theme for test

Co-authored-by: Grzegorz Bokota <bokota+github@gmail.com>

* Fix get_theme, need to pass False

* use ViewerModel for `toggle_theme` definition

Co-authored-by: Grzegorz Bokota <bokota+github@gmail.com>

* tweak _update_theme per @Czaki

* Fix test_canvas_color for system theme

* Assert initial theme not the one we toggle to

Co-authored-by: Andy Sweet <andrew.d.sweet@gmail.com>

* Clarify `get_theme` argument

Co-authored-by: Andy Sweet <andrew.d.sweet@gmail.com>

* Refactor per @andy-sweet: make viewer w/theme func

* if system, set stylesheet rather than theme

* Fix ViewerModel tests per @andy-sweet

* Revert "Fix test_canvas_color for system theme"

This reverts commit 58effe1.

* Fix tests

* fix test

* back to ViewerModel for toggle_theme

Co-authored-by: Grzegorz Bokota <bokota+github@gmail.com>
Co-authored-by: Andy Sweet <andrew.d.sweet@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
qt Relates to qt tests Something related to our tests UI/UX
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Changing current viewer property (e.g. theme) becomes persistent change of settings
6 participants