jdaviz icon indicating copy to clipboard operation
jdaviz copied to clipboard

feat: use Solara FileBrowser to import files and directories.

Open maartenbreddels opened this issue 3 years ago • 12 comments

Description

This file browser is more in line with the rest of the look and feel (not based on ipywidgets-controls, but on ipyvuetify).

Also, maintenance of this will be done by Maarten&Mario.

Also, looking at the comments at #573 this is implemented using:

  • Single click to select a directory or file
  • Import button for directory or file
  • Double click to quickly import a file.

Tooltips are added to make clear what each button does, or why it is disabled.

This requires solara >= 0.3

maartenbreddels avatar Jun 07 '22 14:06 maartenbreddels

Screen recording:

https://user-images.githubusercontent.com/1765949/172414464-07551493-0598-46f8-a82b-7cfd87d22227.mp4

maartenbreddels avatar Jun 07 '22 14:06 maartenbreddels

Codecov Report

:x: Patch coverage is 50.00000% with 16 lines in your changes missing coverage. Please review. :white_check_mark: Project coverage is 85.31%. Comparing base (fa11278) to head (71ab987). :warning: Report is 2509 commits behind head on main.

Files with missing lines Patch % Lines
...z/configs/default/plugins/data_tools/data_tools.py 50.00% 16 Missing :warning:
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1376      +/-   ##
==========================================
+ Coverage   84.90%   85.31%   +0.40%     
==========================================
  Files          91       90       -1     
  Lines        8347     8183     -164     
==========================================
- Hits         7087     6981     -106     
+ Misses       1260     1202      -58     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:
  • :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

codecov[bot] avatar Jun 07 '22 15:06 codecov[bot]

The Solara repo is currently private (but MIT licensed). We do this so we know which projects are using Solara, such that when we break things, we can fix them. If people from jdaviz want access, just message a ✋ in this PR and I'll add you to the repo.

maartenbreddels avatar Jun 07 '22 15:06 maartenbreddels

"Open directory" seems dangerous. If Imviz loads all 100s of images when someone accidentally selected a directory with 100s of images and click that, bad things happen. (Unless "open directory" means navigate into the directory in file browser?)

pllim avatar Jun 07 '22 15:06 pllim

That was already a feature right? I didn't introduce anything new did I?

(from mobile phone)

Op di 7 jun. 2022 17:28 schreef P. L. Lim @.***>:

"Open directory" seems dangerous. If Imviz loads all 100s of images when someone accidentally selected a directory with 100s of images and click that, bad things happen.

— Reply to this email directly, view it on GitHub https://github.com/spacetelescope/jdaviz/pull/1376#issuecomment-1148827032, or unsubscribe https://github.com/notifications/unsubscribe-auth/AANPEPJNDKMX4BB22CY5TGLVN5TDFANCNFSM5YDE4KKQ . You are receiving this because you authored the thread.Message ID: @.***>

maartenbreddels avatar Jun 07 '22 15:06 maartenbreddels

That was already a feature right? I didn't introduce anything new did I?

I think it is just the wordings that confused me. I thought it was going to open all the files in that directory but now I think I understand it is going to traverse into that directory without loading any files...

pllim avatar Jun 07 '22 15:06 pllim

I think it is just the wordings that confused me. I thought it was going to open all the files in that directory but now I think I understand it is going to traverse into that directory without loading any files...

But for mosviz we want to be able to import an entire directory, right? It looks like we can just skip this button for non-mosviz viewers if we want (although I can see some useful cases where it might be nice to have... maybe we should just check the number of files before doing the import?).

kecnry avatar Jun 07 '22 16:06 kecnry

It's probably out of scope for this PR but I think it'd be useful to mark certain package dependencies as plugin-specific, and make them optional installs. For example, we disable the Import File plugin for use in MAST. It would also be nice to then not have the extra solara dependency installed.

havok2063 avatar Jun 07 '22 16:06 havok2063

@havok2063 , is there such a thing as optional plugins? In the __init__.py import hierarchy, there are a lot of star imports. I think at the moment you import a viz, everything gets imported whether they are used or not. And all the viz is imported into root-level __init__.py (also a headache for #881).

pllim avatar Jun 07 '22 16:06 pllim

But for mosviz we want to be able to import an entire directory, right?

I still disagree with that idea. I think it is more explicit to have Mosviz take a single YAML file that defines the list of inputs (not dissimilar to @list in IRAF), rather than relies on some well-behaved directory structure. But that is a discussion for another day.

pllim avatar Jun 07 '22 16:06 pllim

@pllim no I don't believe there are any optional plugins, but one could imagine a future state where there are. But indeed that's a big ask and would be a high level of effort.

I'm in favor of the Mosviz directory loader for at least for three cases. The data processing output on the isilon for MAST, the Portal download bundles, and the output of a local JWST pipeline run, all have specific directory formats that would be useful for Mosviz to load from. If you do make a YAML file input, it would also be nice to just be able to pass in a dict arrangement of that file. For MAST, we don't want to create temporary YAML files on disk just to load data.

havok2063 avatar Jun 08 '22 13:06 havok2063

Let's move the dependencies discussion to https://github.com/spacetelescope/jdaviz/issues/1399 .

pllim avatar Jun 14 '22 16:06 pllim

I think with #3564 and plans to deprecate the former import button in favor of the new infrastructure there, that this probably isn't worth rebasing just to have temporarily.

kecnry avatar Jun 17 '25 17:06 kecnry