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

Add upload subtitle button #1229

Merged
merged 12 commits into from
Jun 10, 2024
Merged

Add upload subtitle button #1229

merged 12 commits into from
Jun 10, 2024

Conversation

Arnei
Copy link
Member

@Arnei Arnei commented Dec 12, 2023

Resolves #999.

Adds an "Upload" button next to the "Download" button in the subtitle edit view. The button opens a file dialog where you can select a vtt file. The contents of the selected file will then replace the current subtitle cues. Tags (optional metadata from Opencast that specifiy e.g. the language of the subtitle) remain unaffected by uploading.

Courtesy of the Opencast 2023 Crowdfunding.

Adds an "Upload" button next to the "Download" button in the
subtitle edit view. The button opens a file dialog where you can select
a vtt file. The contents of the selected file will then replace the current
subtitle cues. Tags (optional metadata from Opencast that specifiy e.g. the language of the subtitle) remain unaffected by uploading.
@Arnei Arnei added the type:enhancement New feature or request label Dec 12, 2023
Copy link

This pull request is deployed at test.editor.opencast.org/1229/2023-12-12_09-06-22/ .
It might take a few minutes for it to become available.

Copy link

This pull request has conflicts ☹
Please resolve those so we can review the pull request.
Thanks.

@github-actions github-actions bot added the status:conflicts Conflicts with another pull request or issue label Jan 18, 2024
@github-actions github-actions bot removed the status:conflicts Conflicts with another pull request or issue label Jan 18, 2024
Copy link

This pull request is deployed at test.editor.opencast.org/1229/2024-01-18_15-23-06/ .
It might take a few minutes for it to become available.

@lkiesow
Copy link
Member

lkiesow commented Feb 19, 2024

Testing this, I found a few issues:

Opening the upload dialog doesn't seem to work reliably

(Tested in Firefox)

  • Click upload
    • A warning dialog will show up
  • Wait for 30s
  • Click continue
  • Nothing happens

Uploaded files do not always replace subtitles

Create a new, empty WebVTT file:

touch 123.vtt

Uploading this file will not produce an error, but also not replace the existing subtitles.

Arnei added 2 commits April 8, 2024 09:15
An empty file is not valid WebVTT, but our
parser will not recognize this as an error.
So we just check for the empty string ourselves.
Copy link

github-actions bot commented Apr 8, 2024

This pull request is deployed at test.editor.opencast.org/1229/2024-04-08_08-56-27/ .
It might take a few minutes for it to become available.

@Arnei
Copy link
Member Author

Arnei commented Apr 8, 2024

Opening the upload dialog doesn't seem to work reliably [...]

So this is annoying. It seems that using window.confirm() to open the browser default modal will also cause the browser to think that there was no user interaction if the user takes too long, preventing the file dialog from opening. Does not seem to be a catchable error, so the user is not notified.

File chooser dialog can only be shown with a user activation.

