Horizontal layer buttons #2566

javerbukh · 2023-11-13T20:25:04Z

Description

Screen.Recording.2023-11-17.at.3.13.54.PM.mov

This pull request converts the layer select dropdown menu in Plot Options into a horizontal panel of buttons. Both single and multiselect are supported in this PR. The work left to do is as follows:

Closes #2491

Change log entry

Is a change log needed? If yes, is it added to CHANGES.rst? If you want to avoid merge conflicts,
list the proposed change log here for review and add to CHANGES.rst before merge. If no, maintainer
should add a no-changelog-entry-needed label.
Convert the layer select dropdown into a horizontal panel of buttons. [Horizontal layer buttons #2566]

Checklist for package maintainer(s)

This checklist is meant to remind the package maintainer(s) who will review this pull request of some common things to look for. This list is not exhaustive.

Are two approvals required? Branch protection rule does not check for the second approval. If a second approval is not necessary, please apply the trivial label.
Do the proposed changes actually accomplish desired goals? Also manually run the affected example notebooks, if necessary.
Do the proposed changes follow the STScI Style Guides?
Are tests added/updated as required? If so, do they follow the STScI Style Guides?
Are docs added/updated as required? If so, do they follow the STScI Style Guides?
Did the CI pass? If not, are the failures related?
Is a milestone set? Set this to bugfix milestone if this is a bug fix and needs to be released ASAP; otherwise, set this to the next major release milestone.
After merge, any internal documentations need updating (e.g., JIRA, Innerspace)?

jdaviz/configs/default/plugins/plot_options/plot_options.py

kecnry · 2023-11-13T20:43:10Z

Excited to give this a spin and pleasantly relieved at how lightweight the diff is so far!

Color change and multiselect working Move logic to LayerSelect

kecnry

This is coming together! I still see some lag, so will take a closer look through the logic tomorrow and see if we can avoid that as much as possible 🤞

jdaviz/core/template_mixin.py

* redundant and conflicting with tab backgrounds

kecnry

This is really getting there!

Note that we have decided to postpone:

the bug (also on main) of the image visibility eye button not actually toggling the visibility
optimizing this so that changes to a layer color don't rebuild the entire dictionary but instead edit it in place
style fine-tuning

kecnry · 2023-11-17T19:54:33Z

jdaviz/configs/default/plugins/plot_options/plot_options.py

+        # Determine layers visible in selected viewer(s) - consider mixed to be visible
+        visible_layers = [layer['label'] for layer in self.layer.items if not layer['is_subset'] and (layer['visible'] or layer['mixed_visibility'])]  # noqa


@rosteen - does this change make sense to you? Now that we have this info already parsed, we can avoid the double-loop and can explicitly handle the mixed case - this means that if a layer is visible in one selected viewer, but not all, it will still be included in the logic. The alternative would be to only act all layers visible in all selected viewers, but I think you said you intended for this behavior, right?

Seems fine to me, and yeah I preferred updating all visible layers in any selected viewer.

kecnry · 2023-11-17T19:55:50Z

jdaviz/core/template_mixin.py

             "color": layer.state.color,
-             "icon": self.app.state.layer_icons.get(layer.layer.label)}
+             "all_colors_to_label": label_to_color.get(layer.layer.label, False),


can we merge color and all_colors_to_label (some updates will also be needed to plugin_layer_select_tabs.vue if so)? Maybe just color = label_to_color.get(layer.layer.label, layer.state.color)?

We could, but I'm not sure what the benefit would be.

jdaviz/core/template_mixin.py

kecnry · 2023-11-17T19:58:36Z

jdaviz/core/template_mixin.py

-        layer_labels = [layer.layer.label for layer in layers]
+        layer_labels = [layer.layer.label for layer in layers if self.app.state.layer_icons.get(layer.layer.label)]  # noqa


what does this change do?

Sometimes this gets called before the icon has been created, which then crashes when we try to sort self.items by the icon key.

codecov · 2023-11-17T20:15:40Z

Codecov Report

Attention: 9 lines in your changes are missing coverage. Please review.

