[FilePreview] Use syntax highlighting for `.srt` #35651

PesBandi · 2024-10-29T16:36:01Z

Summary of the Pull Request

Adds syntax highlighting to .srt preview (Peek and Preview Pane).

PR Checklist

Closes: [Monaco] Use syntax highlighting for .srt #35152
Communication: I've discussed this with core contributors already.
Tests: All pass
Localization: All end user facing strings can be localized
Dev docs: Updated
New binaries: None
Documentation updated: No need

Detailed Description of the Pull Request / Additional comments

Removes srt from txtExt and registers it as a new language
Changes customTokenColors to customTokenThemeRules
Adds custom Monarch definition
- Block numbers tokenized as number
- Timestamp tokenized as tag
- Subtitle content tokenized as string
- Bold, italic and underline are the same color as the rest of the subtitle, just in their respective font style
- Partially assumes that the file is valid to make the definition simpler, which means:
  - All text on the same line as a timestamp will be colored the same as the subtitle text, even though it should be ignored
  - Assumes that a block number is followed by a timestamp (doesn't break if it isn't, just incorrectly tokenizes the number as a number instead of ignoring it)
  - ~~Assumes that all format tags are closed (if not, the rest of that subtitle block is in that format)~~ Correction: that's the intended behavior
  - Only timestamps in the HH:mm:ss,mmm format are considered valid, even though something like 00:123:01,1234 --> 00:00:02,000 is technically valid and would get interpreted as 02:03:02,234 --> 1193:02:49,296 (I have no idea what's going on)
- I couldn't find any official description of the srt format, so I used ffmpeg to determine what is considered valid

Screenshot:

Validation Steps Performed

Tested previewing srt files with Peek and Preview Pane. Monarch definition tested for edge cases.

htcfreek · 2024-10-29T17:04:54Z

src/Monaco/monacoSpecialLanguages.js

-    registerAdditionalLanguage("cppExt", [".ino", ".pde"], "cpp", monaco)
-    registerAdditionalLanguage("xmlExt", [".wsdl", ".csproj", ".vcxproj", ".vbproj", ".fsproj"], "xml", monaco)
-    registerAdditionalLanguage("txtExt", [".sln", ".log", ".vsconfig", ".env", ".srt"], "txt", monaco)
-    registerAdditionalLanguage("razorExt", [".razor"], "razor", monaco)
-    registerAdditionalLanguage("vbExt", [".vbs"], "vb", monaco)
-    registerAdditionalLanguage("iniExt", [".inf", ".gitconfig", ".gitattributes", ".editorconfig"], "ini", monaco)
-    registerAdditionalLanguage("shellExt", [".ksh", ".zsh", ".bsh"], "shell", monaco)
-    registerAdditionalNewLanguage("reg", [".reg"], regDefinition(), monaco)
-    registerAdditionalNewLanguage("gitignore", [".gitignore"], gitignoreDefinition(), monaco)
+    registerAdditionalLanguage("cppExt", [".ino", ".pde"], "cpp", monaco);
+    registerAdditionalLanguage("xmlExt", [".wsdl", ".csproj", ".vcxproj", ".vbproj", ".fsproj"], "xml", monaco);
+    registerAdditionalLanguage("txtExt", [".sln", ".log", ".vsconfig", ".env"], "txt", monaco);
+    registerAdditionalLanguage("razorExt", [".razor"], "razor", monaco);
+    registerAdditionalLanguage("vbExt", [".vbs"], "vb", monaco);
+    registerAdditionalLanguage("iniExt", [".inf", ".gitconfig", ".gitattributes", ".editorconfig"], "ini", monaco);
+    registerAdditionalLanguage("shellExt", [".ksh", ".zsh", ".bsh"], "shell", monaco);
+    registerAdditionalNewLanguage("reg", [".reg"], regDefinition(), monaco);
+    registerAdditionalNewLanguage("gitignore", [".gitignore"], gitignoreDefinition(), monaco);
+    registerAdditionalNewLanguage("srt", [".srt"], srtDefinition(), monaco);


why all of the are changed

I don't know, I don't see any changes there. Maybe it's just git being weird?

Oh no, sorry, I see it now. There are semicolons added to each line. I guess I hit format and it added them without me noticing. But I guess we can keep that in, right? After all, they're supposed to be there.

htcfreek · 2024-10-29T17:07:31Z

src/Monaco/customTokenThemeRules.js

+    {token: 'string.bold', fontStyle: 'bold'},
+    {token: 'string.emphasis', fontStyle: 'italic'},
+    {token: 'string.underline', fontStyle: 'underline'}


Don't we need combined styles for the nested formats? Or is this to complicated?

Yes we would for proper nesting, however as far as I know there isn't any nice way to do that, we would need a separate state for every possible combination (bold, italic, underline, bold-italic, bold-underline, italic-underline and so on...) and not even VSCode does that for Markdown, so I think that it's unnecessary.
The tags are styled, so even if you have something like text you can see both bold and italic.

src/Monaco/customLanguages/gitignore.js

htcfreek · 2024-10-29T17:12:54Z

All text on the same line as a timestamp will be colored the same as the subtitle text, even though it should be ignored

What color?

Can we instead color it white? Then it is more clear what is used as sub title.

Maybe define the and of time stamp color correctly.
Maybe require that the subtitle text starts in a new line.

htcfreek · 2024-10-29T17:20:07Z

You might look at this:
https://docs.lokalise.com/en/articles/5365539-srt-files-and-all-you-need-to-know-about-subrip-subtitles

This page contains additional sytax that you currently not support in your PR.
And I am wondering if we really should show the text based on the containing format tags. Because applying the color format definition will be tricky. Maybe we should only format text in one color and format tags in an other color.

Edited at 30. October 2024 00:08 AM.

PesBandi · 2024-10-30T08:03:32Z

This page contains additional sytax that you currently not support in your PR.

Yes, it does contain the following syntax that I don't support:

Curly braces for tags - {b}bold{/b}
Font color with 
Line position with X1:… X2:… Y1:… Y2:…

I'm aware of this, but I have decided not to support these features for a reason:

Most sources never mention curly braces for tags. I don't think they were ever an official part of the srt format. I am guessing that this misconception comes from the fact that the SubStation Alpha (ASS) format uses curly braces, someone published one wrong article and everyone copied it. Moreover, FFMpeg, one of the most trusted video tools, doesn't support that kind of syntax and just ignores it.
There is no way I can support proper coloring, so there are three options:
- Ignore  completely
- Color/style all  tags in one way to make it clear that there is one
- Support some of the basic colors (red, green, blue...) and do option 2 for the rest of the colors
X1:… X2:… Y1:… Y2:… was never officially supported, it's a story similar to {b}

PesBandi · 2024-10-30T08:06:02Z

Can we instead color it white? Then it is more clear what is used as sub title.

I am working on this ~~turns out it's a little more complicated than I thought though, so it may take some time for me to get it working~~ Solved it

htcfreek · 2024-10-30T08:13:44Z

Can we instead color it white? Then it is more clear what is used as sub title.

I am working on this, turns out it's a little more complicated than I thought though, so it may take some time for me to get it working

You know the test page?
https://microsoft.github.io/monaco-editor/monarch.html

PesBandi · 2024-10-30T08:21:51Z

Can we instead color it white? Then it is more clear what is used as sub title.

I am working on this, turns out it's a little more complicated than I thought though, so it may take some time for me to get it working

You know the test page? https://microsoft.github.io/monaco-editor/monarch.html

Yeah, I do, that's where I did most of my testing. Thanks for making sure

PesBandi · 2024-10-30T08:23:27Z

I pushed the changes, it now looks like this

htcfreek · 2024-10-30T08:54:58Z

Wondering if we should format in the text only the tags instead of memic the format. This would help with the font tag problem.

We could use a lighter color or make it italic. Then you can see that it is a valid tag or not.

PesBandi · 2024-11-05T19:59:24Z

Hi @htcfreek, I apologize for not responding sooner, I was unexpectedly offline for some time :(
Anyhow, this thread has gotten really chaotic over time, so...
The current state is this:

What exactly should I change?
The only question from me is how do we style ?

htcfreek · 2024-11-05T20:06:06Z

Hi @htcfreek, I apologize for not responding sooner, I was unexpectedly offline for some time :(
Anyhow, this thread has gotten really chaotic over time, so...
The current state is this:

What exactly should I change?
The only question from me is how do we style ?

Hi @PesBandi .
My idea is to do a normal syntax highlighting of all format code in a lighter color or italic instead of formatting the text based on the style definition it contains. (Only in thie text part that is valid.)

So instead of making the text underlined, italic and bold we can make the following parts italic: , , ..., , 

PesBandi · 2024-11-05T20:24:07Z

So make the tags , ... italic and leave the text inside them completely unstyled?

htcfreek · 2024-11-05T22:28:38Z

So make the tags , ... italic and leave the text inside them completely unstyled?

yes. That is how I would do it.

PesBandi · 2024-11-06T20:59:54Z

So make the tags , ... italic and leave the text inside them completely unstyled?

yes. That is how I would do it.

@htcfreek, I did it that way and now it looks like this

In my opinion this isn't very noticeable. I'm not sure if I'd know it's there if I didn't know what to look for. You also have to keep in mind that the text is relatively small in Peek, way smaller than this screenshot.

htcfreek · 2024-11-06T21:20:05Z

So make the tags , ... italic and leave the text inside them completely unstyled?

yes. That is how I would do it.

@htcfreek, I did it that way and now it looks like this

In my opinion this isn't very noticeable. I'm not sure if I'd know it's there if I didn't know what to look for. You also have to keep in mind that the text is relatively small in Peek, way smaller than this screenshot.

Maybe combine with different color. Maybe declare it as type.identifier (blue-green).

PesBandi · 2024-11-12T16:55:31Z

Maybe declare it as type.identifier (blue-green).

@htcfreek, this is what that would look like

htcfreek · 2024-11-12T17:24:49Z

Maybe declare it as type.identifier (blue-green).

@htcfreek, this is what that would look like

Feels a bit to much different colors. What do you think?
Maybe switching the color between numbers and tags?

PesBandi added 2 commits October 28, 2024 21:01

[FilePreview] Use syntax highlighting for .srt

0123bda

Change customTokenColors to customTokenThemeRules

953e3ef

htcfreek reviewed Oct 29, 2024

View reviewed changes

Ignore text on the same line as a timestamp

6b49ad1

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FilePreview] Use syntax highlighting for `.srt` #35651

[FilePreview] Use syntax highlighting for `.srt` #35651

PesBandi commented Oct 29, 2024 •

edited

Loading

htcfreek Oct 29, 2024

PesBandi Oct 29, 2024

PesBandi Oct 29, 2024

htcfreek Oct 29, 2024

PesBandi Oct 29, 2024 •

edited

Loading

htcfreek commented Oct 29, 2024

htcfreek commented Oct 29, 2024 •

edited

Loading

PesBandi commented Oct 30, 2024

PesBandi commented Oct 30, 2024 •

edited

Loading

htcfreek commented Oct 30, 2024

PesBandi commented Oct 30, 2024

PesBandi commented Oct 30, 2024

htcfreek commented Oct 30, 2024

PesBandi commented Nov 5, 2024

htcfreek commented Nov 5, 2024

PesBandi commented Nov 5, 2024

htcfreek commented Nov 5, 2024

PesBandi commented Nov 6, 2024 •

edited

Loading

htcfreek commented Nov 6, 2024 •

edited

Loading

PesBandi commented Nov 12, 2024

htcfreek commented Nov 12, 2024 •

edited

Loading

[FilePreview] Use syntax highlighting for .srt #35651

Are you sure you want to change the base?

[FilePreview] Use syntax highlighting for .srt #35651

Conversation

PesBandi commented Oct 29, 2024 • edited Loading

Summary of the Pull Request

PR Checklist

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

htcfreek Oct 29, 2024

Choose a reason for hiding this comment

PesBandi Oct 29, 2024

Choose a reason for hiding this comment

PesBandi Oct 29, 2024

Choose a reason for hiding this comment

htcfreek Oct 29, 2024

Choose a reason for hiding this comment

PesBandi Oct 29, 2024 • edited Loading

Choose a reason for hiding this comment

htcfreek commented Oct 29, 2024

htcfreek commented Oct 29, 2024 • edited Loading

PesBandi commented Oct 30, 2024

PesBandi commented Oct 30, 2024 • edited Loading

htcfreek commented Oct 30, 2024

PesBandi commented Oct 30, 2024

PesBandi commented Oct 30, 2024

htcfreek commented Oct 30, 2024

PesBandi commented Nov 5, 2024

htcfreek commented Nov 5, 2024

PesBandi commented Nov 5, 2024

htcfreek commented Nov 5, 2024

PesBandi commented Nov 6, 2024 • edited Loading

htcfreek commented Nov 6, 2024 • edited Loading

PesBandi commented Nov 12, 2024

htcfreek commented Nov 12, 2024 • edited Loading

[FilePreview] Use syntax highlighting for `.srt` #35651

[FilePreview] Use syntax highlighting for `.srt` #35651

PesBandi commented Oct 29, 2024 •

edited

Loading

PesBandi Oct 29, 2024 •

edited

Loading

htcfreek commented Oct 29, 2024 •

edited

Loading

PesBandi commented Oct 30, 2024 •

edited

Loading

PesBandi commented Nov 6, 2024 •

edited

Loading

htcfreek commented Nov 6, 2024 •

edited

Loading

htcfreek commented Nov 12, 2024 •

edited

Loading