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

Reverted cutout and fix rotations #439

Merged
merged 8 commits into from Dec 22, 2023
Merged

Conversation

hugleo
Copy link
Contributor

@hugleo hugleo commented Sep 24, 2023

This change is Reviewable

@hugleo hugleo marked this pull request as draft September 24, 2023 04:04
@hugleo
Copy link
Contributor Author

hugleo commented Sep 24, 2023

Wondering why the changes for Android 14 don't work for me after I merged the changes with the checks and then I went to logging.

09-24 09:50:34.558 20926 20926 V Android_Version: onAttachedToWindow Android version is: 33
09-24 09:50:34.595 20926 20926 V Android_Version: surfaceChanged Android version is: 33
09-24 09:50:34.649 20926 20959 V Android_Version: getScreenWidth Android version is: 33
09-24 09:50:34.649 20926 20959 V Android_Version: getScreenHeight Android version is: 33

It is because my device is Android 13. DUH, DUH, DUH. I feel so dumb. That Xiaomi version screen really tricks me out."

Screenshot_2023-09-24-09-57-03-291_com android settings

Now the discussion about Android 14 doesn't make any sense since I actually have an Android 13 device.

@pazos Should I then apply my working changes to Android 13 then since I simulate the problem now on a real device?

Oh boy....

}
// ONLY FOR ANDROID 14
else if (android.os.Build.VERSION.SDK_INT == 34) {
val cut = windowManager.defaultDisplay.cutout
Copy link
Member

Choose a reason for hiding this comment

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

Does this chunk of code works for android 9 - 12?

It would be wonderful to have a single codepath for all versions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does this chunk of code works for android 9 - 12?

It would be wonderful to have a single codepath for all versions.

That's what I would like to show you!

for android <= 12 as you say so works.
val cut: DisplayCutout? = window.decorView.rootWindowInsets.displayCutout

for android 13 this seems to be the secret:
windowManager.defaultDisplay.cutout
Following the doc this method should work for >= P

For now I've tested on devices without the hole on some devices < 13 and works.
But I can test some more on the emulator enabling holes in the dev options.

Did you remember to try the method windowManager.defaultDisplay.cutout before?

Copy link
Member

Choose a reason for hiding this comment

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

Did you remember to try the method windowManager.defaultDisplay.cutout before?

I actually don't remember. I think I didn't try anything. Just read the documentation, find something useful, went with that and pushed an APK for others to test. Since the test were ok I never bothered again :)

So please go with that for everything.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did you remember to try the method windowManager.defaultDisplay.cutout before?

I actually don't remember. I think I didn't try anything. Just read the documentation, find something useful, went with that and pushed an APK for others to test. Since the test were ok I never bothered again :)

So please go with that for everything.

Tested on emulator and doesn't work for Android 9. Need Android >= 10

Copy link
Member

Choose a reason for hiding this comment

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

Ok, thanks!.

Then from 9 up to 12 the legacy method and from 13 onwards the new one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, thanks!.

Then from 9 up to 12 the legacy method and from 13 onwards the new one.

Already there ;-)

