Skip to content

Add toggle status view menu items#6137

Merged
DragaDoncila merged 4 commits intonapari:mainfrom
lucyleeow:toggle
Sep 20, 2023
Merged

Add toggle status view menu items#6137
DragaDoncila merged 4 commits intonapari:mainfrom
lucyleeow:toggle

Conversation

@lucyleeow
Copy link
Copy Markdown
Contributor

@lucyleeow lucyleeow commented Aug 11, 2023 •
edited by github-actions bot
Loading

Fixes/Closes

Ref: #4826 (comment)

Description

Adds toggle status to view menu items: Toggle Play, Toggle Fullscreen and Toggle Menubar . These were not added as part of view menu app model migration PR as they did not exist in main and thus decided to be out of scope of that PR

References

Type of change

  • Bug-fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Maintenance (changes required to run napari, tests, & CI smoothly)
  • Enhancement (non-breaking improvements of an existing feature)
  • Documentation (update of docstrings and deprecation information)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

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.

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 Aug 11, 2023
@lucyleeow lucyleeow added the bugfix PR with bugfix label Aug 11, 2023
@github-actions github-actions bot added the tests Something related to our tests label Aug 11, 2023
@codecov
Copy link
Copy Markdown

codecov bot commented Aug 11, 2023 •
edited
Loading

Codecov Report

Merging #6137 (e56b5b2) into main (eaff277) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #6137   +/-   ##
=======================================
  Coverage   91.60%   91.61%           
=======================================
  Files         583      583           
  Lines       51364    51371    +7     
=======================================
+ Hits        47051    47061   +10     
+ Misses       4313     4310    -3
Files Changed Coverage Δ
napari/_qt/_qapp_model/qactions/_view.py 94.44% <100.00%> (+3.53%) ⬆️

... and 1 file with indirect coverage changes

@jni jni modified the milestones: 0.5, 0.5.0 Aug 15, 2023
DragaDoncila
DragaDoncila approved these changes Aug 18, 2023
View reviewed changes
Copy link
Copy Markdown
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.

Thanks for this one @lucyleeow. Can confirm Toggle Play, Toggle Fullscreen and Toggle Menubar are now showing the checkmarks in the menu as expected on Mac and Ubuntu (didn't check Windows). Can you please update the description to mention these three items specifically, as some of them e.g. the axes already had the ToggleRules so they haven't changed.

@github-actions github-actions bot removed the tests Something related to our tests label Sep 19, 2023
@lucyleeow
Copy link
Copy Markdown
Contributor Author

lucyleeow commented Sep 19, 2023

@DragaDoncila is this ready to go in?

@lucyleeow
Copy link
Copy Markdown
Contributor Author

lucyleeow commented Sep 19, 2023 •
edited
Loading

mypy error unrelated and fixed by #6230

@DragaDoncila
Copy link
Copy Markdown
Contributor

DragaDoncila commented Sep 19, 2023

@lucyleeow yep, I've added the ready-to-merge label and will merge in 24 hours!

@DragaDoncila DragaDoncila added the ready to merge Last chance for comments! Will be merged in ~24h label Sep 19, 2023
@melonora
Copy link
Copy Markdown
Contributor

melonora commented Sep 19, 2023

There is currently a test failing due to _pref_dialog being able to be None in which case _list does not exist. See fix for this here #6216

psobolewskiPhD
psobolewskiPhD approved these changes Sep 20, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@psobolewskiPhD psobolewskiPhD left a comment

Choose a reason for hiding this comment

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

Makes sense to me, thanks.

Note: testing this did remind me of that on macOS we get double Full Screen toggle in the View menu, which I'd previously reported.
#5092 (comment)
It's wierd.

Here, toggling the native macOS full screen don't trigger the check-mark toggle status.
Very niche case, doubt anything can be done about it here.

@lucyleeow
Copy link
Copy Markdown
Contributor Author

lucyleeow commented Sep 20, 2023 •
edited
Loading

Here, toggling the native macOS full screen don't trigger the check-mark toggle status.

Does the 2nd full screen toggle,'Enter Full Screen' have a check next to it?

Also, have you noticed this before? I don't know if we previously (with action manager) had check marks? Worth adding to #5092 so it's noted at least?

@psobolewskiPhD
Copy link
Copy Markdown
Member

psobolewskiPhD commented Sep 20, 2023

I very rarely use full screen, so hard for me to say.
But native macOS, the bottom one Enter Fullscreen, doesn't get a check, instead it switches to Exit Fullscreen.
This works in napari too.
But the napari fullscreen doesn't know that happened?
As noted in the other issue, I don't get where that extra native menu item comes from. But it's there, so maybe best to just hide the non-native one on macOS.

@lucyleeow
Copy link
Copy Markdown
Contributor Author

lucyleeow commented Sep 20, 2023

so maybe best to just hide the non-native one on macOS.

Good idea, we can do that with 'when', I will open an issue for that to keep track.

@DragaDoncila
Copy link
Copy Markdown
Contributor

DragaDoncila commented Sep 20, 2023

Yep, agreed - we should just hide the non native option on Mac. Looks like the test failure here is a timeout

@lucyleeow lucyleeow mentioned this pull request Sep 20, 2023
Open
@lucyleeow
Copy link
Copy Markdown
Contributor Author

lucyleeow commented Sep 20, 2023 •
edited
Loading

I'll put up a PR to fix #6240, hopefully an easy fix

@DragaDoncila DragaDoncila merged commit 066707e into napari:main Sep 20, 2023
@lucyleeow lucyleeow deleted the toggle branch September 21, 2023 00:32
@psobolewskiPhD
Copy link
Copy Markdown
Member

psobolewskiPhD commented Sep 21, 2023

I'll put up a PR for #6240, hopefully an easy fix

Had me very confused...

╰─ gh pr checkout 6240                                           (napari-dev) ─╯
GraphQL: Could not resolve to a PullRequest with the number of 6240. (repository.pullRequest)

That's no PR, that's an issue!
🤣

@lucyleeow
Copy link
Copy Markdown
Contributor Author

lucyleeow commented Sep 21, 2023 •
edited
Loading

Weird 6240 seems the right number for me:

image

Oh sorry I see what you mean, i'll edit my comment to make it more clear

kne42 added a commit to kne42/napari that referenced this pull request Sep 21, 2023
@kne42
Merge branch 'main' into kb-impl
f2e39a8 
* main:
  Move `test_file_menu.py` to new app model location (napari#6233)
  Add toggle status view menu items (napari#6137)
  More removal of np.all(... == ...) (napari#6238)
  [pre-commit.ci] pre-commit autoupdate (napari#6229)
@Czaki Czaki removed the ready to merge Last chance for comments! Will be merged in ~24h label Sep 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@DragaDoncila DragaDoncila DragaDoncila approved these changes

@psobolewskiPhD psobolewskiPhD psobolewskiPhD approved these changes

Assignees

@lucyleeow lucyleeow

Labels

bugfix PR with bugfix qt Relates to qt

Projects

None yet

Milestone

0.5.0

Development

Successfully merging this pull request may close these issues.

6 participants

@lucyleeow @DragaDoncila @melonora @psobolewskiPhD @jni @Czaki