Viewers icon indicating copy to clipboard operation
Viewers copied to clipboard

Added an extension for downloading an entire study

Open Christian-e-S opened this issue 4 years ago • 9 comments

related issue: https://github.com/OHIF/Viewers/issues/2430#issue-907754649

  • User can trigger the download of a complete study, via toolbar button
  • User gets detailed status information in a modal view
    • Individual steps (downloading, zipping, saving)
    • a pull request for the dicomweb-client client has been issued. With this version the user sees also the progress of the download in terms of downloaded bytes (see https://github.com/dcmjs-org/dicomweb-client/pull/44#issue-676285054).
  • [x] Brief description of changes
  • [x] Links to any relevant issues
  • [x] Required status checks are passing
  • [x] User cases if changes impact the user's experience
  • [ ] @mention a maintainer to request a review

Christian-e-S avatar Jun 23 '21 13:06 Christian-e-S

Codecov Report

Merging #2454 (065fde3) into master (fe57c00) will decrease coverage by 0.18%. The diff coverage is 0.49%.

:exclamation: Current head 065fde3 differs from pull request most recent head e058beb. Consider uploading reports for the commit e058beb to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2454      +/-   ##
==========================================
- Coverage   12.68%   12.50%   -0.19%     
==========================================
  Files         306      307       +1     
  Lines        8233     8360     +127     
  Branches     1593     1654      +61     
==========================================
+ Hits         1044     1045       +1     
- Misses       5797     5862      +65     
- Partials     1392     1453      +61     
Impacted Files Coverage Δ
platform/core/src/classes/StudyLoadingListener.js 1.02% <0.00%> (ø)
...latform/core/src/classes/metadata/StudyMetadata.js 1.28% <0.00%> (-0.02%) :arrow_down:
...m/core/src/utils/loadAndCacheDerivedDisplaySets.js 0.00% <0.00%> (ø)
...r/src/connectedComponents/ConnectedStudyBrowser.js 0.00% <0.00%> (ø)
platform/viewer/src/connectedComponents/Viewer.js 0.00% <0.00%> (ø)
...tform/viewer/src/connectedComponents/ViewerMain.js 0.00% <0.00%> (ø)
...rm/viewer/src/googleCloud/DicomStorePickerModal.js 0.00% <ø> (ø)
platform/viewer/src/index.js 0.00% <ø> (ø)
platform/core/src/utils/naturalizeSOPClassUID.js 0.74% <0.74%> (ø)

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 c59d664...e058beb. Read the comment docs.

codecov[bot] avatar Jun 23 '21 13:06 codecov[bot]

Hey @Christian-e-S, great job here :) 🎖️

So, I just quickly tested the feature and it seems to be working fine. We have some of this logic in another extension (debugging) but I think it its currently broken, maybe we are better off accepting this separate extension and dumping the old download logic from debugging (what do you think @swederik?)

I have some comments regarding the modal though: Screen Shot 2021-07-07 at 08 42 11

  • I think toolbar button should not be enabled in master by default (@pieper @swederik do we have liability concerns?)
  • When clicking to open the modal, it downloads right away but I think we should add a 'download' button there to avoid clicking the button in the toolbar by mistake and starting a huge downloading request in the background
  • The size was not updated after downloading
  • There's no visible loading indicator, I thought it was stuck with a blank status: ...
  • You can leverage the notification service to display notifications e.g. 'Study downloaded successfully' or something

Extras

  • What about adding options in the modal to download display set (series) based or instance-based (we can tackle this in another branch)
  • Another extra case that is really not important right now (just thought about it): Adding an option to disable and enable a save button in the viewport overlay so users can download the active viewport

igoroctaviano avatar Jul 07 '21 11:07 igoroctaviano

Hey @igoroctaviano!

Thank you very much for your review!

Please find my comment below:

Hey @Christian-e-S, great job here :) 🎖️

So, I just quickly tested the feature and it seems to be working fine. We have some of this logic in another extension (debugging) but I think it its currently broken, maybe we are better off accepting this separate extension and dumping the old download logic from debugging (what do you think @swederik?)

I have some comments regarding the modal though: Screen Shot 2021-07-07 at 08 42 11

  • I think toolbar button should not be enabled in master by default (@pieper @swederik do we have liability concerns?)

I agree, this makes absolutely sense!

  • When clicking to open the modal, it downloads right away but I think we should add a 'download' button there to avoid clicking the button in the toolbar by mistake and starting a huge downloading request in the background

We had it like this, but users complained about the additional click. In some use cases, users use the online viewer just for previewing images, and then download the series to be able to use the specific features of the tool that they are used to, so downloading becomes their major use case and they want to have it efficient.

  • The size was not updated after downloading

This works only with a recent commit of the dicomweb-client https://github.com/dcmjs-org/dicomweb-client/commit/c292726a421cd84e6a34aec4e72fbbad3c87621c that was created from my pull request to the dicomweb-client repository. This is required to get progress information in terms of bytes downloaded.

Should we wait until this gets into the next release of dicomweb-client?

image

  • There's no visible loading indicator, I thought it was stuck with a blank status: ...

See comment above. With the latest master branch of the dicomweb-client you can see the transferred bytes progressing, during the download, which gives a good indication:

image

image

  • You can leverage the notification service to display notifications e.g. 'Study downloaded successfully' or something

Agreed

Extras

  • What about adding options in the modal to download display set (series) based or instance-based (we can tackle this in another branch)
  • Another extra case that is really not important right now (just thought about it): Adding an option to disable and enable a save button in the viewport overlay so users can download the active viewport

I like these ideas.

Christian-e-S avatar Jul 08 '21 09:07 Christian-e-S

@igoroctaviano what is the status on this?

sedghi avatar Dec 23 '21 17:12 sedghi

@sedghi codewise LGTM but we need to check if we want to merge it (I know that there are some implications to having an easy download button in a viewer).

igoroctaviano avatar Jan 12 '23 23:01 igoroctaviano

base has changed, read more here https://github.com/OHIF/Viewers/issues/3477

sedghi avatar Jun 19 '23 13:06 sedghi

@Christian-e-S , do you have this extension to v3? rgds and great job.

fmotta1968 avatar Aug 29 '23 12:08 fmotta1968

@Christian-e-S , do you have this extension to v3? rgds and great job.

Did you ever try this with v3? If so, how did you go about it? I don't even know how to get started with adding the extension.

threehappypenguins avatar Apr 18 '24 22:04 threehappypenguins

@threehappypenguins No we haven't done it in v3

sedghi avatar May 14 '24 16:05 sedghi