Comparison is base (cfe7e25) 90.84% compared to head (bea110d) 90.79%.
Report is 7 commits behind head on main.

❗ Current head bea110d differs from pull request most recent head 32a75df. Consider uploading reports for the commit 32a75df to get more accurate results

Files	Patch %	Lines
jdaviz/core/template_mixin.py	90.00%	9 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2566      +/-   ##
==========================================
- Coverage   90.84%   90.79%   -0.05%     
==========================================
  Files         160      160              
  Lines       19357    19475     +118     
==========================================
+ Hits        17584    17682      +98     
- Misses       1773     1793      +20

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

rosteen · 2023-11-20T15:46:14Z

Starting to test this, and it concerns me that the color swatch is so laggy to update when you click a layer. I also get an "add large data?" warning from Glue every time I click one of the layer tabs, which makes me worry that something unintended is happening, although my guess is that's from the histogram viewer:

Screen.Recording.2023-11-20.at.10.40.11.AM.mov

Is the color of the colorbar in the histogram viewer supposed to match the selected color for the layer, or is that a future feature?

javerbukh · 2023-11-20T15:54:04Z

@rosteen It's the same amount of lag when switching layers as on main, at least the was the case with my testing. The histogram color should be updating but I think @kecnry found that was an issue introduced in the RGB presets code. I saw the warning messages as well but I am not sure where they are coming from, although the histogram is a good guess.

rosteen

Alright, if the issues I saw are unrelated then I can approve. I like the styling.

javerbukh · 2023-11-20T15:59:20Z

Sounds good! Thank you @kecnry and @rosteen , merging when CI passes.

* Layers now represented as horizontal buttons Color change and multiselect working Move logic to LayerSelect * Fix codestyle * Fix codestlye and remove comments * Viewer multiselect mixed state working * Working on handling edge cases of mixed state * Use messages in LayerSelect to call parts of _on_layers_changed * Remove unused elements * Fix button colors not updating after unmix state * Fix codestyle * Attempt 2 at fixing code style * Fix CI, put buttons in icon order * Code style again * Fix more tests * Everything handled by _on_layers_changed * button styling/tooltip based on (mixed)-color/visibility * improve tab styling * avoid second loop when applying RGB presets * improved handling of colormap and mixed colormode * remove elevation * redundant and conflicting with tab backgrounds * use all_color_to_label in tab styling * Mixed visibility and color represented in button * improved handling of subset color when in colormap * improved dark-theme support * tooltip and RGB preset support for mixed visibility * Visible is set to layer visible and bitmap visible * avoid displacement when no tabs selected * Fix CI tests * RGB presets: handle unmixing state * Address review comments * Forgot one change * for styling, always check if item in list, not as substring * Update changelog * Add tests --------- Co-authored-by: Kyle Conroy <kyleconroy@gmail.com>

kecnry reviewed Nov 13, 2023

View reviewed changes

jdaviz/configs/default/plugins/plot_options/plot_options.py Outdated Show resolved Hide resolved

Layers now represented as horizontal buttons

bf22850

Color change and multiselect working Move logic to LayerSelect

javerbukh force-pushed the horizontal-layer-buttons branch from ddeb60c to bf22850 Compare November 14, 2023 18:36

javerbukh added 12 commits November 14, 2023 13:40

Fix codestyle

2de03cf

Fix codestlye and remove comments

6483a01

Viewer multiselect mixed state working

3c31e18

Working on handling edge cases of mixed state

9ab3162

Use messages in LayerSelect to call parts of _on_layers_changed

f34e42b

Remove unused elements

1fd124f

Fix button colors not updating after unmix state

1ef9bdb

Fix codestyle

93ce319

Attempt 2 at fixing code style

fdf0703

Fix CI, put buttons in icon order

85c1a3b

Code style again

7b2ef3d

Fix more tests

10aaa91

kecnry reviewed Nov 16, 2023

View reviewed changes

jdaviz/core/template_mixin.py Outdated Show resolved Hide resolved

jdaviz/core/template_mixin.py Outdated Show resolved Hide resolved

jdaviz/core/template_mixin.py Outdated Show resolved Hide resolved

