use app-model for view menu #4826

tlambert03 · 2022-07-16T18:10:07Z

Description

Partially to keep moving forward, but also to keep testing the app-model pattern, I'm playing with some branches that use #4784 to start removing old code. This refactors the menu bar view menu with the new pattern.

@Czaki, particularly interested to get your thoughts on this. While I'm mostly very happy with the pattern of declaring commands that require dependencies to be injected (rather than always using methods bound to specific instances), I do recognize that some of it feels a little different as I go. Your Qt expertise and critical eye is very much appreciated here.

diff relative to #4784: tlambert03/napari@use-app-model...tlambert03:napari:appmodel-view-menu

for more information, see https://pre-commit.ci

…into use-app-model

nclack · 2022-10-13T22:49:27Z

@lucyleeow @DragaDoncila Is this ready to merge? Do we need to get another review from someone?

DragaDoncila · 2022-10-13T23:00:40Z

@nclack would be great if we could get someone's eyes (yours maybe... 👀) on how we ended up setting up the app fixture in conftest.py. Mainly:

we've made it autoused, so that every test gets an instance
we've made it a NapariApplication not a plain app-model Application. This is because we felt like for the majority of tests, we'd want the providers, accessors and actions registered in the NapariApplication.__init__. If someone wanted a completely clean unadulterated app in their test, they could always create one, or clear the existing one.
we didn't init_qactions in the fixture, as we didn't feel it would be best practice to have qt associated bits and bobs registered for every test. These do get registered whenever make_napari_viewer is used, and init_qactions can be called explicitly (as we've done in the build_qmodel_menu test) when you want all the bells and whistles, as long as the appropriate providers are mocked.

If all of this sounds sane and reasonable to you, then I think this is ready for merge? @lucyleeow can confirm as she's been doing all the heavy lifting here.

liu-ziyang · 2022-10-13T23:25:55Z

@nclack would be great if we could get someone's eyes (yours maybe... 👀) on how we ended up setting up the app fixture in conftest.py. Mainly:

we've made it autoused, so that every test gets an instance

we've made it a NapariApplication not a plain app-model Application. This is because we felt like for the majority of tests, we'd want the providers, accessors and actions registered in the NapariApplication.__init__. If someone wanted a completely clean unadulterated app in their test, they could always create one, or clear the existing one.

we didn't init_qactions in the fixture, as we didn't feel it would be best practice to have qt associated bits and bobs registered for every test. These do get registered whenever make_napari_viewer is used, and init_qactions can be called explicitly (as we've done in the build_qmodel_menu test) when you want all the bells and whistles, as long as the appropriate providers are mocked.

If all of this sounds sane and reasonable to you, then I think this is ready for merge? @lucyleeow can confirm as she's been doing all the heavy lifting here.

This sounds in line with what the group discussed before, I think we should start the 24 hour clock for merging

lucyleeow · 2022-10-15T08:03:37Z

Thank you for the notes @DragaDoncila ! I was half way through typing some but didn't finish. Have added minor notes to the docstring of mock app fixture to add one of the things you mentioned. I agree that this deserves another pair of eyes due to the changes made - so ready for review and merge.

The only thing I would note, for our future reference, is that we mock patched the NapariApplication get_app method and not the function napari._app_model.get_app() because of funny import behaviour observed when mocking napari._app_model.get_app() - we found you had to import napari._app_model.get_app() within the test function for it to work when mocking the function (instead of the method).

lucyleeow · 2022-10-17T01:11:42Z

One of the CI's is still failing: https://github.com/napari/napari/actions/runs/3254935776/jobs/5343726097

It seems maybe unrelated? It was during test collection

E ValueError: <enum 'NotificationSeverity'> may only be called with a str or an instance of <enum 'NotificationSeverity'>. Got <class 'shibokensupport.enum_310.auto'>

lucyleeow · 2022-10-17T02:17:00Z

Okay seems unrelated and can probably be ignored - see #5217

DragaDoncila · 2022-10-20T00:30:25Z

Both failures here are unrelated, and I'm happy with the state of the PR atm. I've pulled it down and checked the menu behaves as expected, including the toggle functionality, so I'm going to merge this now. @lucyleeow @potating-potato I think next focus should be on #4977 and #4922 as they don't include any dynamic menu generation.

lucyleeow · 2022-10-20T00:40:20Z

Thank you @DragaDoncila !!

I will update #4977 which should fix the current CI failures!

* feat: use app-model WIP * partially fixed circ imports * bring back some menus with lazy import * wip * incorporate changes from app-model and ino * remove old code * remove more * add app-model dep * remove module from typecheck * small changes * remove not gc.collect assertion * reduce test code * add comments about context objects * add in-n-out to deps * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * fix test * bump in-n-out min ver * wip * refactor: add constants module, add test for menu building * add __all__ to constants * add some tests * add more comments * bump app-model * add internal documentation * add motivations and vision * wip * wip * wip * working replacement for view_menu * add parent to menu, fix test * remove comment * bump version, raise synchronous errors * add back napari namespace * sort import * comment navigation global * use navigation global * lenient test * Update docs/guides/app_model.md Co-authored-by: alisterburt <alisterburt@gmail.com> * Update napari/_app/actions/_layer_actions.py Co-authored-by: alisterburt <alisterburt@gmail.com> * Update napari/_qt/widgets/_tests/test_qt_extension2reader.py Co-authored-by: Grzegorz Bokota <bokota+github@gmail.com> * rename module * use get_app getter * bump appmodel * fix comment * add comment * move layer actions constants * clarify expression * fix merge * update docs * add tests * fix test * reorg actions * add comment * fix import * update docs * add comment to layer actions * add comment to named tuple * more comments for nathan * add comment * remove in-n-out, pin app-model * Update docs/guides/app_model.md Co-authored-by: Kira Evans <contact@kne42.me> * Update docs/guides/app_model.md Co-authored-by: Kira Evans <contact@kne42.me> * Update napari/_qt/_qapp_model/_menus.py Co-authored-by: Grzegorz Bokota <bokota+github@gmail.com> * toggle action * change viewer toggle action * add viewer toggle action subclass * simplify toggle action with app-model v0.0.8 * fix test * undo last change * fix test * fix type * global import * module docstring * quote type hint * add simple test, move action * relative import * add viewer toggle test * fix activity dock toggle * bump app-model, fix final actions * remove dual param to _QtMainWindow * remove print * remove comment * more comments * add shorcut * amend injection to pass window in init * nits * black * add comment in _QtMainWindow * fix toggles * try ci * test CI, no activity dock toggle * check provider in CI * CI - test just qapp_model_menus * fix app mock in conftest * revert tox ini * fix app tests * cruft * update docstring fixture Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: alisterburt <alisterburt@gmail.com> Co-authored-by: Grzegorz Bokota <bokota+github@gmail.com> Co-authored-by: Kira Evans <contact@kne42.me> Co-authored-by: Ziyang Liu <zliu@chanzuckerberg.com> Co-authored-by: lucy liu <jliu176@gmail.com> Co-authored-by: Nathan Clack <nclack@gmail.com>

# 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.

tlambert03 and others added 30 commits July 3, 2022 14:59

feat: use app-model WIP

7a7f14c

partially fixed circ imports

8241dc2

bring back some menus with lazy import

6fec004

wip

00ca1a6

incorporate changes from app-model and ino

7073aa3

remove old code

2c12cb7

remove more

6e93b9b

add app-model dep

197268a

remove module from typecheck

29d7e1b

Merge branch 'main' into use-app-model

d81a277

small changes

9e42378

Merge branch 'main' into use-app-model

eb0958c

Merge branch 'main' into use-app-model

6ee215b

remove not gc.collect assertion

cd41480

reduce test code

adf237f

add comments about context objects

3f8978c

add in-n-out to deps

27a495f

[pre-commit.ci] auto fixes from pre-commit.com hooks

168e890

for more information, see https://pre-commit.ci

fix test

b0c742e

Merge branch 'use-app-model' of https://github.com/tlambert03/napari …

5832656

…into use-app-model

bump in-n-out min ver

0cbad81

Merge branch 'main' into use-app-model

86ab609

wip

c1fbd05

refactor: add constants module, add test for menu building

8a6bd5b

add __all__ to constants

34fb5cb

add some tests

94aa8fd

add more comments

26b54ce

bump app-model

543e411

add internal documentation

dca809f

add motivations and vision

dc5cd29

lucyleeow added 4 commits October 13, 2022 17:50

fix app mock in conftest

3bfe165

revert tox ini

7661044

fix app tests

b5e10e1

cruft

b7afb0a

update docstring fixture

e998eff

DragaDoncila merged commit ec69e69 into napari:main Oct 20, 2022

psobolewskiPhD mentioned this pull request Dec 5, 2022

Add parent when creating layer context menu to inherit application theme and add style entry for disabled widgets and menus #5381

Merged

4 tasks

Czaki mentioned this pull request Jun 7, 2023

v0.4.18 - release process #5911

Closed

Czaki added this to the 0.5.0 milestone Jun 7, 2023

Czaki added triaged-0.4.18 To mark PR that is triaged in 0.4.18 release process maintenance PR with maintance changes, labels Jun 7, 2023

Czaki modified the milestones: 0.5.0, 0.4.18 Jun 16, 2023

lucyleeow mentioned this pull request Aug 11, 2023

Add toggle status view menu items #6137

Merged

15 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

use app-model for view menu #4826

use app-model for view menu #4826

tlambert03 commented Jul 16, 2022 •

edited

Loading

nclack commented Oct 13, 2022

DragaDoncila commented Oct 13, 2022

liu-ziyang commented Oct 13, 2022

lucyleeow commented Oct 15, 2022 •

edited

Loading

lucyleeow commented Oct 17, 2022 •

edited

Loading

lucyleeow commented Oct 17, 2022

DragaDoncila commented Oct 20, 2022

lucyleeow commented Oct 20, 2022

use app-model for view menu #4826

use app-model for view menu #4826

Conversation

tlambert03 commented Jul 16, 2022 • edited Loading

Description

nclack commented Oct 13, 2022

DragaDoncila commented Oct 13, 2022

liu-ziyang commented Oct 13, 2022

lucyleeow commented Oct 15, 2022 • edited Loading

lucyleeow commented Oct 17, 2022 • edited Loading

lucyleeow commented Oct 17, 2022

DragaDoncila commented Oct 20, 2022

lucyleeow commented Oct 20, 2022

tlambert03 commented Jul 16, 2022 •

edited

Loading

lucyleeow commented Oct 15, 2022 •

edited

Loading

lucyleeow commented Oct 17, 2022 •

edited

Loading