-
-
Notifications
You must be signed in to change notification settings - Fork 5.4k
feat(view): Enable mouse cursor movement in shell readline #4775
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
base: master
Are you sure you want to change the base?
feat(view): Enable mouse cursor movement in shell readline #4775
Conversation
|
@TomJo2000 kindly look into this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi thanks for the PR, I don't do reviews in this repo and I'm currently out sick, so I'm taking a bit of a break from triage as well.
I'm not that fluent in Java but this seems to pass a preliminary sniff test.
|
@obfusk @fornwall @agnostic-apollo @ayoubelmhamdi Kindly look into the PR. |
from the video, i saw the experience will be more awful than i expected before, because we always make wrong presses while using touch screen, but the cursor will be moved to the first character in the first line, we should think about it, after people use this feature. |
e66dfda to
4e3c4e0
Compare
|
Hi, the improved version: the issue part: |
|
Hi, just a quick follow-up to note that I've updated the PR with the requested fixes. |
|
Hi, I appreciate the value of this idea and I believe it would be useful for some people, however, I have two requests: Even though I have many years of experience with terminal emulators, I have never personally seen any terminal emulator on desktop PC that implements this feature. Because of that, I believe it should not be the default behavior, but should be configurable to be enabled by a setting in the Termux App "Settings" menu. If it's not inconvenient for you to do this additional work, could you please try the following steps?
|
4e3c4e0 to
3aeb1b6
Compare
|
Hi, I've pushed the final updates. The feature is now controlled by an on/off switch in the settings, as requested. I've also added the boundary check to prevent the cursor from jumping on accidental taps in empty areas. Here is a new video demonstrating the final behavior, including the settings toggle: made_button_inSettings.mp4I'm also participating in Hacktoberfest, so I'd be grateful if this contribution could be considered for the event. Thank you again for all your guidance! |
| // vvv YOUR NEW CODE vvv | ||
| case "mouse_cursor_movement_enabled": | ||
| mPreferences.setMouseCursorMovementEnabled(value); | ||
| break; | ||
| // ^^^ END OF NEW CODE ^^^ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was it chatgpt?
| switch (key) { | ||
| case "terminal_margin_adjustment": | ||
| return mPreferences.isTerminalMarginAdjustmentEnabled(); | ||
| // vvv YOUR NEW CODE vvv |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And here too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the feedback! I added those markers to visually separate the new code but see now they should be removed. I've pushed a commit to clean them up.
@twaik
| if (diff > 0) { // Need to move right | ||
| for (int i = 0; i < diff; i++) { | ||
| handleKeyCode(android.view.KeyEvent.KEYCODE_DPAD_RIGHT, 0); | ||
| } | ||
| } else if (diff < 0) { // Need to move left | ||
| for (int i = 0; i < -diff; i++) { | ||
| handleKeyCode(android.view.KeyEvent.KEYCODE_DPAD_LEFT, 0); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like a bad idea to me. It will not work fine in the case if text editing widget is not as wide as screen (i.e. widget has borders or other decorations).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have tested this PR on my device Samsung Galaxy S7 SM-G930U.
This PR is working, however, as should be expected,
(and as also applies to the previous, similar PR):
this mode does work in regular shells like bash, fish,
but it does not work very well (but does a little bit) in curses or curses-like programs like nano.
that is why I asked for it to be disabled by default, and only enabled by a setting (which I can confirm, in the current version of this PR, the setting is not enabled by default)
| public void reloadPreferences() { | ||
| android.content.SharedPreferences prefs = androidx.preference.PreferenceManager.getDefaultSharedPreferences(getContext()); | ||
| mIsMouseCursorMovementEnabled = prefs.getBoolean("mouse_cursor_movement_enabled", false); | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I said before, it will be better to use termux.properties for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, however, it might be relevant to mention that currently this preexisting setting:
and this preexisting setting:
both do very similar (but subtly different) things, and there appears to currently be no way to configure the first setting purely from termux.properties, or vice versa, the second setting purely from the GUI,
therefore, if either setting is modified from the default, software keyboard will not work, and both must be set to the default before software keyboard works again.
this might be a different topic from what this PR is about, but this summary explains why I didn't mention termux.properties in my suggestion - it is because the currently-existing settings are confusing, so there is not really, as far as I can see, a good example to follow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The setting in PR should go in properties, but in my local changes, both props and prefs have been rewritten, so would be moot to work on it currently.
The first setting for soft keyboard enabled is in settings because if it was in properties file and soft keyboard gets disabled, you wouldn't be able to enable it again without an external keyboard to type into the terminal to open the properties file, hence its in a UI. The later hide property only hides soft keyboard at startup and tapping screen brings it up again.
Eventually all settings will be moved to properties or yaml file and shown in a UI, but that's in distant future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The later hide property only hides soft keyboard at startup and tapping screen brings it up again.
In my testing, both settings caused the software keyboard to remain closed and simply tapping the screen did not make it appear on my device if either setting was changed from its default. The subtle difference between them I mentioned is something else that is very subtle and harder to explain. However, the problem of the "hide-soft-keyboard-on-startup" setting making the software keyboard difficult to access because of it no longer appearing when touching the Termux Activity might be a bug that isn't present in your rewritten version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My rewritten solution doesn't make any changes for keyboard.
Hide on startup only hides keyboard and should show on tap, while disable setting, completely disables it until toggled with keyboard key or setting. You likely need to restart termux app after making changes for them to properly take effect. Soft keyboard enabled should be enabled, and hide set to true, and then restart.
What does it take for keyboard to appear again with hide?
termux-app/app/src/main/java/com/termux/app/terminal/TermuxTerminalViewClient.java
Line 598 in 7bceab8
// If soft keyboard is to be hidden on startup
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you are correct, when I test again I am unable to reproduce the problem I saw before on any of my devices, so I must have been wrong. I don't know what happened because I thought that I set everything to default and restarted Termux using the exit command in between tests. thank you for pointing out what the actual behavior is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Welcome and thanks for the confirmation for saving me work, there was a chance there could have been a bug on some devices. Android does not provide any API to check if keyboard is open or not, so termux code for it is very hacky, which could have caused problems, but it was well tested before I merged it and I haven't received any other reports. I tend not to touch that code.
| android.content.SharedPreferences prefs = androidx.preference.PreferenceManager.getDefaultSharedPreferences(getContext()); | ||
| mIsMouseCursorMovementEnabled = prefs.getBoolean("mouse_cursor_movement_enabled", false); | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Termux does not use androidx preferences. Probably it would be better to use existing termux.properties file and related code.
3aeb1b6 to
928d003
Compare
|
Hi @twaik and @robertkirkman, Thanks so much for the detailed feedback! You're right, @twaik, my calculation logic is too simple and will break. I'd love to fix it, but I'm not sure of the best way to map the screen coordinates to the shell's command buffer. If you have any suggestions on the right approach, I'd really appreciate it! And thanks, @robertkirkman, for the testing and for the context on the settings. I'm happy to implement the setting however the team decides is best. I'll hold off on pushing more code until I hear your thoughts on the calculation fix. Thanks again! |
|
@TomJo2000 @twaik @robertkirkman any info |
|
Neither of us 3 merge pull requests in this repository. |
|
can you help me if to know if any changes required. or if someone who can take this serious.. |
|
There are many PRs waiting in order before this PR. The people who can merge PRs in this repository will be busy with all of the other ones before they reach this one. |
|
Hi, sorry about late reply, I am busy with termux-app rewrite and won't be merging any pulls before that, and as robert said some more important PRs will take precedence even after I finish rewrite and push. And this cannot be merged anyways before rewrite as it needs changes to properties, which conflict with my local rewrite changes. Additionally, such changes for mouse tracking in terminal will need to be looked into carefully to not break things and whether it is viable to merge, and whether issues above for ncurses can be solved. That will take time, I am also not very familiar with mouse related implementation in terminal, so will need to research first too. So I cannot give you any immediate timeline for merging this, sorry. |
| @Override | ||
| public boolean getBoolean(String key, boolean defValue) { | ||
| if (mPreferences == null) return false; | ||
| if (mPreferences == null) return defValue; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this catch though.
This pull request adds support for moving the cursor within a long command in the shell by clicking with a mouse or tapping on the touchscreen. This makes editing multi-line commands significantly easier and faster.
How it Works
The implementation works by:
Intercepting ACTION_DOWN events from both mice and touchscreens in TerminalView.
Calculating a 1D character index for both the click location and the current cursor position, correctly handling multi-line wrapping.
Sending the appropriate number of left or right arrow keypress events (KEYCODE_DPAD_LEFT/RIGHT) via the existing handleKeyCode() method to move the cursor to the target location.
This approach ensures that the cursor movement is handled by the underlying readline library, behaving exactly like physical keypresses.
Recording2.mp4
Closes #4748