else if (android.os.Build.VERSION.SDK_INT == 34) {
val cut = windowManager.defaultDisplay.cutout
if (cut != null && cut.boundingRects.isNotEmpty()) {
// TODO: we can handle various kinds of cutouts: getSafeInsetLeft, getSafeInsetRight, getSafeInsetTop, getSafeInsetBottom
Copy link
Member

Choose a reason for hiding this comment

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

Not a fan of random TODO lists.

Do you have something in mind to do with those cutouts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not a fan of random TODO lists.

Do you have something in mind to do with those cutouts?

Yes. Today we only deal with topInsetHeight when the hole is at the top.
On the emulator, we can enable holes in any direction: top, bottom, left, or right.
When you guys changed surfaceWidth = width; surfaceHeight = height inside the surfaceChanged method, it enabled support for all kinds of holes.
When I reverted the changes I realized that it was not implemented in the old version ;-)

Copy link
Member

Choose a reason for hiding this comment

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

Ok, so remove the TODO and add a NOTE instead.
Somebody else that reads the code in the future might find that useful.

Not so with a TODO.

Clean up the code and implement cutout InsetBottom
@pazos
Copy link
Member

pazos commented Dec 15, 2023

@hugleo: sorry for the delay.

two nitpicks and this can be merged.

Please add a log statement in the new condition, too. Use exactly the same string, so logs provide the same feedback in all android versions.

Please fix https://app.codacy.com/gh/koreader/android-luajit-launcher/pullRequest?prid=12685762

@hugleo
Copy link
Contributor Author

hugleo commented Dec 15, 2023

@pazos nitpicks addressed

if (Build.VERSION.SDK_INT >= 33) {
val cut = windowManager.defaultDisplay.cutout
if (cut != null && cut.boundingRects.isNotEmpty()) {
insetsHeight = cut.safeInsetTop + cut.safeInsetBottom
Copy link
Member

Choose a reason for hiding this comment

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

any reason to take in account safeInsetBottom?. Did you find a device (I mean any device outside an emulator with fake cutouts) where it is not 0?.

It's not a vetto. Just trying to figure out what's the best option:

a) ignore cut.safeInsetBottom on both branches.
b) count cut.safeInsetBottom on both branches.

Anyhow, I'm fine if you go with b* but then please use the same log statement.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

any reason to take in account safeInsetBottom?. Did you find a device (I mean any device outside an emulator with fake cutouts) where it is not 0?.

It's not a vetto. Just trying to figure out what's the best option:

a) ignore cut.safeInsetBottom on both branches. b) count cut.safeInsetBottom on both branches.

Anyhow, I'm fine if you go with b* but then please use the same log statement.

No, see only in the emulator. Fine with a) since it makes more sense for the log as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think can get rid of cut.boundingRects.isNotEmpty() as well since I don't remember if it's needed and it would save some duplication

Copy link
Member

@pazos pazos left a comment

Choose a reason for hiding this comment

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

Looks much better, thanks!

@jospalau
Copy link

This broke the screen for my Xiaomi. I don't read on the phone. Just letting you know. MIUI 14 too.

I am using the patch I used to use previously to the original fix that this is reverting to add the missing height.

@hugleo
Copy link
Contributor Author

hugleo commented Feb 14, 2024

Do you have a screenshot of the problem?

@jospalau
Copy link

Sure.

Screenshot_2024-02-14-22-11-36-977_org koreader launcher

@hugleo
Copy link
Contributor Author

hugleo commented Feb 14, 2024

If you rotate to landscape in KOReader options and then back to portrait, does the problem go away?

@hugleo
Copy link
Contributor Author

hugleo commented Feb 14, 2024

Also tell which device model

@jospalau
Copy link

No. In landscape mode the cut is on the right side.
Screenshot_2024-02-14-22-26-18-349_com android settings

@hugleo
Copy link
Contributor Author

hugleo commented Feb 14, 2024

This device has a hole at the top just like mine. The black bar is supposed to fill the hole and should be drawn at the top. However, on yours, it is being drawn at the bottom. Unless, of course, you have a rotation configured like the picture below, this should be the normal behavior.

rotation

@jospalau
Copy link

I have it by default but I don't know why the order of the icons is different than in yours.
IMG_20240214_224916

I think I know what you mean but I want it full screen like the rests of the apps.

Previous code reverted and the patch makes it work:
local screen = require("android").screen
screen.height = screen.height +90

@hugleo
Copy link
Contributor Author

hugleo commented Feb 14, 2024

I have it by default but I don't know why the order of the icons is different than in yours. IMG_20240214_224916

I think I know what you mean but I want it full screen like the rests of the apps.

Previous code reverted and the patch makes it work: local screen = require("android").screen screen.height = screen.height +90

Your default setting is correct; in my case, I intentionally changed it. Your black bar should be at the top for this rotation mode.

@jospalau
Copy link

It is not a problem to be honest. Just curious.
Thanks

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.

None yet

3 participants