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

Improve age restriction and yt restricted content strings #4354

Merged
merged 1 commit into from
Oct 11, 2020

Conversation

Stypox
Copy link
Member

@Stypox Stypox commented Sep 29, 2020

What is it?

  • Bug fix (user facing)
  • Feature (user facing)
  • Code base improvement (dev facing)
  • Meta improvement to the project (dev facing)

Description of the changes in your PR

This PR changes the titles for "Show age restricted content" and "Enable YouTube's Restricted Mode" and adds a summary to them, since they have been confused by many users lately. In order to reduce redundancy in translations, I also changed the restricted-video-error string to include a %1$s instead of the duplicate "Show age restricted content" string. If the latter change causes prolems with translation syncing I can revert it. @wb9688 @opusforlife2 @comradekingu what do you think of these strings?

Testing apk

I also tested whether the latter change cause problems with languages different than English, where %1$s is not there yet, but it isn't creating problems.
app-debug.zip

Agreement

@Stypox Stypox added this to the 0.20.0 milestone Sep 29, 2020
@Stypox Stypox added the localisation / translation Everything that has to do with translations or Weblate label Sep 29, 2020
@comradekingu
Copy link
Contributor

@Stypox Not a fan of "<string name="show_age_restricted_content_summary>", I'll read up on what is actually "age restricted". Right now it explains where it shouldn't.

@Stypox
Copy link
Member Author

Stypox commented Sep 29, 2020

@comradekingu thank you

@opusforlife2
Copy link
Collaborator

@Stypox It doesn't make sense to have a universal age restriction setting. It should be per-service.

@Stypox
Copy link
Member Author

Stypox commented Sep 29, 2020

@opusforlife2 well, if someone just wants to protect their kid from viewing age restricted videos, he would do that with all services at once, wouldn't him? When would it be the case that the user hides only age restricted videos of some services?

@opusforlife2
Copy link
Collaborator

Because different services (companies) would have different definitions and thresholds of what constitutes as 'not suitable for children'. A person might want to enable age restriction for one service but not find any need to do it for another.

On a conceptual level, age restriction is a content filter that is implemented on the service side, not in Newpipe. Hence it should be controllable on a per-service level.

@wb9688
Copy link
Contributor

wb9688 commented Sep 30, 2020

Imho the strings @Stypox has now are fine.

@opusforlife2: If we even agree that we want that, not in this PR.

@opusforlife2
Copy link
Collaborator

Of course.

@Stypox
Copy link
Member Author

Stypox commented Sep 30, 2020

@opusforlife2 @comradekingu I don't think we have to aim for shortness. Descriptiveness is the most important thing here, and in my humble opinion all of the proposed alternatives miss some point 😅

@opusforlife2
Copy link
Collaborator

opusforlife2 commented Sep 30, 2020

The problem is not the length. The sentence is a run-on sentence. It should be broken into two sentences. Let's try that now.

"Show 18+ content. May be unsuitable for children."

@Stypox
Copy link
Member Author

Stypox commented Oct 2, 2020

What about: Show content with an age limit (like 18+). May be unsuitable for children.
As I said above I don't want to only say 18+, since NewPipe blocks videos with any age limit different than 0 if that option is on.

@opusforlife2
Copy link
Collaborator

How about this? The main setting title says "Show age restricted content", so just make the description "May be unsuitable for children."

@B0pol
Copy link
Member

B0pol commented Oct 2, 2020

What happens if the string is translated (old version) but not updated? Will it produce an error with %1$s missing? We may not want to ship it two days before and update

@TobiGr
Copy link
Contributor

TobiGr commented Oct 2, 2020

@B0pol We need to remove all translations of this string to ensure there is no crash. No matter if we put it into this release or the next one.

@B0pol
Copy link
Member

B0pol commented Oct 2, 2020

Then I would delay this change to another PR, there is no reason to delete, thus untranslate this string for this update.

@opusforlife2
Copy link
Collaborator

Then I would delay this change to another PR

You mean another release. 🤭

@B0pol
Copy link
Member

B0pol commented Oct 2, 2020

Both

@opusforlife2
Copy link
Collaborator

Wut. Why another PR and not this PR in another release?

@Stypox
Copy link
Member Author

Stypox commented Oct 2, 2020

What happens if the string is translated (old version) but not updated? Will it produce an error with %1$s missing? We may not want to ship it two days before and update

I tested this and it won't

@opusforlife2 opusforlife2 modified the milestones: 0.20.0, 0.20.1 Oct 4, 2020
@Stypox Stypox removed this from the 0.20.1 milestone Oct 4, 2020
TobiGr
TobiGr previously approved these changes Oct 10, 2020
@Stypox
Copy link
Member Author

Stypox commented Oct 11, 2020

@comradekingu I implemented your suggestions, thank you! @TobiGr please approve again ;-)

@TobiGr TobiGr merged commit 6fce069 into TeamNewPipe:dev Oct 11, 2020
This was referenced Nov 10, 2020
@aand18
Copy link

aand18 commented Nov 23, 2020

🙏 thanks for this! I had enabled restricted because of lacking description and videos weren't loading anymore and didn't have a clue why...

@Stypox Stypox deleted the restriction-strings branch August 4, 2022 09:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
localisation / translation Everything that has to do with translations or Weblate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants