Multiple file upload button implementation #380

[xpire]

resolves #380

I'm a big fan of this project, so I decided to pick one of the tickets up to begin contributing to!

What

In order to complete this ticket, there needs to be some changes to how we currently process uploaded files.

Upload Path

The "Open Files" button runs a function which takes the uploaded file(s) from the input element and sends them to the BE as SerializedSubtitleFile.

Complete Button Path

The "OK" button takes the selected subtitles, figures out which from ConfirmedVideoDataSubtitleTrack[] are selected, sends them to BE, and the Server will then serialise each one into a SerializedSubtitleFile.

In both cases, we eventually reach a SerializedSubtitleFile.

The ask

The proposal I have to support #380 is to:

• refactor out ConfirmedVideoDataSubtitleTrack.
• modify handleFileInputChange to not send the openFile message to bridge/server, but instead update subtitles[]. That way, users can then select in the dropdown the newly uploaded subtitle files.
• figure out a way to store the information of uploaded (SerializedSubtitleFile) and video-parsed (VideoDataSubtitleTrack) subtitles in the VideoDataUiBridgeConfirmMessage. Current contenders are typescript union array, or two separate arrays of tuples which maintain order.
• process the data, keeping the SerializedSubtitleFile as-is, but doing the same processing as before for VideoDataSubtitleTrack
• remove the openFile message/command to server, as now uploads are handled by confirm.

The user flow will look like this instead:

1. user clicks "LOAD SUBTITLES"
2. user clicks "OPEN FILES" in dialogue
3. user selects file(s)
4. user returns to dialogue, and then can use the dropdown to select which parsed subtitle/subtitle file to use for 1st/2nd/3rd.
5. user clicks OK and this data is sent to server for processing

Notes:

• I refactor out ConfirmedVideoDataSubtitleTrack, as it seems to be a duplicated class providing little utility, so I've merged it into one interface.
• I recognise that the new flow will actually make it slightly slower for people accustomed to just uploading files and expecting it to automatically start. I'm happy to recieve feedback on how to improve this aspect if this is a must.
• This is also not following spec directly, but I think this will solve the original issue, in a slightly different way. Multiple buttons can be done if we think this is better.

[killergerbah]

Hi thanks for showing interest in contributing. Overall your plan sounds great and well thought-out. Thanks for taking all the time to read through the code. Here's my feedback:

figure out a way to store the information of uploaded (SerializedSubtitleFile) and video-parsed (VideoDataSubtitleTrack) subtitles in the VideoDataUiBridgeConfirmMessage. Current contenders are typescript union array, or two separate arrays of tuples which maintain order.

I think it is fine to use a union array as long as it's easy to tell which type each element is in code. But that would not be my first instinct since in any case you will need some if else logic to do this type of switching between types anyway. For example, you could make url optional, a serializedFile optional field to VideoDataSubtitleTrack and assume that exactly one of url or serializedFile are populated.

user returns to dialogue, and then can use the dropdown to select which parsed subtitle/subtitle file to use for 1st/2nd/3rd.

For this to make sense I think it would have to be completely obvious that the file is now available in the subtitle selector. Maybe even auto-select it in the first available drop-down. In any case I would hope that the multiple-files-in-different-tracks use-case could be solved by your solution efficiently, since that is the problem posed by the original issue.

[xpire]

Thanks for the quick feedback :D I really appreciate your help so early in the PR

I think it is fine to use a union array as long as it's easy to tell which type each element is in code. But that would not be my first instinct since in any case you will need some if else logic to do this type of switching between types anyway. For example, you could make url optional, a serializedFile optional field to VideoDataSubtitleTrack and assume that exactly one of url or serializedFile are populated.

Yes, I agree, I was actually having trouble with type guarding with type script.

I like the idea of using an optional field, (just some of the other fields would need to be optional like language, possibly extension), which made me initially hesitant with this approach. Will work with this approach and see how I go.

For this to make sense I think it would have to be completely obvious that the file is now available in the subtitle selector. Maybe even auto-select it in the first available drop-down.

Yes, good call out, auto-fill out sounds good to me too.

In any case I would hope that the multiple-files-in-different-tracks use-case could be solved by your solution efficiently, since that is the problem posed by the original issue.

I think we get this for free, but I would also like the upload button to be used multiple times in case a user is unfamiliar with selecting multiple files, which emulates the functionality of having a separate upload button for each of the 3 tracks. 🙏 I will keep the original problem in mind and ensure the new flow does solve the original problem.

[artjomsR]

