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

Add RTL layout support #7652

Closed
wants to merge 9 commits into from

Conversation

triallax
Copy link
Contributor

@triallax triallax commented Jan 13, 2022

What is it?

  • Feature (user facing)

Description of the changes in your PR

Add RTL layout support to NewPipe.

Based on #3488 by @Royosef (thank you for your work!). Differences:

  • Fixed merge conflicts
  • Removed RTL handling from AboutActivity, since ViewPager2 supports RTL natively
  • Other minor refactors

Issues to fix:

  • Dialog action buttons should be reversed in order
  • Broken with YouTube's Fast Forward/Rewind behavior #4833
  • fast-forward/reverse seek in player (double taps) are not reversed accordingly seekbar isn't flipped anymore, so this isn't necessary (it might be necessary to investigate the player more closely in general)

Before/After Screenshots/Screen Record

Before After
Subscriptions before Subscriptions after
Drawer before Drawer after
Comments before Comments after
Player before Player after
About us before About us after
Licenses before Licenses after

Fixes the following issue(s)

APK testing

The APK can be found by going to the "Checks" tab below the title. On the left pane, click on "CI", scroll down to "artifacts" and click "app" to download the zip file which contains the debug APK of this PR.

Due diligence

NOTE: SQUASH MERGE

Royosef and others added 6 commits April 25, 2020 23:42
* supportsRtl="true"
* replace Left/ Right properties with Start/ End
* tabs:
	* layoutDirection="ltr" for <TabsLayout>
	* reverse items for RTL in PagerAdapter
* properties for force right in RTL:
	* textAlignment="viewStart"
	* textDirection="anyRtl"
* export getLayoutPosition() & isRTL() to LocalizeLayoutUtil class
* fix info section direction in report error page
* fix margin at the feed group carousel in subscriptions page
@triallax
Copy link
Contributor Author

triallax commented Jan 13, 2022

Regarding this suggestion from #3488:

I'd merge LocalizeLayoutUtils (used to know if the device should be RTL and one method around that) and AndroidTvUtils to DeviceUtils.

I don't think we should merge LocalizeLayoutUtils into DeviceUtils. It doesn't make sense to call a device RTL; that's a description for a language script. Also, if the user chooses a language in NewPipe different from their system's, then it's not really related to their device anymore.

@triallax triallax added GUI Issue is related to the graphical user interface localisation / translation Everything that has to do with translations or Weblate labels Jan 13, 2022
@triallax triallax changed the title Add rtl layout support Add RTL layout support Jan 13, 2022
@opusforlife2
Copy link
Collaborator

It doesn't make sense to call a device RTL

Indeed.

@triallax
Copy link
Contributor Author

triallax commented Jan 13, 2022

The CI is failing because of this line:

On first look this doesn't seem to be related to the PR, but no such failure is present on dev, so either the test is flaky, or I've messed up something.

Edit: this is probably #7553.

@triallax triallax marked this pull request as draft January 13, 2022 21:04
@sonarcloud
Copy link

sonarcloud bot commented Jan 14, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@triallax
Copy link
Contributor Author

So the only issue I found so far with my PR is this:
App in Arabic but system in English

Both app and system in Arabic

In both screenshots, NewPipe's language is Arabic, but in the first one, the system language is English, while in the second it's Arabic. As you can see, the buttons are in different positions. I've checked with some other apps (e.g. Tasks) and they don't seem to exhibit this issue (i.e. they behave as the second screenshot when in RTL).

Do you think this is a blocker?

This makes things simpler. For instance, there's no need to worry
anymore about the seekbar preview.
@triallax
Copy link
Contributor Author

triallax commented Feb 6, 2022

If I want to always use LTR in certain places, is it better to use left and right, or should I use start and end but with android:layoutDirection="ltr"?

@Stypox
Copy link
Member

Stypox commented Feb 16, 2022

@mhmdanas probably left and right so that only the things that you specify are affected, otherwise (I think) android:layoutDirection="ltr" would propagate to all attributes and children. Unless you just want the latter ;-)

Do you think this is a blocker?

No, but if you find a way to solve it it's better ;-)

@triallax
Copy link
Contributor Author

The player is too complicated for me to touch in this PR, so I'll try to leave it as-is for now, and perhaps open an issue when this PR is merged.

@triallax
Copy link
Contributor Author

triallax commented Aug 1, 2023

I don't think I'll be picking this up, so I'll be closing it (though I'd be happy to see someone else finish up the work).

@triallax triallax closed this Aug 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
GUI Issue is related to the graphical user interface localisation / translation Everything that has to do with translations or Weblate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RTL UI for rtl languages such as ckb,ku,ar,urdu...etc
4 participants