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

Move help actions under `_qt` and remove lambdas #6883

Open lucyleeow opened 2 weeks ago

lucyleeow commented 2 weeks ago

References and relevant issues

Closes https://github.com/napari/napari/issues/6744 Related: https://github.com/napari/napari/pull/6848#issuecomment-2089374696

Description

Move all help actions to _qt/ and remove lambdas and replace with partials.

codecov[bot] commented 2 weeks ago

Codecov Report

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

Project coverage is 92.44%. Comparing base (f66fce6) to head (dc286b5). Report is 18 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #6883 +/- ## ========================================== - Coverage 92.45% 92.44% -0.02% ========================================== Files 617 616 -1 Lines 55156 55150 -6 ========================================== - Hits 50993 50981 -12 - Misses 4163 4169 +6 ```

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

lucyleeow commented 2 weeks ago

cc @Czaki @DragaDoncila :pray:

jni commented 1 day ago

I apologise if this is me being a goldfish 🐠, but why are they moved to Qt? I see nothing Qt related in them, and presumably if we have a different front-end one day, these actions can be used unchanged?

jni commented 1 day ago

Nope, not me being a goldfish, just me not clicking through, or rather clicking "open in new tab" then not looking. 🤦

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.

jni commented 1 day ago

Anyway I'm not super sure I'm convinced about the design but it's not a hard thing to revert.

lucyleeow commented 1 day ago

The new layer menu actions have opened up some questions too. I'm just about to open a PR to start a discussion about this stuff. Please share your views there @jni (i'll ping you)!!

DragaDoncila commented 3 hours ago

I see nothing Qt related in them, and presumably if we have a different front-end one day, these actions can be used unchanged?

Yeah they're not qt related but they are only useful with a GUI. Since we don't have a "non qt but defs gui" folder, we've decided to move these here so that they're explicitly considered if a time comes when we want to make them available via a different front end.

I'm not in love with this either, but as Lucy says we've run into this question a few times so I think best is to merge this, and think about the separation of headless-gui-qt a bit more carefully in a broader sense.

DragaDoncila commented 3 hours ago

@jni I will add ready-to-merge here, but please feel free to request changes if you want more discussion