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

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

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

Conversation

dalthviz
Copy link
Member

@dalthviz dalthviz commented Sep 11, 2023

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:

  • Creates individual classes per widget control (QtWidgetControlsBase, QtOpacityBlendingControls, QtEdgeColorControl, QtEdgeWidthSliderControl, etc) that handle the layer attributes <-> widget values connections.
  • Defines methods to add buttons and widget controls (_add_radio_button_mode, _add_push_button_action, add_annotation_widget_controls and add_display_widget_controls) in the base QtLayerControls class.
  • Move logic related to layer interactive mode and editable/visible widget controls handling to the base QtLayerControls class.
  • Update base controls class layout and napari style rules to start accomodating to the designs details at Create system for layer controls layout and visuals #5358 (comment)

Notes

  • ⚠️ As mentioned, the current changes on the branch only make sure that the Shapes layer controls work so trying to use napari to create other layer types running from this branch will probably end up showing errors/failing to create the respective layer controls. To prevent errors/failing tests the old base layer controls class is still there for the controls of layer types still not changed. After moving all the control layer classes to use the new base definition it should be removed.
  • As mentioned at Create system for layer controls layout and visuals #5358 (comment) the buttons order will be preserved here. Also not just the order but the layout itself, including spacing between buttons
  • It would be nice to first get some feedback on the implementation/code refactor done to see if that makes sense (that's why I only changed how things work for the Shapes layer controls for the moment). If the approach actually makes sense or after incorporating given feedback things look good (in terms of the implementation/code refactor), then I will continue doing here the changes for other layer types. So, although this is still WIP, feel free to comment here what do you think/other ideas to implement this!

@github-actions github-actions bot added design Design discussion qt Relates to qt labels Sep 11, 2023
@github-actions github-actions bot added the tests Something related to our tests label Sep 12, 2023
@codecov
Copy link

codecov bot commented Sep 12, 2023

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 ⚠️
...r_controls/widgets/qt_opacity_blending_controls.py 81.13% 10 Missing ⚠️
..._qt/layer_controls/widgets/qt_edge_width_slider.py 91.42% 3 Missing ⚠️
...i/_qt/layer_controls/widgets/qt_text_visibility.py 93.10% 2 Missing ⚠️
.../layer_controls/widgets/qt_widget_controls_base.py 89.47% 2 Missing ⚠️
.../_qt/layer_controls/qt_layer_controls_container.py 85.71% 1 Missing ⚠️
Additional details and impacted files
@@            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     

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

@dalthviz dalthviz changed the title [WIP] Layer controls designs implementation and code refactor Layer controls designs implementation and code refactor Sep 15, 2023
@dalthviz dalthviz changed the title Layer controls designs implementation and code refactor [WIP] Layer controls designs implementation and code refactor Sep 20, 2023
@jni
Copy link
Member

jni commented Mar 2, 2024

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:

  • love collapsing the "display" section
  • we didn't like that the controls scroll bar is always there, even if there is plenty of room. The scroll bar should disappear (and the full width used) if there is space.
  • when display is collapsed, the min height for the controls is too much — I can't actually make use of the collapsing to get more room for the layer list
  • the spacing between the buttons is too much now — it should be like the original, or the buttons can be bigger and fill up the space. Also the button arrangement should be the same — it's weird that the bring-to-front/send-to-back buttons are miles away from each other now. 😂

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

…buttons spacing and arrangement to be closer with the current/original layout
@dalthviz
Copy link
Member Author

dalthviz commented Mar 4, 2024

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
Copy link
Member

jni commented Mar 8, 2024

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
Copy link
Contributor

@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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
design Design discussion qt Relates to qt tests Something related to our tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants