termux / termux-x11

Termux X11 add-on application.
https://termux.dev
GNU General Public License v3.0
2k stars 303 forks source link

Dex touchpad fix: orientation, click, gesture, etc. #617

Closed knyipab closed 3 months ago

knyipab commented 5 months ago

In short, it should fix #419. The suggested changes should have no impact on existing functionality. If you need to review the code changes and features added, I hope below explanation helps and won't be too long.

Root Causes of #419

My solution

knyipab commented 5 months ago

Force pushed fix to crash in "Automatic (for touchpad)" mode mentioned in #419.

knyipab commented 4 months ago
@twaik perhaps we should rehthink the gesture, though I do not have a concrete idea in mind. After some time testing this PR with soft touchpad in DeX, I found that there are not enough gesture/button to accomplish all four features below. feature default gesture/button (with this PR) comment
escape pointer capture 3+ finger swipe down necessary cuz soft keyboard often has no "ESC" key
activate soft keyboard 3+ finger swipe up necessary cuz soft touchpad users often rely on soft keyboard
toggle EK bar vol down highly preferred feature as EK bar is useful for DeX + soft keyboard users
volume up / down None when preference set to not intercept physical vol+/- buttons, it will work adjust volume, but sacrifices the function of toggling EK bar
twaik commented 4 months ago

Remove the "if condition" so that touchpad event can invoke TrackpadInputStrategy. Note that we could not simply create another variable to be invoked because the code logics test mInputStrategy at multiple places. I found it easiest to just override mInputStrategy in DeX (SamsungDexUtils.checkDeXEnabled(mContext)). This makes sense because (1) finger input in DeX can only be touchpad not touchscreen, and (2) mouse input handler in DeX is also overrided.

The touchpad handling code is created for both Dex and real touchpad cases. And there may be physical touchpad, mice and touchpad connected to the same device at the same time. That is a reason I am invoking the code handling touchpads and Dex in separate TouchInputHandler instance with forced trackpad mode. mInputStrategy overriding will break this case.

knyipab commented 4 months ago

I am indeed well aware of the fact that touchpad + mouse should be fine, but in touchpad + mouse + touchscreen (in DeX) or perhaps simply touchscreen in DeX, the touchscreen mode would be completely overridden. Besides, I am not sure how touchscreen can behave under onscreen DeX mode (as a touchpad? or mouse? does Termux:X11 touchscreen mode work?) which is only available to Tablet user and I don't own one.

It is just that

the code logics test mInputStrategy at multiple places

So it will require some more changes to your code base (probably a real mTouchpadHandler resembling touchpad InputStrategy). I was afraid to do such big change and unsure about my expertise.

It's up to you. Want me to implement it (though not any time soon) or you change it directly? Or is it still acceptable as is?

twaik commented 3 months ago

Ok, probably it is a good time to make a rebase.

So it will require some more changes to your code base

In the case if you detect event came from touchpad you should redirect it to mTouchpadHandler immediately (in TouchInputHandler::onTouch).

About

                if (capturedPointerTransformation == CapturedPointerTransformation.AUTO) {
                    for (Display display : mDisplayManager.getDisplays()) {
                        if (display.getState() == Display.STATE_ON) {
                            transform = display.getRotation() % 4;
                            break;
                        }
                    }
                }

I still think it is a good idea to use DisplayManager::getDisplay(Display.DEFAULT_DISPLAY) which should return phone's primary display.

About

                case MotionEvent.ACTION_BUTTON_PRESS:
                case MotionEvent.ACTION_BUTTON_RELEASE:
                    // For hardware touchpad in DeX (captured mode), handle physical click buttons
                    if ((event.getSource() & InputDevice.SOURCE_TOUCHPAD) == InputDevice.SOURCE_TOUCHPAD) {
                        currentBS = event.getButtonState();
                        for (int[] button: buttons)
                            if (isMouseButtonChanged(button[0]))
                                mInjector.sendMouseEvent(null, button[1], mouseButtonDown(button[0]), true);
                        savedBS = currentBS;
                        break;
                    }

I still think it is a good idea to move it out from switch. There are some weird firmwares with custom actions hardcoded. Sending event will not be triggered in the case if button state is not changed so it should be safe.

Also be aware of new preferences making possible to configure three-finger swipes, back button, volume keys actions. Also there is new option to toggle soft keyboard from additional keyboard bar (enabled by default) so the changes overriding default actions in dex mode should be removed.

knyipab commented 3 months ago

