napari / napari

napari: a fast, interactive, multi-dimensional image viewer for python
https://napari.org
BSD 3-Clause "New" or "Revised" License
2.07k stars 410 forks source link

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

Open lucyleeow opened 1 month ago

lucyleeow commented 1 month ago

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.

lucyleeow commented 1 month ago

cc @DragaDoncila @psobolewskiPhD

codecov[bot] commented 1 month ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 92.43%. Comparing base (26976b5) to head (b24387e).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #6848 +/- ## ========================================== - Coverage 92.48% 92.43% -0.06% ========================================== Files 614 613 -1 Lines 55181 55178 -3 ========================================== - Hits 51036 51003 -33 - Misses 4145 4175 +30 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

lucyleeow commented 2 weeks ago

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.

lucyleeow commented 2 weeks ago

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 commented 1 week ago

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 commented 1 week ago

merge conflict

lucyleeow commented 1 day ago

merge conflict resolved, cc @Czaki