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

Bugfix(android): dynamic pixel density change #2679

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

kedikx
Copy link

@kedikx kedikx commented Oct 7, 2024

This is a fix for #2635.

Summary:

  • PlatformContext: implement get/set pixel density methods, set/update current density from ReactContext on resume.

  • RNSkAndroidPlatformContext: reuse getPixelDensity() from jniContext instead on base member variable.

@wcandillon
Copy link
Contributor

Thanks a lot, can you recommend me a way to test the pixel density change? Is this an issue on iOS as well?

@kedikx
Copy link
Author

kedikx commented Oct 7, 2024

Thanks a lot, can you recommend me a way to test the pixel density change?

Link to repo/app I used to spot the problem and test the solution.
https://github.com/kedikx/rns-density

I also had a plan to add case to app/paper or dedicated app/density, but suppose it will be another story.
#2635 also have the video for use case.

Is this an issue on iOS as well?

I am not aware of possibility to change density on iOS.
Never saw it, at least from a user point of view.

@kedikx
Copy link
Author

kedikx commented Oct 7, 2024

I have signed the CLA!

@kedikx
Copy link
Author

kedikx commented Oct 7, 2024

can you recommend me a way to test the pixel density change?

General idea:

You need a random case where canvas and it's content depends on container size (i.e. "button skin").

Steps:

  1. Get canvas on the screen and check image - it should look like expected by design.
  2. Go to Android Settings -> Display -> Display size & text -> Change "Display Size".
  3. Go back to App: In case of problem: canvas size will change (could be verified with Fill), but "skin" will not be resized: it will either smaller or bigger than the container. Otherwise, it should look similar to (1) but shrink/expand together with container.

NB!:
/android/app/src/main/AndroidManifest.xml should have 'density' in configChanges
(fontScale also if you want to get things even more complicated):

android:configChanges="...|density|fontScale|..."

@wcandillon
Copy link
Contributor

@kedikx Sorry it took me so much time to get back to you on this. The reason I was a bit silent on this is that where were doing refactoring where we thought the problem would be addressed differently. But indeed this is makes sense and I am now looking to merge this change.

@@ -99,6 +101,15 @@ public byte[] getJniStreamFromSource(String sourceUri) throws IOException {
return null;
}

void onResume() {
Copy link
Contributor

Choose a reason for hiding this comment

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

should this be "onConfigurationChanged"?

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