Hi, a couple of thoughts from a fan of the project as well :)

1. Not sure if it was considered already but it'd be good to only display the after loading pop up if there's 2+ subtitle files to load. If there's only 1, it would make sense to simply load it into the 1st track and skip the popup
2. Like you said, the new popup would make it slower. I'm thinking it would be possible to detect the language from the subtitle filename (not sure there's any other methods we could use?) and add a new setting for preferred languages per each subtitle track

• so if all languages are successfully loaded from all the subtitle files and they match those in the user's setting, then we can assign them accordingly and skip the popup
• we could try to extract the language from the file name as the code between 2 last periods, eg en in breaking-badS1E01.en.srt
• Example of the new setting could be eg Preferred subtitle language for track 1: jp, jap & Preferred subtitle language for track 2: en, eng
• It's important to support locales here, eg en-US etc
• If any of the languages are not detected, it's probably best to show the popup anyway, but prepopulate the subtitles that could be matched?

I realise no. 2 adds quite a bit of complexity to this so it can be easily added as a follow up pr later if you deem it beneficial enough. Not wanting to add extra burden but just thinking of the ideal scenario :)

[xpire]

Not sure if it was considered already but it'd be good to only display the after loading pop up if there's 2+ subtitle files to load. If there's only 1, it would make sense to simply load it into the 1st track and skip the popup

There might be users who are only uploading 1 track but intending to use an embedded track for the secondary one. I guess this case makes sense if there are no embedded subtitles and they are uploading the files though, good call out 👍

Like you said, the new popup would make it slower. I'm thinking it would be possible to detect the language from the subtitle filename (not sure there's any other methods we could use?) and add a new setting for preferred languages per each subtitle track

This is actually a pain point for me right now. After the OK button is clicked, the language field in VideoDataSubtitleTrack is used to save the last used subtitle languages. The settings here are evaluated before the subtitle file is parsed currently, so my current thought was to parse the file in client and server, and the client use something like chrome.i18n.detectLanguage() to figure out locale (gets very complicated). If detecting languages based on the filename is fine, I might just stick to that instead.

For example, you could make url optional, a serializedFile optional field to VideoDataSubtitleTrack and assume that exactly one of url or serializedFile are populated.

I've been spiking this, and it actually gets quite messy, and we still eventually have to do if else statements. Displaying the options in the dropdown section assume url exists and is unique, so that logic also complicates things as well. I might reconsider the options I listed earlier (two arrays of tuple (obj + index), or heterogeneous array)

[artjomsR]

There might be users who are only uploading 1 track but intending to use an embedded track for the secondary one

Yep that's a good call. So maybe it's better to always display the popup, but we could also add a upload subtitle button for each individual track (on the initial asb player load). So, eg, if you click that button under the 2nd track, then it'd load a single subtitle file and apply it to the 2nd track. No popup would be shown in this case

This once again complicates it, so probably worth doing in a separate pr as well :)

[killergerbah]

Regarding auto-detected languages: My suggestion would be to not attempt to detect languages or modify language preference at all in the case of uploaded files. The language preference really only makes sense for auto-selecting one of the auto-detected subtitle tracks on the website. So I think user-provided files should not be used for this, only auto-detected subtitle tracks.

[artjomsR]

Not sure if this is helpful, given the PR is already on course, but had another thought. We could mimic what MPV player does: it has a shortcut for selecting a subtitle for a given track. So, assuming that I've got .jp.srt and .en.srt files loaded, the shortcut would cycle between displaying Japanese, English or no subtitles for that track. So by using these shortcuts for both subtitle tracks, the user is able to effectively swap the diplayed subtitles positions

[xpire]

Not sure if this is helpful, given the PR is already on course, but had another thought. We could mimic what MPV player does: it has a shortcut for selecting a subtitle for a given track. So, assuming that I've got .jp.srt and .en.srt files loaded, the shortcut would cycle between displaying Japanese, English or no subtitles for that track. So by using these shortcuts for both subtitle tracks, the user is able to effectively swap the diplayed subtitles positions

Interesting, does MPV support multiple subtitle tracks at the same time? If there is interest in this feature, should probably make a new ticket, as it feels distinct from this work, beginner users like me won't be able to get used to shortcuts in-place of the UI.

[artjomsR]

Yes, mpv has 2 subtitle tracks https://mpv.io/manual/master/#options-secondary-sid

mpv.net overcomes this by providing UI to select a subtitle for the track, so we could do the same for ASB. But, like you say, job for another PR