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

Enable inline view of assets #1534

Merged
merged 12 commits into from
Mar 30, 2023
Merged

Enable inline view of assets #1534

merged 12 commits into from
Mar 30, 2023

Conversation

waxlamp
Copy link
Member

@waxlamp waxlamp commented Mar 14, 2023

Closes #1389
Closes #1057

This is a simple premise: add the generation of asset download URLs with a content-disposition of inline, and use those to drive an in-browser view of assets in the file browser.

This works for many file types, including text files and video files, but specifically not for .mkv files. This PR contains a hack to generate an HTML response with a <video> tag that "shims" the lack of support for those files.

@satra
Copy link
Member

satra commented Mar 14, 2023

@waxlamp - it would be nice if we could view any text/png/jpeg files as well. if i click on the eye right now for a txt file it tries to download it instead of providing a view. perhaps this is browser dependent? i'm on a chromium-based browser

@waxlamp waxlamp marked this pull request as ready for review March 14, 2023 20:09
@waxlamp
Copy link
Member Author

waxlamp commented Mar 14, 2023

@waxlamp - it would be nice if we could view any text/png/jpeg files as well. if i click on the eye right now for a txt file it tries to download it instead of providing a view. perhaps this is browser dependent? i'm on a chromium-based browser

Hm. Did you try this through the staging deploy preview? When I tried with a .txt file locally, it worked as expected. If you try to open a text file in your browser directly, does it display it or offer to download it?

@mvandenburgh
Copy link
Member

Note that the deploy previews don't include server changes. So if that's what's being used for testing, that would explain the inconsistencies.

@waxlamp
Copy link
Member Author

waxlamp commented Mar 14, 2023

Note that the deploy previews don't include server changes. So if that's what's being used for testing, that would explain the inconsistencies.

Thanks @mvandenburgh, this would explain why it's not working--the query parameter specifying the content disposition would simply be ignored, resulting in the same behavior for 👁️ as ⬇️.

Specifically:
- `controls` shows the playback controls
- `autoplay` begins the video playing as soon as possible
- `muted` means the video won't blare audio when it begins autoplaying
  (this is actually required to make autoplay work on certain browsers,
  but it's a nice thing to do for the user anyway)
@waxlamp
Copy link
Member Author

waxlamp commented Mar 14, 2023

So, in addition to not working via the deploy preview, @mvandenburgh also helped me figure out a different problem: this feature worked when using the create_dev_dandiset management script, but not when uploading files using dandi-cli. This turned out to be a problem of filenames and content-types and a few other things; with his help, I got this working in a more organic and (hopefully) robust way.

It will still not work via deploy previews. To test locally, check out this branch and build the software, then create a new Dandiset. Run dandi download http://localhost:8085/dandiset/000009 (or whatever your Dandiset ID is) to get a copy of the empty dandiset locally. Unpack files.tar.gz somewhere, and then copy the contents (consisting of files of several types, including an MKV) into your dandiset folder. Finally, run DANDI_DEVEL=1 dandi upload -i dandi-api-local-docker-tests --validation ignore --allow-any-path (this will ask you for your API key; you can alternatively start with export DANDI_API_KEY=<...> before running the upload command).

Now you can visit your dandiset in the browser and experiment with how it behaves under the download and "eye" icons.

UPDATE (2023-03-15 10:24): I forgot to mention that there are two text files in that bundle, bar.txt and an identical copy named bar. The new feature will work for the former but not the latter. If you look at the asset metadata for each, you'll see that bar.txt has an explicit encoding of text/plain, while bar has application/octet-stream. From my limited understanding, both should wind up with text/plain--perhaps this points to a problem with metadata extraction?

dandiapi/api/views/serializers.py Outdated Show resolved Hide resolved
dandiapi/api/storage.py Show resolved Hide resolved
dandiapi/api/views/asset.py Outdated Show resolved Hide resolved
Comment on lines 26 to 34
CONTENT_DISPOSITION_PARAM = openapi.Parameter(
'content_disposition',
openapi.IN_QUERY,
'Content Disposition',
type=openapi.TYPE_STRING,
required=False,
pattern='inline|attachment',
)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be removed if the suggestion about manual_parameters is applied

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See 5eba818.

Comment on lines 204 to 211
content_disposition = serializers.CharField(default='attachment')

def validate_content_disposition(self, value):
if value not in ['attachment', 'inline']:
raise ValidationError(
f'Illegal value {value} for parameter "content_disposition"', code=400
)
return value
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I overlooked this in my previous review comment - instead of using a CharField with a validator, we can just use a ChoiceField here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch -- see bb5f924.