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

Localization inconsistency fix for Local API #2535

Conversation

MarmadileManteater
Copy link
Contributor

@MarmadileManteater MarmadileManteater commented Sep 3, 2022


Localization inconsistency fix for Local API

Pull Request Type
Please select what type of pull request this is:

  • Bugfix
  • Feature Implementation
  • Documentation
  • Other

Related issue
closes #2530

Description
This applies a workaround for the inconsistency in the responses from ytdl and ytpl. The result of this change is that the video title and description are localized to en-US by ytdl (as it was expected to be based on the following code snippet).

// line 353 of src\renderer\store\modules\ytdl.js
ytdl.getInfo(videoId, {
        lang: 'en-US',// this locale was not being honored before this change
        requestOptions: { agent }
      }).then((result) => {
        resolve(result)
      }).catch((err) => {
        reject(err)
      })

Screenshots (if appropriate)
Before this change:
image
image
image

After this change:
image
image
image

Desktop (please complete the following information):

  • OS: Windows 10
  • OS Version: Pro Version 21H2 Installed on ‎4/‎3/‎2022 OS build 19044.1889 Experience Windows Feature Experience Pack 120.2212.4180.0
  • FreeTube version: 0.17.1

Additional context
The inconsistency described in #2530 still occurs when using the Invidious API; however, as far as I can tell, this is an issue with invidious. Even if we aren't sending a locale, it should give back titles and descriptions that reflect a consistent locale, but I don't believe there is a way to fix that inconsistency without digging into the invidious source, and at that point, it isn't an issue with FreeTube.

@PrestonN PrestonN enabled auto-merge (squash) September 3, 2022 18:07
@efb4f5ff-1298-471a-8973-3d47447115dc efb4f5ff-1298-471a-8973-3d47447115dc added the PR: waiting for review For PRs that are complete, tested, and ready for review label Sep 3, 2022
src/renderer/views/Watch/Watch.js Outdated Show resolved Hide resolved
src/renderer/views/Watch/Watch.js Outdated Show resolved Hide resolved
src/renderer/views/Watch/Watch.js Outdated Show resolved Hide resolved
@absidue absidue added PR: changes requested and removed PR: waiting for review For PRs that are complete, tested, and ready for review labels Sep 3, 2022
Co-authored-by: absidue <48293849+absidue@users.noreply.github.com>
auto-merge was automatically disabled September 3, 2022 19:27

Head branch was pushed to by a user without write access

@PrestonN PrestonN enabled auto-merge (squash) September 3, 2022 19:28
auto-merge was automatically disabled September 3, 2022 19:29

Head branch was pushed to by a user without write access

Co-authored-by: absidue <48293849+absidue@users.noreply.github.com>
@PrestonN PrestonN enabled auto-merge (squash) September 3, 2022 19:30
Co-authored-by: absidue <48293849+absidue@users.noreply.github.com>
auto-merge was automatically disabled September 3, 2022 19:30

Head branch was pushed to by a user without write access

@PrestonN PrestonN enabled auto-merge (squash) September 3, 2022 19:30
Copy link
Contributor Author

@MarmadileManteater MarmadileManteater left a comment

Choose a reason for hiding this comment

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

I just approved the change to using .join for the description, but I walked away from my desk, and the more I thought about it, the more I realized it won't work because the array contains objects with a field named 'text', not strings.
image

auto-merge was automatically disabled September 3, 2022 19:46

Head branch was pushed to by a user without write access

Co-authored-by: absidue <48293849+absidue@users.noreply.github.com>
@PrestonN PrestonN enabled auto-merge (squash) September 3, 2022 19:47
@absidue absidue added PR: waiting for review For PRs that are complete, tested, and ready for review and removed PR: changes requested labels Sep 3, 2022
@MarmadileManteater
Copy link
Contributor Author

I feel like these were all very good changes. I see the merging is blocked because 1 review is requesting changes, but I believe that I reviewed all of the requested changes. I can make more changes if it is necessary. I am just checking in on where we left off on this.

