gujjwal00 / avnc

VNC Client for Android
GNU General Public License v3.0
596 stars 57 forks source link

Virtual keys cause unexpected offset/zoom #237

Closed breakingspell closed 3 weeks ago

breakingspell commented 1 month ago

Hello, I have a report on a regression after v2.5.0 with the virtual keys rewrite (otherwise an excellent improvement!) https://github.com/gujjwal00/avnc/commit/9a0dfdef9ddee74a410abf6aef0f8cf2af594378


Description

Summoning the device keyboard with the new virtual keys visible will cause the zoom to change when the device keyboard is invoked or dismissed. Likely related, if the virtual keys are visible on session start, this also offsets the viewer by the height of the two-row bar, which causes vertical travel that the zoom lock would normally prevent.


Notes Quick recordings of the expected behavior (2.4.2)

https://github.com/user-attachments/assets/7ac024dd-59a2-4be4-bee9-89167b3db32b

and then the regression (2.5.1)

https://github.com/user-attachments/assets/e01d5d64-a549-4e5b-b0cc-2ab2681eefd9

Essentially there shouldn't be any zoom changes thanks to the zoom lock (another excellent feature). It's most noticeable if you have a remote matched to your device screen height.

It's like the keys are now being drawn in the same region as the viewer. Quick glance makes me think it may be related to the style changes in app/src/main/res/layout/virtual_keys.xml app/src/main/res/values/styles.xml


Additional Info

gujjwal00 commented 1 month ago

Hi @breakingspell , This is how virtual keys have behaved in previous versions too. You might be just now noticing it now because virtual keys are a bit taller, and they retain visibility across sessions.

breakingspell commented 1 month ago

Hello @gujjwal00, I do notice now that the offset behavior is the same in both versions (1-row virtual bar also offsets the viewer, it just wasn't behavior yet to show by default). The vertical travel/offset is undesirable when the guest height is locked to device height, but not a "regression" then :slightly_smiling_face:

However, zoom is still affected and probably the most pressing. Zoom seems to be retained in all cases on the old version, no small "jump" as the latest version does.


~~Could the virtual keys be treated as "floating" in some manner to prevent both of these behaviors? I found some posts regarding elements and changing scrolling/floating behavior by cleverly nesting the <layout> structure, I want to test and see if they help~~


Edit - I've found what seems to be the actual culprit when testing a debug build: Settings -> Viewer -> Fullscreen

When Fullscreen is enabled (default), the zoom issue does not occur on 2.5.x with virtual keys present or not. When Fullscreen is disabled (usual for system Gesture Nav users), the zoom will jump when the virtual keys are present, and does not jump when the keys are hidden. In 2.4.x, the zoom behavior does not seem affected by the fullscreen mode.

Any ideas? :sweat_smile: I noticed a comment here that almost made me chuckle: https://github.com/gujjwal00/avnc/blob/master/app/src/main/java/com/gaurav/avnc/ui/vnc/LayoutManager.kt#L33. I'm comparing the differences for now and seeing if anything stands out that needs to be updated or reference the new keys. Regarding differing API versions, I can confirm the behavior is the same on Android 11 (30) and 14 (34)

gujjwal00 commented 1 month ago

Thanks for the details. You have already done the legwork for me. I can confirm the issue.

It seems to be caused by the use of ViewPager2 for hosting the virtual keys. For some reason, simply adding the ViewPager2 causes the root view to report incorrect size which in turn affects the base zoom level.

Getting ViewPager2 to work for this use case was already quite cumbersome, and it didn't do as well with the users as I had hoped for. So, it needs to be replaced with a better alternative.

breakingspell commented 1 month ago

Glad I could help ID the root cause. I didn't realize ViewPager2 was a standard library till just now, learning at each step of this!

In testing yesterday I tried adding and removing some of the android: content parameters and moving them between the layout nodes, no positive effect. I haven't ran into other issues with the virtual keys, to note.

Now knowing the value is being reported incorrectly by this library, I wonder if there's a way to check for the condition that triggers it (!isFullscreen && virtKeysVisible) and fix the value by re-checking or using another method to check size.

gujjwal00 commented 1 month ago

So the root cause was that ViewPager2 uses RecyclerView internally, which sets itself as a 'scrollable' view. When keyboard is shown, Android checks if there is a scrollable view, and if one is found, it will resize the Activity, hoping that user can just scroll up/down to access content hidden by keyboard.

I have re-implemented virtual key layout, and replaced ViewPager2 with ViewPager. It has simplified a lot of things, and fixed a bunch of issues, including this one.

Test APK: https://github.com/gujjwal00/avnc/actions/runs/10096008170/artifacts/1739904356

breakingspell commented 1 month ago

Terrific, the zoom works as expected. The virtual keys also fit horizontally with Show All Keys now, they didn't before.

Great refinement, thanks!

breakingspell commented 3 weeks ago

Thanks again, this is fixed 💯