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

Define submenus with their actions, remove submenus.py file #6848

Merged
merged 9 commits into from
May 28, 2024

Conversation

lucyleeow
Copy link
Contributor

@lucyleeow lucyleeow commented Apr 16, 2024

References and relevant issues

Towards #6516

Description

Remove _submenus.py file, in favour of defining them with their actions.

I am not 100% happy with this though because some menu actions will have a qt-actions and non-qt-actions file, in which case it would be again confusing for the dev to guess where the submenus are defined.

Luckily the 3 menus that do have submenus (layer, file and view) only have one action file.

Note I have had to move submenu registration inside init_qactions.

Not sure what to do here, open to suggestions and happy to close this as well.

@github-actions github-actions bot added the qt Relates to qt label Apr 16, 2024
@lucyleeow
Copy link
Contributor Author

cc @DragaDoncila @psobolewskiPhD

Copy link

codecov bot commented Apr 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.43%. Comparing base (ded87bf) to head (fef1476).
Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6848      +/-   ##
==========================================
- Coverage   92.48%   92.43%   -0.06%     
==========================================
  Files         612      611       -1     
  Lines       55153    55151       -2     
==========================================
- Hits        51009    50977      -32     
- Misses       4144     4174      +30     

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

@lucyleeow
Copy link
Contributor Author

After discussing with @DragaDoncila, we think all the menu bar actions should be defined in one file in _qt - currently the only non-qt actions are some help menu actions that open a URL. Although these actions technically do not require QT as they are defined for the purpose of living in a menu, it should be considered a 'GUI' action.

The only non-qt actions are the layer actions, which are not in the menubar.

This resolves any previous conflict for this PR as I think submenus can live with their actions, as only one file will exist for each menubar menu.

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.

This looks good to me. If someone wants to add a new action they now should only need to edit one of these files in two spots right? Unless they're adding a brand new menubar menu? @jni do you prefer this?

@lucyleeow
Copy link
Contributor Author

lucyleeow commented May 3, 2024

one of these files in two spots right?

2 spots only if they want to add a submenu as well. Only one spot if they are just adding an action.

Note if the action is a method, you'd add/amend the method in the file where the class is defined and update the action file.
Without the decorator we still have one 'extra' file to change. Note if you want to be able to add a submenu in the decorator as well, it makes things complicated as submenus may be re-used so adding it in a decorator is not ideal. And if we don't allow submenu addition it in a decorator you would still have to change 2 files to add an action AND a submenu.

But do like that we can see all the actions for a menu in one file, and its useful for checking how items are grouped inside the menu.

@lucyleeow
Copy link
Contributor Author

lucyleeow commented May 7, 2024

The failing test does not seem to be related? I've re-run it.

Some are timeout but e.g., test_last_point_is_visible_in_viewport may be legit? Passed on re-run

@Czaki
Copy link
Collaborator

Czaki commented May 7, 2024

merge conflict

@jni jni added the maintenance PR with maintance changes, label May 15, 2024
@jni jni added this to the 0.5.0 milestone May 15, 2024
@jni jni added the ready to merge Last chance for comments! Will be merged in ~24h label May 15, 2024
@lucyleeow
Copy link
Contributor Author

merge conflict resolved, cc @Czaki

jni pushed a commit that referenced this pull request May 20, 2024
# References and relevant issues
Closes #6744
Related:
#6848 (comment)

# Description
Move all help actions to `_qt/` and remove lambdas and replace with
partials.
DragaDoncila pushed a commit that referenced this pull request May 24, 2024
# Description
As menu actions are all one file (see #6848 and #6883), update the file
docstrings to remove mention of 'non-qt' actions.
@jni
Copy link
Member

jni commented May 27, 2024

Merged in main, which included removing the HELP menu actions from napari/_app_model/_app.py. I think that's correct, based on the changes in main. (btw in case folks haven't found it yet, I highly recommend git config --global merge.conflictStyle diff3.) If tests pass, any core dev please feel free to merge this!

@DragaDoncila DragaDoncila merged commit 07dbc0a into napari:main May 28, 2024
34 checks passed
@DragaDoncila DragaDoncila removed the ready to merge Last chance for comments! Will be merged in ~24h label May 28, 2024
@lucyleeow lucyleeow deleted the submenu branch May 28, 2024 01:38
andy-sweet pushed a commit to andy-sweet/napari that referenced this pull request May 28, 2024
# Description
As menu actions are all one file (see napari#6848 and napari#6883), update the file
docstrings to remove mention of 'non-qt' actions.
andy-sweet pushed a commit to andy-sweet/napari that referenced this pull request May 28, 2024
)

# References and relevant issues
 Towards napari#6516

# Description
Remove `_submenus.py` file, in favour of defining them with their
actions.

I am not 100% happy with this though because some menu actions will have
a qt-actions and non-qt-actions file, in which case it would be again
confusing for the dev to guess where the submenus are defined.

Luckily the 3 menus that do have submenus (layer, file and view) only
have one action file.

Note I have had to move submenu registration inside `init_qactions`.

Not sure what to do here, open to suggestions and happy to close this as
well.

---------

Co-authored-by: Juan Nunez-Iglesias <jni@fastmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance PR with maintance changes, qt Relates to qt
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants