Jdaviz Launcher

[duytnguyendtn]

Description

This pull request adds a first-pass (i.e. ugly) launcher for Jdaviz with the ability to either select a config from a button or a filepath through #2124. Additional work is necessary to get this to the visual level of our wireframe.

Untitled.-.Jupyter.Notebook.-.Brave.2023-06-16.09-44-50.mp4

Because this is a UI element, this is expected to reduce coverage.

@mariobuikhuizen is investigating why the launcher isn't showing in voila, something Mario foresaw when prototyping this in April

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.

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)?

[codecov]

Codecov Report

Patch coverage: 13.51% and project coverage change: -0.14 ⚠️

Comparison is base (b365d80) 91.04% compared to head (60a2223) 90.91%.

❗ Current head 60a2223 differs from pull request most recent head 294c8d6. Consider uploading reports for the commit 294c8d6 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2257      +/-   ##
==========================================
- Coverage   91.04%   90.91%   -0.14%     
==========================================
  Files         151      152       +1     
  Lines       17025    17059      +34     
==========================================
+ Hits        15501    15509       +8     
- Misses       1524     1550      +26     

Impacted Files	Coverage Δ
jdaviz/cli.py	20.00% <0.00%> (-0.90%)	⬇️
jdaviz/core/launcher.py	15.62% <15.62%> (ø)

... and 3 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[pllim]

Definitely need a change log.

In addition of updating doc for existing CLI, the new launcher behavior also need to be documented for the users.

[duytnguyendtn]

Ok I think this is ready for re-review! One comment on the documentation part: I'm intentionally NOT adding documentation YET because I don't think this is ready for prime time yet. I think we should polish the UI AND think about what API we want to expose to our users (hence, why the launcher and the open function are buried under jdaviz.core.launcher and jdaviz.core.data_formats.open)

We should also think about what terminology to use, as I think we are close to being overloaded between load, open, launch

[pllim]

Without a file dialog, the standalone app behavior of this feature is not very intuitive for me. My workflow:

1. I go into my data directory.
2. I run jdaviz --theme=dark command to start Jdaviz without any file.
3. Launcher shows up as expected:

4. I cannot find a way to get the file dialog to pop up. A little annoying but no matter, I copy-paste a file name in the directory I started the app from. At this point, I assume it is running from the directory I started the app from, like when you run jupyter notebook. So, I only pasted the filename without path, e.g., jbt7a3020_drz.fits. I click that up arrow button.
5. I click "Imviz".
6. Launcher goes away and Imviz shows up.

❗ But there is no data loaded! Where did my data go? There is no error message either. Just an empty Imviz.

❗ I also tried using full path, still nothing happens. Imviz gets launched without any data.

💡 Side note: That up arrow is also not very intuitive. I only know to press it because I saw your video. Otherwise, I would have just pasted the filename and then click Imviz, ignoring the arrow altogether because I cannot tell what it is supposed to do just from the icon without any tooltip or documentation.

💡 If I then use the "Import Data" file dialog to load the same file, it loads fine.

[duytnguyendtn]

Thanks @pllim! Admittedly, this is the reason why I'm hesitant on exposing documentation to the user; I feel like we're still trying to figure out how exactly we want this launcher to work. Could you try #2267 (that's built off of this one) instead? I think the behavior there is closer to what you're expecting to happen here and we can just review everything together over there/close this PR. For posterity's sake (even though it probably doesn't matter), the behavior in THIS PR is:

• There is no file picker dialog. The user must enter the filepath into the textfield
• Pressing the "upload" icon (changed in PR2267 to a magnifying glass) autoidentifies the config and IMMEDIATELY launches the configuration. No need to press any of the config buttons below
• Selecting the corresponding viz button launches the config blank WITHOUT any data, ignoring anything in the filepath

[pllim]

Duy and I discussed the UI/UX offline and I agree that it can be improved over at #2267 and does not block this PR.

I updated voila from 0.4.0 to 0.4.1 and now the up arrow thingy works. 🤯

Thanks!

[kecnry]

This is a good first step towards building out the framework for this. I agree that we can wait to publicly document this until the UI and workflows are finalized/iterated in future follow-up PRs. I'm still a bit skeptical about the python vs vue building of the layout, but that can be swapped out in the future as well once we have a better feel for the feasibilities of both approaches.

[kecnry]

I think this definitely deserves to be quite "loud" in the change log entry as it technically is a breaking change to the CLI.

Discussed offline: after the launcher is more finalized and ready for public use, we might want to update the docs to suggest jdaviz filename directly and only keeping --layout as advanced functionality. But that can (and should) be deferred.

[pllim]

Now that change log is in, I am approving as promised. Thanks!

[duytnguyendtn]

@pllim Whoops I confused the --layout functionality with #2267. I fixed the changelog. Feel free to re-enable automerge if this is acceptable! 60a2223

[duytnguyendtn]

Merging, thanks for the eyes!