Skip to content

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