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

Fix VTT issues #341

Merged
merged 5 commits into from
Jul 12, 2024
Merged

Fix VTT issues #341

merged 5 commits into from
Jul 12, 2024

Conversation

dsavinov-actionengine
Copy link
Contributor

Problem and/or solution

VTT fixes for issues found during the 1st QA pass

How to test

Reviewer checklist

Code:

  • Change is covered by unit-tests
  • Code is well documented, well styled and is following best practices
  • Performance issues have been taken under consideration
  • Errors and other edge-cases are handled properly

PR:

  • Problem and/or solution are well-explained
  • Commits have been squashed so that each one has a clear purpose
  • Commits have a proper commit message according to TEM

@@ -45,7 +45,9 @@ def parse(self, content, **kwargs):
self.transcriber.copy_until(len(source))

template = self.transcriber.get_destination()
if not template.startswith("WEBVTT"):
if len(stringset) == 0:
raise ParseError("There are no strings to translate")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Implemented issue 6 from QA spreadsheet: system should fail if source content consists of only WEBVTT signature

@@ -101,10 +103,6 @@ def _parse_section(self, offset, section):

# Content
string_to_translate = "\n".join(src_strings[timings_index + 1 :])
if string_to_translate == "":
raise ParseError(
f"Subtitle is empty on line {self.transcriber.line_number + 2}"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Implemented issue 3 from QA spreadsheet: system should allow uploading (parsing) even if some subtitles are empty

@dsavinov-actionengine
Copy link
Contributor Author

@kbairak
Important note below for issue 1 ("when the stringset sent to the compiler has no strings, the source strings should be added instead to that stringset.")

In short: At openformats level, we don't have info during compile() about the source strings. The fix might be appliet in transifex code.
Details:
When Transifex calls parse method, the parse(content) makes a template (where hashes replace strings) and a stringset from the content.
Then, when Transifex calls our compile(template, stringset) method, only the new translated stringset is passed to us, but we don't get the original stringset or at least the original content in the compile() method. It seems only Transifex has access to everything at once: source content, source stringset, template, translated stringset.

Please let us know your thoughts on this.

@dsavinov-actionengine
Copy link
Contributor Author

@kbairak added fixes for AE-45 + checked for scenarios you mentioned:

  1. during parsing, if the string that's about to be extracted is empty, don't include it in the resulting stringset at all
  2. during compiling, if the stringset has an empty string, the resulting file should have this string's section but with the string content empty
  3. during compiling, if a string is missing from the stringset, the resulting file should not have the string's section at all (the timestampts and anything else should be removed)

Testable were 1 and 2. 3 is only testable outside openformats testbed

@kbairak
Copy link
Member

kbairak commented Jul 10, 2024

Hey @dsavinov-actionengine, thanks for the update. I will start checking.

This

3 is only testable outside openformats testbed

however, is not true. You can do/write a test like this:

handler = VttHandler()
template, stringset = handler.parse(...)  # A source text with 3 strings
compiled = handler.compile(template, stringset[:2])  # skip the 3rd string
assertEqual(compiled, ...)

@kbairak kbairak merged commit 3c12d4d into transifex:devel Jul 12, 2024
3 checks passed
@txsentinel txsentinel mentioned this pull request Jul 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants