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

Layers dropdown to follow visible Data layer #244

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

pllim
Copy link
Contributor

@pllim pllim commented Jul 23, 2021

Description

This is to solve spacetelescope/jdaviz#743 for Jdaviz/Imviz. cc @astrofrog @maartenbreddels

Also fix a bug introduced by #249 cc @rosteen (moved to #250)

If this is accepted, we will need a release for Jdaviz.

  • Fix Layers selecting wrong data when markers are added with imviz.add_markers().

@codecov
Copy link

codecov bot commented Jul 23, 2021

Codecov Report

Merging #244 (4b2d783) into master (296f0f2) will increase coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #244      +/-   ##
==========================================
+ Coverage   89.17%   89.19%   +0.02%     
==========================================
  Files          82       82              
  Lines        4037     4046       +9     
==========================================
+ Hits         3600     3609       +9     
  Misses        437      437              
Impacted Files Coverage Δ
glue_jupyter/bqplot/image/layer_artist.py 88.54% <100.00%> (+0.08%) ⬆️
glue_jupyter/widgets/layer_options.py 90.56% <100.00%> (+1.67%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 296f0f2...4b2d783. Read the comment docs.

@pllim pllim force-pushed the layers-follow-data branch from 1c0f95f to e1975f8 Compare August 10, 2021 18:28
@pllim
Copy link
Contributor Author

pllim commented Aug 10, 2021

With this patch, I still get something like this when I add markers (i.e., link table data to images). They look confusing but the layer auto selection still works as expected for Imviz. 🤷

Screenshot 2021-08-10 170038

@astrofrog
Copy link
Member

Thanks for the PR! The logic isn't quite right yet though - in the following example if I have a subset layer selected and I toggle the visibility of a data layer it will change the selection when it doesn't have to:

Peek 2021-08-12 14-40

I think the logic can actually be simplified. Basically if the current selected layer is visible, leave as is, otherwise select the next visible layer, and if there are none, the next previous visible layer - regardless of whether they are data or subset?

@mariobuikhuizen - on the vue side, I am wondering if we can make things nicer by not actually allowing the user to select an item in the list if it is not visible? Maybe the logic for moving the selection when visibility is toggled could then even be on the vue side too?

@pllim
Copy link
Contributor Author

pllim commented Aug 12, 2021

I toggle the visibility of a data layer it will change the selection when it doesn't have to

What do you mean? When you hide the second image, the first is shown. When people mess with the options, they want to see things change on what is actually shown.

While I don't know why the subset stuff is poluting the dropdown, I think the toggle logic is less confusing than it currently is.

If there is a simpler way to do this, I'll be happy too.

@astrofrog
Copy link
Member

The subset stuff is in the dropdown so that in principle you can change the visual properties of the subsets too. I realised that in jdaviz you may not want this though, so maybe we should have a way to remove subset layers from this dropdown (as an opt-in option that could be enabled in jdaviz).

Layers can have transparency so it doesn't always make sense to select the 'top' visible layer. I think we should only change the selected layer if the current one has its visibility turned off (that was the more general point of my animation above). Note that there are viewers (in the general use case) where one might also only want to see some subsets and no data hence why I think the logic shouldn't be specific to data vs subset. That issue could be resolved by my previous point that subsets could be hidden in jdaviz if you wanted so there would only be data objects anyway.

@pllim
Copy link
Contributor Author

pllim commented Aug 12, 2021

Well, as far as Imviz is concerned, no one is using the transparency thingy. In fact, they are even confused why there is a Data dropdown box at all. But I understand you have other use cases too, so I'll hold off on this until @astrofrog and @mariobuikhuizen can figure out a path forward. But we do this need for MVP, so a fast resolution would be nice. Thanks!

@pllim pllim marked this pull request as draft August 12, 2021 15:54
@pllim
Copy link
Contributor Author

pllim commented Aug 12, 2021

Given #250 has been superseded by #251 , this is not longer merge-able. I converted this to draft so we don't forget about it.

@astrofrog
Copy link
Member

Well, as far as Imviz is concerned, no one is using the transparency thingy

Another real use case for the drop down is if you want to overlay contours and images though which is a jdaviz use case (one layer might be the contours and the other the image)

@pllim
Copy link
Contributor Author

pllim commented Aug 12, 2021

Good point. Haven't gotten to fixing contour yet (spacetelescope/jdaviz#530) but it is not MVP, so... 😬

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants