Skip to content

Move Qt view menu actions inside Qt folder#6767

Merged
DragaDoncila merged 9 commits intonapari:mainfrom
lucyleeow:mv_view_actions
Apr 1, 2024
Merged

Move Qt view menu actions inside Qt folder#6767
DragaDoncila merged 9 commits intonapari:mainfrom
lucyleeow:mv_view_actions

Conversation

@lucyleeow
Copy link
Copy Markdown
Contributor

@lucyleeow lucyleeow commented Mar 22, 2024 •
edited
Loading

References and relevant issues

Noticed when working on #6743

Description

The current 'non-qt' actions defined in: napari/_app_model/actions/_view_actions.py actually only make sense with a GUI viewer (visibility of axes etc). I have moved these inside: napari/_qt/_qapp_model/qactions/_view.py with the other Qt view actions.

Note I have kept ViewerToggleAction where it is as it is general enough that it could be used with a non-qt viewer, but happy to move if requested.

@lucyleeow lucyleeow added this to the 0.5.0 milestone Mar 22, 2024
@lucyleeow lucyleeow added the maintenance PR with maintance changes, label Mar 22, 2024
@github-actions github-actions bot added qt Relates to qt and removed maintenance PR with maintance changes, labels Mar 22, 2024
@lucyleeow lucyleeow added the maintenance PR with maintance changes, label Mar 22, 2024
@lucyleeow
Copy link
Copy Markdown
Contributor Author

lucyleeow commented Mar 22, 2024

cc @DragaDoncila

@github-actions github-actions bot added the tests Something related to our tests label Mar 22, 2024
@lucyleeow lucyleeow mentioned this pull request Mar 22, 2024
Merged
@DragaDoncila
Copy link
Copy Markdown
Contributor

DragaDoncila commented Mar 22, 2024

@lucyleeow definitely agree with moving these actions. Let's also move the ViewerToggleAction imo. I don't see the benefit of keeping it separate from its usage until and unless we actually run into a need for it.

@codecov
Copy link
Copy Markdown

codecov bot commented Mar 22, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.33%. Comparing base (f327d1f) to head (9c2328e).
Report is 7 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #6767      +/-   ##
==========================================
- Coverage   92.40%   92.33%   -0.08%     
==========================================
  Files         613      613              
  Lines       54738    54747       +9     
==========================================
- Hits        50581    50548      -33     
- Misses       4157     4199      +42

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Czaki
Copy link
Copy Markdown
Collaborator

Czaki commented Mar 25, 2024

@lucyleeow It looks like dropping python 3.8 introduces conflicts

@lucyleeow
Copy link
Copy Markdown
Contributor Author

lucyleeow commented Mar 25, 2024

Thanks @Czaki, fixed. Are you interested in reviewing?

@lucyleeow
Copy link
Copy Markdown
Contributor Author

lucyleeow commented Mar 26, 2024

CI error due to timeout and I don't think related.

DragaDoncila
DragaDoncila approved these changes Mar 27, 2024
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.

Looks good to me! I really do think these actions don't make sense without qt. Pulled it down and played with it too.

jni
jni approved these changes Mar 28, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@jni jni left a comment

Choose a reason for hiding this comment

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

I was just chatting with @DragaDoncila and I am fine with this but I wanted to flag that even though these things don't make sense without Qt now, I think in the future they would make sense regardless:

  • when using a different UI such as a web-UI
  • when using a virtual UI to compose an image/animation which then gets rendered off-screen and saved as an image

@DragaDoncila points out that at that point we can take ~all of the Qt menu actions and move them in some intermediate "on-canvas" actions to distinguish from model-only actions.

@lucyleeow
Copy link
Copy Markdown
Contributor Author

lucyleeow commented Mar 28, 2024 •
edited
Loading

Agreed. I think there are other things in the qt folder are just UI specific, not necessarily Qt specific. We don't have a UI but backend agnostic folder atm.

@lucyleeow lucyleeow added the ready to merge Last chance for comments! Will be merged in ~24h label Mar 28, 2024
@DragaDoncila DragaDoncila merged commit e05ca78 into napari:main Apr 1, 2024
@DragaDoncila DragaDoncila removed the ready to merge Last chance for comments! Will be merged in ~24h label Apr 1, 2024
@lucyleeow lucyleeow deleted the mv_view_actions branch April 1, 2024 22:13
@GenevieveBuckley GenevieveBuckley mentioned this pull request Apr 8, 2024
Closed
@lucyleeow lucyleeow mentioned this pull request Apr 9, 2024
Merged
DragaDoncila pushed a commit that referenced this pull request Apr 11, 2024
@lucyleeow @Czaki
Remove redundant _view_actions.py (#6821)
2d2cac6 
# References and relevant issues
closes #396

Forgot to delete in #6767

Co-authored-by: Grzegorz Bokota <bokota+github@gmail.com>
@lucyleeow lucyleeow mentioned this pull request May 16, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jni jni jni approved these changes

@DragaDoncila DragaDoncila DragaDoncila approved these changes

Assignees

No one assigned

Labels

maintenance PR with maintance changes, qt Relates to qt tests Something related to our tests

Projects

None yet

Milestone

0.5.0

Development

Successfully merging this pull request may close these issues.

4 participants

@lucyleeow @DragaDoncila @Czaki @jni