I assume the best way to solve this would be to build our own modal (we currently don't have a modal class in the editor) instead of using window.confirm(). However I would really like to avoid building a modal class for this PR because it seems A. overkill and B. ideally we would finally get Tobiras modal into appkit and then just use those.

Copy link
Member

@KatrinIhler KatrinIhler left a comment

Choose a reason for hiding this comment

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

Just requesting changes to accurately represent this state in the PR overview.

@lkiesow
Copy link
Member

lkiesow commented Apr 26, 2024

Tested this again, and it seems like even with the new patch, I still got neither an error nor will the content be overwritten if I upload an empty subtitle file.

Empty string was getting filtered out
due to being falsy. Fixes that.
@Arnei
Copy link
Member Author

Arnei commented May 16, 2024

Tested this again, and it seems like even with the new patch, I still got neither an error nor will the content be overwritten if I upload an empty subtitle file.

Should now result in an error.

Copy link

This pull request is deployed at test.editor.opencast.org/1229/2024-05-16_12-50-49/ .
It might take a few minutes for it to become available.

@Arnei Arnei self-assigned this May 16, 2024
@snoesberger
Copy link
Contributor

Tested this again, and it seems like even with the new patch, I still got neither an error nor will the content be overwritten if I upload an empty subtitle file.

Should now result in an error.

I can confirm this. I now get an error when I try to upload an empty subtitle file.

Personally, I would prefer a different placement of the error message. The upload button moves to the left when an error occurs, I would prefer the button position to remain fixed. Perhaps the error messages can be displayed below the buttons (other ideas are welcome)?

Arnei added 3 commits May 24, 2024 13:57
Fixes an issue with the upload subtitle button,
where the automatic browser dialog used by
the button would "time out", failing to open
up the subsequent file dialog. This replaces
it with a confirmation modal from appkit.

WARNING: Requires opencast/appkit#5
to be merged and released!
Instead of trying to awkwardly fit an error
box into our layout, instead inform the user
via a modal if anything failed during the
subtitle upload.
@Arnei
Copy link
Member Author

Arnei commented May 30, 2024

Demo of how this will look like with the new modal from appkit:
Bildschirmaufzeichnung vom 2024-05-30, 13-33-32.webm
Cannot make the changes here so can you can easily test, as the depend on a new appkit release, and we only want to do a new release if you people are generally happy with how the modal looks and feels.

This fixes the issue with dialog timing out, and also handles displaying the error differently? @snoesberger What do you think?

@snoesberger
Copy link
Contributor

This fixes the issue with dialog timing out, and also handles displaying the error differently? @snoesberger What do you think?

Looks good to me and is also much better for the error messages. Is there any way to add some sort of "danger" sign (like ⚠️) to the error message?

@Arnei
Copy link
Member Author

Arnei commented May 30, 2024

Easily only in the modal "body". To get it into the title would require some refactoring (possible, but more work).
Bildschirmfoto vom 2024-05-30 15-00-00

@snoesberger
Copy link
Contributor

Easily only in the modal "body". To get it into the title would require some refactoring (possible, but more work).

Thanks for the research, let's use the modal without the warning-sign for now and add it to the "nice to have" list for later.

This has the modals we need for the subtitle upload
@github-actions github-actions bot added the status:conflicts Conflicts with another pull request or issue label Jun 6, 2024
Copy link

github-actions bot commented Jun 6, 2024

This pull request has conflicts ☹
Please resolve those so we can review the pull request.
Thanks.

Copy link

github-actions bot commented Jun 6, 2024

This pull request is deployed at test.editor.opencast.org/1229/2024-06-06_06-27-23/ .
It might take a few minutes for it to become available.

@github-actions github-actions bot removed the status:conflicts Conflicts with another pull request or issue label Jun 6, 2024
Copy link

github-actions bot commented Jun 6, 2024

This pull request is deployed at test.editor.opencast.org/1229/2024-06-06_06-33-11/ .
It might take a few minutes for it to become available.

@snoesberger
Copy link
Contributor

Thanks for including the modal, looks much better now. I tested it and it works as expected.

@oas777
Copy link

oas777 commented Jun 7, 2024

Again, late to the game, but allow me a question: Why does the button come so late in the regular workflow? If I do it correctly, I first have to

  • press + for adding subtitles,
  • choose a language,
  • press "Create"

and then I can finally update a file which has no relation to the clicks I just made.

Wouldn't it be easier to make this an "earlier" option?

grafik

@Arnei
Copy link
Member Author

Arnei commented Jun 7, 2024

Three reasons:

  • We want uploaded subtitles to be associated with a language. We can't get the language from the VTT file, so having the Upload button after selecting a language ensures that we get that association. Even if we add a button like you suggested with your image, that button would still need a language selector.
  • Users should also be able to override existing subtitles through uploading a file. In that case the subtitle to be overridden needs to be selected anyway.
  • Grouping the download and upload button together just kind of felt right. Admittedly not the best reason.

That being said, I'm not necessarily against adding a button like your image suggests. Although I would keep the current Upload button too.

@oas777
Copy link

oas777 commented Jun 7, 2024

Thanks, Arne.

  • I didn't consider/know you need to choose the language of the VTT you want to upload. With that, it's only one additional click, so no need for my additional "earlier" button. The language selector should probably offer more languages than the two I see - can this be customised?
  • If you want to add a VTT to a video without any subtitles, the help text for the + should probably say "Opens a dialog for creating/uploading new subtitles".
  • Replacing existing subtitles requires you to choose that subtitle and then upload the VTT, correct?

@Arnei
Copy link
Member Author

Arnei commented Jun 10, 2024

The language selector should probably offer more languages than the two I see - can this be customised?

It even has to be customised. This way we make sure that the admin who sets up the editor makes sure that subtitles from the editor work well with their Opencast. It also saves us as a project from having to make assumptions on which languages users might want.

If you want to add a VTT to a video without any subtitles, the help text for the + should probably say "Opens a dialog for creating/uploading new subtitles".

Good call, will do.

Replacing existing subtitles requires you to choose that subtitle and then upload the VTT, correct?

Correct.

For users wondering where they can upload their subtitle
files, this hopefully guides them to creating one if a subtitle
for their language does not yet exist.
Copy link

This pull request is deployed at test.editor.opencast.org/1229/2024-06-10_06-53-05/ .
It might take a few minutes for it to become available.

@lkiesow
Copy link
Member

lkiesow commented Jun 10, 2024

Seems like people are okay with this and the changes look fine to me. Merging…

@lkiesow lkiesow merged commit 44afe77 into opencast:main Jun 10, 2024
6 checks passed
@lkiesow lkiesow self-assigned this Jun 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upload subtitle files
5 participants