Copy link
Member

@absidue absidue left a comment

Choose a reason for hiding this comment

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

Hi @MarmadileManteater

Sorry for the delay, at the time I had finished the code review but hadn't tested the code yet.

While testing I noticed that it spits out the fallback error if the video is missing a description (second video in the playlist in your issue), one easy way to fix that would be to check if there is a non translated description (empty string) before doing the whole try catch stuff. Ideally we only want the error being logged if it legitimately failed to extract the translated description (not being able to do it when there is no description is expected behaviour).

@absidue absidue self-requested a review September 7, 2022 21:39
This should prevent errors from erroneously being thrown on videos
which have a blank description. If the description is undefined or does
not contain a 'runs' field, the resulting descriptionLines should be
and empty array. Then, videoDescription will be an empty string.
auto-merge was automatically disabled September 7, 2022 23:30

Head branch was pushed to by a user without write access

@MarmadileManteater
Copy link
Contributor Author

Hi @absidue,

You are exactly correct.
image
Looking at the error, I believe it might just be easy enough to use optional chaining on the description property. Then, descriptionLines would be assigned the value of [], and this would make videoDescription a blank string.

// Adding a ? to line 310 of src\renderer\views\Watch\Watch.js
const descriptionLines = result.response.contents.twoColumnWatchNextResults.results.results.contents[1].videoSecondaryInfoRenderer.description?.runs
this.videoDescription = descriptionLines?.map(line => line.text).join('\n') ?? ''

@PrestonN PrestonN enabled auto-merge (squash) September 7, 2022 23:31
absidue
absidue previously approved these changes Sep 8, 2022
Copy link
Member

@absidue absidue left a comment

Choose a reason for hiding this comment

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

Looks good to me now. Thanks!

Copy link
Collaborator

@PikachuEXE PikachuEXE left a comment

Choose a reason for hiding this comment

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

Code looks fine but how do I test the behaviour locally?

@MarmadileManteater
Copy link
Contributor Author

MarmadileManteater commented Sep 8, 2022

These are some examples of videos I have collected of where this code changes the behavior:

It would also be worth testing some videos which have a default locale of en-US because nothing should change for those videos . . . wait

I just noticed a problem. There are extra new lines that shouldn't be there.
image

It looks like there is already a \n character inside of each line that needs it, so we just need to join by "" instead.

auto-merge was automatically disabled September 8, 2022 12:29

Head branch was pushed to by a user without write access

@PrestonN PrestonN enabled auto-merge (squash) September 8, 2022 12:29
Copy link
Member

@ChunkyProgrammer ChunkyProgrammer left a comment

Choose a reason for hiding this comment

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

Hopefully we won't have to hardcode en-us in the future and will be able to respect user's locale preferences

@PrestonN PrestonN merged commit 7747075 into FreeTubeApp:development Sep 9, 2022
@github-actions github-actions bot removed the PR: waiting for review For PRs that are complete, tested, and ready for review label Sep 9, 2022
PikachuEXE added a commit to PikachuEXE/FreeTube that referenced this pull request Sep 11, 2022
* development:
  Translated using Weblate (Spanish)
  Don't log any errors if there are no SponsorBlock segments (FreeTubeApp#2555)
  Add Scoop to readme, issue template and label workflow (FreeTubeApp#2553)
  Build and release 7zip artifacts (FreeTubeApp#2558)
  Increase width of quality selector so that 1080p60 doesn't overflow into the full screen button (FreeTubeApp#2556)
  Translated using Weblate (Russian)
  Localization inconsistency fix for Local API (FreeTubeApp#2535)
  Translated using Weblate (Italian)
@MarmadileManteater MarmadileManteater deleted the localization-inconsistency-fix-local-api branch October 6, 2022 14:52
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.

[Bug]: Title inconsistency between watch and ft-list-playlist
6 participants