javerbukh and others added 13 commits November 17, 2023 11:28

Everything handled by _on_layers_changed

60ad145

button styling/tooltip based on (mixed)-color/visibility

639eb87

improve tab styling

17f4e33

avoid second loop when applying RGB presets

e4650ff

improved handling of colormap and mixed colormode

f9640e0

remove elevation

9a3c30e

* redundant and conflicting with tab backgrounds

use all_color_to_label in tab styling

d3baec2

Mixed visibility and color represented in button

939d3be

improved handling of subset color when in colormap

611ea92

improved dark-theme support

1f56e99

tooltip and RGB preset support for mixed visibility

fb64d0f

Visible is set to layer visible and bitmap visible

fd4353d

avoid displacement when no tabs selected

5d50565

Fix CI tests

f0a7966

kecnry reviewed Nov 17, 2023

View reviewed changes

kecnry added this to the 3.8 milestone Nov 17, 2023

RGB presets: handle unmixing state

37c9605

javerbukh added 2 commits November 17, 2023 15:21

Address review comments

10d1f20

Forgot one change

02661cc

javerbukh marked this pull request as ready for review November 17, 2023 21:10

javerbukh requested review from rosteen, pllim, bmorris3, haticekaratay and cshanahan1 as code owners November 17, 2023 21:10

kecnry approved these changes Nov 20, 2023

View reviewed changes

kecnry and others added 2 commits November 20, 2023 09:46

for styling, always check if item in list, not as substring

a0e8ee2

Update changelog

bea110d

Add tests

32a75df

rosteen approved these changes Nov 20, 2023

View reviewed changes

javerbukh enabled auto-merge (squash) November 20, 2023 15:59

javerbukh merged commit cde8d3f into spacetelescope:main Nov 20, 2023
12 of 13 checks passed

kecnry mentioned this pull request Nov 22, 2023

UI/UX tweaks for layer tabs #2574

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Horizontal layer buttons #2566

Horizontal layer buttons #2566

javerbukh commented Nov 13, 2023 •

edited

Loading

kecnry commented Nov 13, 2023

kecnry left a comment

kecnry left a comment

kecnry Nov 17, 2023

rosteen Nov 20, 2023

kecnry Nov 17, 2023

javerbukh Nov 17, 2023

kecnry Nov 17, 2023

javerbukh Nov 17, 2023

codecov bot commented Nov 17, 2023 •

edited

Loading

rosteen commented Nov 20, 2023

javerbukh commented Nov 20, 2023

rosteen left a comment

javerbukh commented Nov 20, 2023

		# Determine layers visible in selected viewer(s) - consider mixed to be visible
		visible_layers = [layer['label'] for layer in self.layer.items if not layer['is_subset'] and (layer['visible'] or layer['mixed_visibility'])] # noqa

		layer_labels = [layer.layer.label for layer in layers]
		layer_labels = [layer.layer.label for layer in layers if self.app.state.layer_icons.get(layer.layer.label)] # noqa

Horizontal layer buttons #2566

Horizontal layer buttons #2566

Conversation

javerbukh commented Nov 13, 2023 • edited Loading

Description

Change log entry

Checklist for package maintainer(s)

kecnry commented Nov 13, 2023

kecnry left a comment

Choose a reason for hiding this comment

kecnry left a comment

Choose a reason for hiding this comment

kecnry Nov 17, 2023

Choose a reason for hiding this comment

rosteen Nov 20, 2023

Choose a reason for hiding this comment

kecnry Nov 17, 2023

Choose a reason for hiding this comment

javerbukh Nov 17, 2023

Choose a reason for hiding this comment

kecnry Nov 17, 2023

Choose a reason for hiding this comment

javerbukh Nov 17, 2023

Choose a reason for hiding this comment

codecov bot commented Nov 17, 2023 • edited Loading

Codecov Report

rosteen commented Nov 20, 2023

javerbukh commented Nov 20, 2023

rosteen left a comment

Choose a reason for hiding this comment

javerbukh commented Nov 20, 2023

javerbukh commented Nov 13, 2023 •

edited

Loading

codecov bot commented Nov 17, 2023 •

edited

Loading