-
Notifications
You must be signed in to change notification settings - Fork 7
Issues/1072 video previews #30
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
Conversation
This uses the thumbnail as poster (if available) and correctly sets the source of the video currently only works for anonymously accessible files.
append a shortlived token at the right time (error, seeking, stalled)
Thumbnails, when available, should be shown istead of the generic MIME_TYPE_IMAGE. Content of the TEXT bundle should not be shown at all this is usually automatically extracted "text layer" of a PDF, useful for indexing, but don't want people downloading it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This pull request implements enhancements to video previews on item-view pages by updating the metadata retrieval and preview handling logic. Key changes include reducing metadata bundle types in the preview section, adding robust token handling with reactive video playback controls, and updating the file preview template and bitstream model.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/app/item-page/simple/field-components/preview-section/preview-section.component.ts | Adjusts metadata retrieval parameters for the preview component |
| src/app/item-page/simple/field-components/preview-section/file-description/file-description.component.ts | Introduces reactive streams and error handling for video previews |
| src/app/item-page/simple/field-components/preview-section/file-description/file-description.component.html | Updates video and image bindings to use the new preview and thumbnail observables |
| src/app/core/metadata/metadata-bitstream.model.ts | Changes the bitstream identifier type from number to string |
Comments suppressed due to low confidence (2)
src/app/item-page/simple/field-components/preview-section/preview-section.component.ts:25
- Removing 'TEXT' and 'THUMBNAIL' from the metadata retrieval call may impact downstream components if they expect these bitstreams. Please verify that preview and related functionality have been updated accordingly to handle only the 'ORIGINAL' bundle.
.getMetadataBitstream(this.item.handle, 'ORIGINAL')
src/app/core/metadata/metadata-bitstream.model.ts:34
- Changing the bitstream identifier from a number to a string may affect consumers that assume a numeric id. Verify that all dependent code is updated to accommodate this type change.
id: string;
01bd4a6 to
f852096
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces video preview support for mp4 files by updating metadata retrieval, modifying component behavior, and adjusting tests to reflect new identifier types.
- Updated metadata query for previews
- Integrated token handling and URL building for video playback
- Adjusted tests and model definitions for identifier type consistency
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/app/item-page/simple/field-components/preview-section/preview-section.component.ts | Removed extra metadata formats to focus on the ORIGINAL bitstream |
| src/app/item-page/simple/field-components/preview-section/preview-section.component.spec.ts | Updated metadata ID type in tests |
| src/app/item-page/simple/field-components/preview-section/file-description/file-description.component.ts | Added video preview handling with dynamic URL and token refresh logic |
| src/app/item-page/simple/field-components/preview-section/file-description/file-description.component.spec.ts | Updated service stubs and metadata ID for tests |
| src/app/item-page/simple/field-components/preview-section/file-description/file-description.component.html | Modified template to support video previews and use dynamic sources |
| src/app/item-page/clarin-files-section/clarin-files-section.component.spec.ts | Updated metadata ID to string for consistency |
| src/app/core/metadata/metadata-bitstream.model.ts | Changed the type of the id property from number to string |
Comments suppressed due to low confidence (1)
src/app/item-page/simple/field-components/preview-section/file-description/file-description.component.html:33
- [nitpick] Consider caching the result of the async pipe for 'thumbnail_url$' to avoid duplicate async pipe evaluations and improve template readability.
[src]=" ( thumbnail_url$ | async) ? (thumbnail_url$ | async) : MIME_TYPE_IMAGES_PATH + fileInput.format?.replace('/','-') + '.png'"
| } | ||
| } | ||
|
|
||
| private updateSource(video, currentTime) { |
Copilot
AI
May 13, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding explicit type annotations for the parameters (e.g., 'video: HTMLVideoElement' and 'currentTime?: number') to enhance code clarity and maintainability.
| private updateSource(video, currentTime) { | |
| private updateSource(video: HTMLVideoElement, currentTime?: number) { |
* Updated the no file preview message (#837) * Added spacing between clarin & dspace logo (#848) * Do not load Seznam license every time (#844) * Fixed encoding of the filename from the URL (#838) (#851) * Do not mount the Solr configs; copy them each time instead. (#850) * Fix the bulk access (#852) * Video files previews (ufal#30)
Adding previews of mp4 videos
tweaks what sort of bundles are shown on item-view pages