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

Migrate Debug menu to app-model #6915

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

Conversation

lucyleeow
Copy link
Contributor

References and relevant issues

Towards #4860

Description

Migrates debug menu to app-model

@github-actions github-actions bot added the qt Relates to qt label May 15, 2024
@lucyleeow lucyleeow changed the title Migrate debug menu to app-model Migrate Debug menu to app-model May 15, 2024
Copy link

codecov bot commented May 15, 2024

Codecov Report

Attention: Patch coverage is 66.66667% with 17 lines in your changes missing coverage. Please review.

Project coverage is 92.42%. Comparing base (f640234) to head (bde3173).

Files Patch % Lines
napari/_qt/_qapp_model/qactions/_debug.py 54.83% 14 Missing ⚠️
napari/_qt/qt_main_window.py 81.25% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6915      +/-   ##
==========================================
- Coverage   92.48%   92.42%   -0.07%     
==========================================
  Files         612      612              
  Lines       55184    55189       +5     
==========================================
- Hits        51038    51006      -32     
- Misses       4146     4183      +37     

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

@lucyleeow
Copy link
Contributor Author

Note that the enablement here is not correct currently, I realise in order to use a function call as an enablement we need to create a new context key, like in #6864

Need some thought on how best to do this.

@lucyleeow
Copy link
Contributor Author

In light of all the discussion about contexts (see: pyapp-kit/app-model#199, pyapp-kit/app-model#198, pyapp-kit/app-model#196), I've placed the debug 'is_set_trace_active' context key in QtViewer (could not place in NapariApplication as that is outside of Qt). Not 100% happy with this, thoughts welcome.

Note new changes depend on pyapp-kit/app-model#199.

@lucyleeow lucyleeow added this to the 0.5.0 milestone Jun 4, 2024
@lucyleeow
Copy link
Contributor Author

I think this is ready for review now.

Note that I have 'manually' updated the 'is_set_trace_active' context key on aboutToShow, similar to how it is done in #6864. This is not ideal but there is a on-going discussion about it (see: pyapp-kit/app-model#196 (comment), pyapp-kit/app-model#198 and this thread: pyapp-kit/app-model#199 (comment)) which is unlikely to be resolved soon.

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 and all works as expected! Agreed the manual update of the context on_about_to_show is not great but we're still working to a solution there so I think it's ok for now. Thanks @lucyleeow!

@@ -164,6 +165,10 @@ def __init__(
# done outside the widget. See #1571
self.setFocusPolicy(Qt.FocusPolicy.StrongFocus)

# Ideally this would be in `NapariApplication` but that is outside of Qt
self._ctx = create_context(self)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be nice to call this viewer_context or something

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
qt Relates to qt
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants