napari / napari

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

[WIP] Layer controls designs implementation and code refactor #6219

Open dalthviz opened 9 months ago

dalthviz commented 9 months ago

References and relevant issues

Part of #5358

Description

Initial implementation of the designs being discussed at #5358 while taking into account possible future features that could affect the way the layer controls work/are instantiated (multi-layer selection and LayerGroup definition).

The current changes only cover the controls for the Shapes layer type. From the GUI you can see something like:

controls2

Windows MacOS Linux
imagen image imagen

Some of the elements that this PR changes:

Notes

codecov[bot] commented 9 months ago

Codecov Report

Attention: Patch coverage is 90.11494% with 43 lines in your changes are missing coverage. Please review.

Project coverage is 92.40%. Comparing base (26976b5) to head (392406a).

Files Patch % Lines
...apari/_qt/layer_controls/qt_layer_controls_base.py 87.68% 25 Missing :warning:
...r_controls/widgets/qt_opacity_blending_controls.py 81.13% 10 Missing :warning:
..._qt/layer_controls/widgets/qt_edge_width_slider.py 91.42% 3 Missing :warning:
...i/_qt/layer_controls/widgets/qt_text_visibility.py 93.10% 2 Missing :warning:
.../layer_controls/widgets/qt_widget_controls_base.py 89.47% 2 Missing :warning:
.../_qt/layer_controls/qt_layer_controls_container.py 85.71% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #6219 +/- ## ========================================== - Coverage 92.48% 92.40% -0.09% ========================================== Files 614 621 +7 Lines 55181 55460 +279 ========================================== + Hits 51036 51248 +212 - Misses 4145 4212 +67 ```

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

isabela-pf commented 8 months ago

As anticlimactic as it might be for a first review, I gave the QSS files a look and all looks correct to me so far! The screen shots (thank you for those!) are what I expect as well.

Thank you for getting this started and identifying the feedback you need. I will circle back as more changes go forward, but mostly I'm impressed to see the details percolating so clearly from the start.

brisvag commented 8 months ago

Sorry for leaving this hanging, will probably find the time to get back to this once the 0.4.19 craze is over :)

Czaki commented 4 months ago

@dalthviz This PR still has [WIP] in the title but I see only merging main activity. Did you forget to remove [WIP]?

brisvag commented 4 months ago

I think they're waiting for feedback from us; for now, I haven't had time to dive in and anyways I was waiting for the v0.4.19 to land so folks have mroe time to discuss here.

dalthviz commented 4 months ago

Yep, waiting for feedback here as Lorenzo says. So basically, I left this as [WIP] since this still needs to replicate the changes done to the Shapes layer to the other types of layers. The thing is that, before doing the full refactor for other layers types, I would like to get general feedback about the refactor approach implemented. So to know first if there is something in the approach to improve or if there are any other ideas and in general if this approach is a good enough base for future features like multilayers selection or group layers (so future refactor/changes are smaller/more contained hopefully 🤞🏼 ).

I have been merging with latest main just to fix merge conflicts when they appear and check that the changes here are still compatible with latest main :)

jni commented 3 months ago

Hey @dalthviz,

Are you looking for feedback on the code or on the functionality? We played around with this during the PR party and identified the following issues with the functionality:

But maybe you are looking for code and not functional suggestions, in which case, hold please. 😅 🙏

dalthviz commented 3 months ago

Hi @jni ! Thank you for the feeback! Although a more code related review could be awesome (to see if the refactor approach I took makes sense), any feedback is appreciated! Checking the funtionality related feedback, maybe people would prefer things to look/work something like the following then?:

controls2

Pushed a commit with some changes in case someone wants to check things locally :)

Also, what do you think @isabela-pf ?

jni commented 3 months ago

Checking the funtionality related feedback, maybe people would prefer things to look/work something like the following then?:

Yes, that looks great!

I'll try to make some time this coming week for some code review. Thank you!

isabela-pf commented 3 months ago

@dalthviz This works for me. Thank you for making the update! Thank you for the tag as well. Glad to see this getting some review!