Trying to get rid of the black area #430
Conversation
"Following r-1's suggestion on koreader/koreader#10591 (comment), I got rid of the variables surfaceHeight and surfaceWidth. I don't know if it will break something, but at least it is rendering normally on my devices."
|
Err, yes, you've just broken notches everywhere ;o). |
|
Somehow, it seems to be working 100% here. I'm not a detail-oriented guy. |
|
It works on your device because it doesn't have a hole. It screws all devices with holes. Both Android 13 and below. We can't accept a PR like that |
|
If I understand correctly that means the device is reporting bogus values? Since it doesn't have a cutout and all that? |
I'm just testing with this... Added: window.attributes.layoutInDisplayCutoutMode = WindowManager.LayoutParams.LAYOUT_IN_DISPLAY_CUTOUT_MODE_SHORT_EDGES It seems to have gotten rid of the notch problem on my device, which has a hole but it crashed on my old device, of course.
|
Hi, the latest commit doesn't fix anything. New variables are null so you're returning the same as before. Please, if you want to fix the issue, ask first. We can give you hints to achieve what you want. |
|
Just some crazy tests for now. |
|
No problem :) Always good to see people tinkering in this repo. In case you would like to do another attempt we could:
|
|
Debugging |
|
I've discovered through debugging that the surfaceChanged function is executing some milliseconds after Koreader captures the event C.APP_CMD_CONFIG_CHANGED. This happens more often on older devices. I believe that eventually, this issue could also happens on Android 13, leading to a black area as Koreader draws with the inverted orientation. I reverted the changes and managed to simulate the notch problem on my device with Android 14. I'll attempt to debug this further. |
|
More debugging reveals that the problem is even worse for older devices, such as my Android 6 device: 08-07 22:33:52.797 1824 1837 I KOReader: setting zoom mode to manual 08-07 22:35:01.789 548 1848 I ActivityManager: Config changes=480 {1.3 dualscreenflag=DISABLE ?mcc?mnc en_US ldltr sw702dp w702dp h891dp 320dpi lrg port finger -keyb/v/h -nav/h s.25} Do you notice the recurring line "Config changes=480"? This occurs when I rotate my device to the opposite orientation. We should observe lines similar to: 08-07 22:33:53.974 1824 1824 V Surface : surface changed {... Since the "surfaceChanged" function is not being called, we are not receiving the new dimensions. |
|
I've reverted commit a9d36a0 and have been testing. It appears that the window.decorView.rootWindowInsets.displayCutout method is not being called in full-screen mode. I have now implemented a new method to obtain the cutout information. One thing I've noticed is that sometimes when we first install Koreader it requests all file permissions on the Android system screen. If we attempt to open Koreader after this, it get the weights before the onAttachedToWindow method is called, resulting in an incorrect cutout value. I still haven't tried to tackle this issue. Here is a log demonstrating this behavior: 08-13 11:19:10.718 4421 4421 V Surface : Using Native Content implementation After closing Koreader and reopening it, the problem appears to be resolved for subsequent instances. 08-13 11:20:38.959 4499 4499 V Surface : Using Native Content implementation |
Yup, I definitely noticed that behavior since the PR ;). |
|
I also took a shot at a different approach by making a few changes to the current code, seems to be working and it may be cleaner than this pull request. You can take a look at the changes in this branch: https://github.com/hugleo/android-luajit-launcher/tree/hugleo-fix-cutout But now, there's a new issue that popped up yesterday (koreader/koreader#10808), and I'm thinking about whether he is already using the fixed version from commit a9d36a0 and he is using Android 13, which is supposed work at least with the controls, right? Edit: |
|
Oh, I missed that #10808 was on A13. There may be some Pixel-specific weirdness involved, as I vaguely remember other reports of weirdness on Pixels... |
I prefer this but the old is still in the branch hugleo-fix-cutout-old-alternative
|
For future reference, what are you testing on? |
Just some trouble with github from first time user ;-) |
|
Oh, not that ;p. That was a genuine question, what are you testing the changes on? ;o). |
I tested it some time ago in the Android 13 emulator, both with and without a cutout. I rotated it multiple times and everything looks fine now I don't see the notches anymore. |
Maybe this check is better or is it a waste?
|
Hey, I'm unsuscribing to this ticket because it causes a bit of noise on my inbox. Feel free to play and do crazy tests all you want. If some day this is ready for a review please ping me. |
|
Hi @pazos Is my last change. |
| @@ -236,7 +236,7 @@ fun Activity.requestSpecialPermission(intent: Intent, rationale: String, | |||
| .setCancelable(false) | |||
| .setPositiveButton(ok) { _, _ -> | |||
| startActivity(intent) | |||
| finish() | |||
| finishAffinity() | |||
There was a problem hiding this comment.
I don't understand what are you trying to do here.
There was a problem hiding this comment.
The finish activity is there on purpose, to prevent ANR on bad behaving devices. It happens once in the lifetime of an installation, as the instructions already tell you so.
There was a problem hiding this comment.
When we first install koreader it's ask for permissions like the following images bellow. If I try to open Koreader after that the bottom menu is missing on devices with cutout because somehow gets the wrong dimensions. Not always but eventually happens.
Simply close KOReader and reopen it; this issue won't occur in subsequent runs.
It's not such a big problem, I think, but it seems to get rid of it using a more forceful finish with finishAffinity() that's is not as hard as System.exit(0).
There was a problem hiding this comment.
In that case, lets fix first the bigger problems and lets discard this for the moment.
Once we get something that works on all android versions we can address these little nitpicks ;)
There was a problem hiding this comment.
(I've mentioned it elsewhere before, but I can reproduce similar weirdness on first boot-after-install on my device, FWIW).
| surfaceHeight = height | ||
|
|
||
| // We need to trigger a new configuration event for KoReader to fix rotation when surfaceChanged might be invoked later | ||
| if (onConfigurationChangedCalled) |
There was a problem hiding this comment.
please don't call callbacks within other callbacks. That's strictly wrong. If you need to operate on whatever do it in https://github.com/koreader/koreader/blob/master/frontend/device/android/device.lua#L136
There was a problem hiding this comment.
Right, that's was my main concern about this changes. If it's wrong we try the other way ;-)
Tested and it really works on Lua, thanks.
https://github.com/hugleo/android-luajit-launcher/tree/hugleo-fix-cutout
https://github.com/hugleo/koreader/tree/cutout-handle-surface-in-lua
rotate on C.APP_CMD_CONFIG_CHANGED or C.APP_CMD_WINDOW_RESIZED (surface changed).
It seems to be working, and fix the order when android call surfacechanged later than configurationchanged.
Still can be improved but...
Following your suggestion in #430 (comment) seems better since it's more well tested. You tell me
There was a problem hiding this comment.
mmm, afaik WINDOW_RESIZED always happens after CONFIG_CHANGED.
The issue with WINDOW_RESIZED is that we didn't receive that callback until we backported the glue code (in 623a834)
It looks that you can get rid of the CONFIG_CHANGED entirely (after all we just handle orientation and size changes and that will trigger the next callback)
In the same spirit I bet you can get rid of the if statements that check if it was indeed a change on the width and height and just start with this.device.screen:resize()
There was a problem hiding this comment.
mmm, afaik WINDOW_RESIZED always happens after CONFIG_CHANGED.
The issue with WINDOW_RESIZED is that we didn't receive that callback until we backported the glue code (in 623a834)
It looks that you can get rid of the CONFIG_CHANGED entirely (after all we just handle orientation and size changes and that will trigger the next callback)
In the same spirit I bet you can get rid of the if statements that check if it was indeed a change on the width and height and just start with
this.device.screen:resize()
Let the code speak for itself. I created a match between them.
https://github.com/hugleo/koreader/tree/cutout-handle-surface-in-lua
adb logcat -s KOReader | grep raced
Onyx device:
First time start Koreader
09-23 19:04:37.929 32440 32466 V KOReader: APP_CMD_WINDOW_RESIZED raced
First time we open a document:
none of them raced
Let's rotate
Rotate 1:
09-23 19:02:52.449 32257 32283 V KOReader: C.APP_CMD_CONFIG_CHANGED raced and won the match. Lets rotate here!
09-23 19:02:52.450 32257 32283 V KOReader: APP_CMD_WINDOW_RESIZED raced
Rotate 2:
09-23 19:03:08.435 32257 32283 V KOReader: C.APP_CMD_CONFIG_CHANGED raced and won the match. Lets rotate here!
09-23 19:03:08.436 32257 32283 V KOReader: APP_CMD_WINDOW_RESIZED raced
Rotate n
...
++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
Likebook Mars device:
First time start Koreader:
none of them raced
Fist time we open a document:
09-23 19:22:35.535 1528 1542 V KOReader: APP_CMD_WINDOW_RESIZED raced
09-23 19:22:35.537 1528 1542 V KOReader: APP_CMD_WINDOW_RESIZED raced
Let's rotate
Rotate 1:
09-23 19:26:34.554 1528 1542 V KOReader: C.APP_CMD_CONFIG_CHANGED raced and won the match. Lets rotate here!
09-23 19:26:34.564 1528 1542 V KOReader: C.APP_CMD_CONFIG_CHANGED raced
Rotate 2:
09-23 19:27:27.659 1528 1542 V KOReader: C.APP_CMD_CONFIG_CHANGED raced and won the match. Lets rotate here!
09-23 19:27:27.662 1528 1542 V KOReader: C.APP_CMD_CONFIG_CHANGED raced
Rotate n
...
++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
Android 14 device
(should be similar for Android 13 emulator ;-))
First time start Koreader:
none of them raced
Fist time we open a document:
09-23 19:39:19.107 8470 8499 V KOReader: APP_CMD_WINDOW_RESIZED raced
09-23 19:39:19.107 8470 8499 V KOReader: APP_CMD_WINDOW_RESIZED raced
Rotate 1
09-23 19:40:00.266 8470 8499 V KOReader: C.APP_CMD_CONFIG_CHANGED raced
09-23 19:40:00.301 8470 8499 V KOReader: APP_CMD_WINDOW_RESIZED raced and won the match. Lets rotate here!
Rotate 2
09-23 19:40:27.153 8470 8499 V KOReader: C.APP_CMD_CONFIG_CHANGED raced
09-23 19:40:27.226 8470 8499 V KOReader: APP_CMD_WINDOW_RESIZED raced and won the match. Lets rotate here!
Rotate 3
09-23 19:40:27.153 8470 8499 V KOReader: C.APP_CMD_CONFIG_CHANGED raced
09-23 19:40:27.226 8470 8499 V KOReader: APP_CMD_WINDOW_RESIZED raced and won the match. Lets rotate here!
Rotate 4
09-23 19:40:49.444 8470 8499 V KOReader: C.APP_CMD_CONFIG_CHANGED raced and won the match. Lets rotate here!
09-23 19:40:49.445 8470 8499 V KOReader: APP_CMD_WINDOW_RESIZED raced
Rotate 5
09-23 19:40:59.999 8470 8499 V KOReader: C.APP_CMD_CONFIG_CHANGED raced
09-23 19:41:00.037 8470 8499 V KOReader: APP_CMD_WINDOW_RESIZED raced and won the match. Lets rotate here!
Rotate 6
09-23 19:41:05.681 8470 8499 V KOReader: C.APP_CMD_CONFIG_CHANGED raced
09-23 19:41:05.721 8470 8499 V KOReader: APP_CMD_WINDOW_RESIZED raced and won the match. Lets rotate here!
Rotate 7
09-23 19:41:13.745 8470 8499 V KOReader: C.APP_CMD_CONFIG_CHANGED raced and won the match. Lets rotate here!
09-23 19:41:13.746 8470 8499 V KOReader: APP_CMD_WINDOW_RESIZED raced
Rotate n
Who knows?
The Android thing is a mess, isn't it? :) 😄
There was a problem hiding this comment.
The Android thing is a mess, isn't it? :) 😄
Indeed :/
But I'm not sure what are you looking for.
There was a problem hiding this comment.
The Android thing is a mess, isn't it? :) 😄
Indeed :/
But I'm not sure what are you looking for.
All of this devices will fail when rotate in our current code after a9d36a0. It might not be the first time when rotate for Android13/Android14 but eventually it will :)
So the conclusion is not true that WINDOW_RESIZED always happens after CONFIG_CHANGED ;-)
And as we can see in the race tests we can't get rid CONFIG_CHANGED either for some old devices since WINDOW_RESIZED is never called when rotates.
So rotate in the two events is the only thing I can think ATM.
For the other PR that I'll submit later it's a different story as we obtain the measurements on start in onAttachedWindow ;-)
There was a problem hiding this comment.
All of this devices will fail when rotate in our current code after a9d36a0
Could you please test my suggestions all together?
Please reread all my comments again :), you should start with a working environment, as suggested.
Then confirm that everything is working again on all versions from 4.3 to 13.
Then check what happens with 14.
So the conclusion is not true that WINDOW_RESIZED always happens after CONFIG_CHANGED ;-)
Did it happen before?
There was a problem hiding this comment.
All of this devices will fail when rotate in our current code after a9d36a0
Could you please test my suggestions all together?
Please reread all my comments again :), you should start with a working environment, as suggested.
Sure,
I did. But I don't "reapply the changes just for android 13" just yet and make a different change instead that I will show you if you already tried that function that I used. Android 13 and Android 14 hit success in my teste at first glance ;)
It's true that will fix the notches.
My concern is only about rotation.
But I suspected that people saying in the issues that works for Android 13 only uses it in portrait mode and got lucky when they rotate, because in my tests is more probable that work that not.
Some evidences is that I can simulate the problem in Android 13 (with cutout support enabled in developer options ;))
My Android was 13 before became 14 and I remember facing the rotation problem as well but I don't remember the koreader version
So I already have the changes for Android 1-12, Android 14. Android 13 you see if you can accept the changes or we keep the current ;-)
If everyone is happy I'm totally fine, after all I don't have a Android 13 hardware anymore :)
I'll open the other PR and we work on that to make the changes to make everyone happy ;)
Then confirm that everything is working again on all versions from 4.3 to 13. Then check what happens with 14.
So the conclusion is not true that WINDOW_RESIZED always happens after CONFIG_CHANGED ;-)
Did it happen before?
What do you mean by before?
There was a problem hiding this comment.
I really don't understand the intention of the change.
Doing #430 (comment) gets us the proper behaviour on android 4-12 and android 13. If that's not enough for android 14 please review the developers documentation about what changed.
If you don't find anything relevant then feel free to address the oddities guarded by that specific android version.
It was another method that I tried (The intention was just keep the code clean and avoid the checks on getScreenWidth/getScreenHeight). I also implemented reverting the changes in the branch: https://github.com/hugleo/android-luajit-launcher/tree/hugleo-fix-cutout-old-alternative I didn't know which one was better and choose one myself. Didn't know how to work to github workflow so didn't put that to review first. Sorry for the trouble. Can I replace the changes here for review with that instead? |
|
first see my two comments above.
Yes, or open a new PR. One that reverts a9d36a0 and other that reapplies the changes there for android 13. We most likely want to keep the changes here for a few weeks, since any stuff that reaches the main repo will cause a lot of noise on bug reports. In a couple of days I'll have my dev machine ready to poke a bit with android, something that I didn't seriously for the last couple of years. |
will delve into it some other way some other time.
forget to put the changes also in this branch
|
superseeded by #439 |
"Following r-1's suggestion on koreader/koreader#10591 (comment), I got rid of the variables surfaceHeight and surfaceWidth. I don't know if it will break something, but at least it is rendering normally on my devices."
This change is