Rebased and addressed all your comments.

In the case if you detect event came from touchpad you should redirect it to mTouchpadHandler immediately (in TouchInputHandler::onTouch).

Sounds smart and requires minimal changes. I frontload the mTouchpadHandler in handleTouchEvent in https://github.com/termux/termux-x11/pull/617/commits/a5a91473a0224f9c8391f970147ddd917affdae1. Now mInputStrategy overriding only takes place when it is mTouchpadHandler. Perhaps a thorough test is probably having a tablet owner to use physiccal touchpad and touchscreen in non-trackpad mode together.

I still think it is a good idea to use DisplayManager::getDisplay(Display.DEFAULT_DISPLAY) which should return phone's primary display.

Thanks. While mActivity.getLorieView().getDisplay() does not work well for my foldable device soft touchpad under DeX, DisplayManager::getDisplay(Display.DEFAULT_DISPLAY) works perfectly.

I still think it is a good idea to move it out from switch. There are some weird firmwares with custom actions hardcoded. Sending event will not be triggered in the case if button state is not changed so it should be safe.

I moved it out of the switch statement. See if you may want to invite other users to conduct a final UAT cuz I don't own a tablet.

Also be aware of new preferences making possible to configure three-finger swipes, back button, volume keys actions. Also there is new option to toggle soft keyboard from additional keyboard bar (enabled by default) so the changes overriding default actions in dex mode should be removed.

I am fine with new preferences and now this PR leaves the three-finger swipe gestures untouched. Cuz I found that under DeX soft touchpad mode, I can escape the pointer capture by swipe up at the bottom of phone screen. That will bring up the bottom "navigation gesture bar" (not sure the correct name) and escape pointer capture. Nonetheless, it assumes new Termux:X11 user to know such trick, and perhaps some first time user would be panic and ends up unplugging the HDMI cable to escape.

Well... perhaps this app has already made quite some assumptions about new users when we don't have a starter guide LOL. For example, I discovered preferences page can be triggered from notification bar after few weeks of use, or perhaps I am stupid haha.

knyipab commented 3 months ago

By the way,

  1. shall we make "transform captured pointer movements" by default to be "Automatic (for touchpad)"? It seems harmless and perhaps enhane user experience
  2. would it be better to show the current chosen option right below "transform captured pointer movements"? I couldn't quite find where to implement unless dive deeper in your code.
twaik commented 3 months ago

Well... perhaps this app has already made quite some assumptions about new users when we don't have a starter guide LOL. For example, I discovered preferences page can be triggered from notification bar after few weeks of use, or perhaps I am stupid haha.

Not stupid. I have some troubles with writing user manuals. Also yesterday I pushed a commit which adds preferences and toggle soft keyboard actions to the additional key bar.

shall we make "transform captured pointer movements" by default to be "Automatic (for touchpad)"? It seems harmless and perhaps enhance user experience

I already checked it when tried to fix touchpad by myself. It breaks touchpads on other devices.

would it be better to show the current chosen option right below "transform captured pointer movements"? I couldn't quite find where to implement unless dive deeper in your code.

I think it will be enough to change the summary of preference when it changes. I'll do that a bit later.

twaik commented 3 months ago

Probably polling for display rotation with mDisplayManager.getDisplay(Display.DEFAULT_DISPLAY).getRotation() on every single event is not very wise. You can get more than 200 input events in a sec and invoking IPC for every single event seems to be wasteful. Probably it would be better to create self-calling lambda like checkXEvents which invokes MainActivity.handler.postDelayed to call itself every 300 ms which will save current rotations somewhere.

twaik commented 3 months ago

@AlphaBs @hansm629 can you please test latest CI artifact of this PR?

hansm629 commented 3 months ago

@AlphaBs @hansm629 can you please test latest CI artifact of this PR?

@twaik I'll test it out after work today

AlphaBs commented 3 months ago

I tested this build on Galaxy Tab S8+ bookcover keyboard, with trackpad, capturing external pointer, and automatic point movements mode.

Touch gestures (tap, two-finger tap to right click, two-finger scroll, click buttons, long tap to drag, tap-to-move), scroll axis, pointer movement axis, pointer speed are all normal and work perfectly. Thanks for a great job!

knyipab commented 3 months ago

@twaik just pushed a commit to update rotation using DisplayManager#registerDisplayListener() which works on my phone. See if you may have concern about firmware/vendor-specific issue and prefer MainActivity.handler.postDelayed.