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

Viewer creator in app toolbar #94

Merged
merged 10 commits into from
Mar 11, 2024
Merged

Conversation

kecnry
Copy link
Member

@kecnry kecnry commented Mar 6, 2024

NOTE: this should be tested with jdaviz 3.8.x since this does not include updates for jdaviz 3.9 covered in #68.

This adds a viewer creator dropdown in the app-level toolbar which can create viewers, including phase viewers attached to a specific ephemeris.

Screen.Recording.2024-03-08.at.2.05.20.PM.mov

The current behavior is to first check for an existing viewer with a matching label and if it exists, clone the existing viewer (keeping data visibilities and plot options, etc), and otherwise create a new viewer.

TODO:

  • testing with renaming ephemeris
  • either here or follow-up: move ephemeris logic into viewer metadata instead of relying on viewer label
  • decide whether to allow all time viewers to be closed (and if so, test layer-visibility logic assumptions for new phase viewers)
  • upstream support for automatically opening data menu on blank viewer: Open data menu on new blank viewer jdaviz#2742

Once merged:

  • rebase the 3.9-updates and TPF feature branch on top of main and add support for image viewers (might require fixing clone support first)
  • design and create ticket for what to show when no viewers exist?

The styling will also be improved by upstream spacetelescope/jdaviz#2743

kecnry added 3 commits March 4, 2024 16:16
still needs better ephemeris support
* re-introduce if/when upstream support is available (which would then give us automatic inherited tooltips, when also available upstream)
Copy link

codecov bot commented Mar 6, 2024

Codecov Report

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

Project coverage is 94.05%. Comparing base (db3548d) to head (f699c82).

Files Patch % Lines
lcviz/plugins/ephemeris/ephemeris.py 87.17% 5 Missing ⚠️
lcviz/helper.py 95.00% 1 Missing ⚠️
lcviz/parsers.py 80.00% 1 Missing ⚠️
lcviz/plugins/viewer_creator/viewer_creator.py 96.55% 1 Missing ⚠️
lcviz/viewers.py 90.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #94      +/-   ##
==========================================
- Coverage   94.18%   94.05%   -0.14%     
==========================================
  Files          35       37       +2     
  Lines        1548     1598      +50     
==========================================
+ Hits         1458     1503      +45     
- Misses         90       95       +5     

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

@kecnry kecnry mentioned this pull request Mar 6, 2024
8 tasks
@kecnry kecnry force-pushed the viewer-creator branch 2 times, most recently from 20f1998 to fb508d1 Compare March 8, 2024 16:23
@kecnry kecnry marked this pull request as ready for review March 8, 2024 19:23
@kecnry kecnry requested a review from bmorris3 March 8, 2024 19:23
@rosteen
Copy link
Contributor

rosteen commented Mar 8, 2024

Functionality seems to work but I think the button really needs a tooltip - is there some difficulty with adding one to a downstream (from Jdaviz) component like this?

@kecnry
Copy link
Member Author

kecnry commented Mar 11, 2024

Yes, this will be covered by spacetelescope/jdaviz#2743 and spacetelescope/jdaviz#2744 upstream and pulled in here by #68.

@rosteen
Copy link
Contributor

rosteen commented Mar 11, 2024

Yes, this will be covered by spacetelescope/jdaviz#2743 and spacetelescope/jdaviz#2744 upstream and pulled in here by #68.

Got it, thanks - I figured it wasn't trivial to insert something into out tooltip registry as-is. In that case I think this looks good to get in, I worry slightly about the number of things in the duplication menu getting out of hand, but also think it's very unlikely someone would create that many viewers.

@kecnry kecnry merged commit 7b78df5 into spacetelescope:main Mar 11, 2024
9 of 11 checks passed
@kecnry kecnry deleted the viewer-creator branch March 11, 2024 14:23
@kecnry kecnry restored the viewer-creator branch March 11, 2024 15:23
@kecnry kecnry deleted the viewer-creator branch March 11, 2024 15:24
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