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

More cursor keys #925

Closed
wants to merge 5 commits into from
Closed

Conversation

devycarol
Copy link
Contributor

@devycarol devycarol commented Jun 28, 2024

This patch implements toolbar keys for the following previously-reserved codes:

  • Page up/down (just didn't have buttons)
  • Page start/end

It also adds a new pair: word left/right.

Other changes:

  • Word left/right replace line start/end for the toolbar long-clicks of the left & right arrows. I can revert this if you wish, or I could also try to implement long-press customization if it's not too settings-creepy (it is :P).
    • Line start/end is instead the long-press for word left/right, and page start/end is the long-press for the pagers. There's a sort of consistency to it, but it could also disrupt people's previous layouts.
  • The arrow keys are now set as repeatable for regular layouts. To test out the functionality of all these keys, here's a D-pad prototype that you can replace your numpad layout with FTTB: dpad_toy.json
    • Word left/right & page up/down are also repeatable as standalone keys
    • I'll note that currently, most text-editing keys like select-word, copy, etc. aren't guarded against key previews. The only reason it doesn't show up in that prototype is that the numpad is considered a "numeric" layout. If a navigation keypad is implemented, perhaps we could ignore this and guard against previews in that layout as well; or I could spin up with a more concise function for preview prevention.
      • TL;DR: previews are weird.

Remaining hiccups:

  • The delete key has a mechanism to prevent continued noise if it's not deleting characters. It would be best for polish if the repeating cursor keys also had this safeguard.
    • I just need to figure out a way to reliably detect if there are characters before/after the cursor, without fail regardless of how primitive the input field is.
    • This also raises the question of whether the arrow keys should have a safeguard against moving into different input fields—it can be disruptive in how it resets the keyboard layout. The worst of it is when they move focus to a view element that isn't an input field, which does happen on occasion.
    • But I also want to preserve the function of, say, using the vertical arrows to browse through previous commands in a terminal emulator. So it's a bit sticky.
  • The word left/right keys behave a bit unexpectedly:
    • They won't advance the cursor past the first/final instance of a word character.
    • They skip over space-separated non-word characters such as punctuations and emojis, which is contrary to how this movement is typically handled on PCs.
      • This is caused by how Android natively handles Ctrl+Left/Right (I've tried with a physical keyboard on 2 separate devices and gotten the same behavior). Fixing it may require a custom implementation, which may make it tricky to maintain 1:1 functionality as with a physical keyboard. Or would it be possible to fine-tune the way the physical keyboard handles it directly using this app?
  • The arrow keys have a 1.2x scale factor in the toolbar. This would also be beneficial in the regular keyboard.

Would appreciate the input of anybody with knowledge on these topics.


Progress on #357

@Helium314
Copy link
Owner

This PR appears to be small enough so I can handle it, but in the future: please avoid putting multiple things into a single PR.
Adding toolbar keys is completely unrelated to making arrow keys repeatable, and should be handled in separate PRs. (see also the contribution guidelines)

Word left/right replace line start/end for the toolbar long-clicks of the left & right arrows

I don't really care. The current long-press behavior was added by @codokie, so I'd follow their opinion here.

@devycarol
Copy link
Contributor Author

devycarol commented Jun 29, 2024

but in the future: please avoid putting multiple things into a single PR.

Do you mind PRs that aren't linked to an issue, then? I could open a feature request for each one of these changes, but I don't want to create more work with redundant write-ups. I'll always try my best to explain what's going on in these PRs, of course.

I'll also note that combining these features was slightly helpful since the changes to Key.java for the new keys depend on the repeating-arrow-key change, but I understand your gripe. I could open pull requests for branches-of-branches and mark them as dependent on other PRs, not sure how you'd feel about that.

I hope you'll pardon my naivety with this stuff, I'm still very inexperienced with git and code contributions :)

@Helium314
Copy link
Owner

Do you mind PRs that aren't linked to an issue, then?

I would say these are essentially liked to the text editing layout you're working on. No need to have a separate issue for each of those parts.
With smaller PRs you can probably also reduce the need for explanation, as it's easier for me to see what's going on.

I could open pull requests for branches-of-branches and mark them as dependent on other PRs, not sure how you'd feel about that.

I'd prefer just having one PR after another. I see that with the Key changes you need both, so you can't really submit 2 completely separate PRs. You could merge the main branch though after the first PR is merged.

@devycarol
Copy link
Contributor Author

Alrighty, that's what I'll do moving forward. The forward/word-delete keys branch I'm working on is based on this one.

@Helium314
Copy link
Owner

I just need to figure out a way to reliably detect if there are characters before/after the cursor, without fail regardless of how primitive the input field is.

Basically you can't. Terminal emulators and a few other things don't tell you the text. It's possible to guess some text because what you entered is cached, but this may not be the correct text in some specific cases.
For "normal" text fields you can use RichInputConnection to get the text before / after cursor.
If this ends up being complicated, I recommend to move this to a separate PR.

This also raises the question of whether the arrow keys should have a safeguard against moving into different input fields

Is this about up/down arrows? If yes, then it should not be relevant for this PR, because except for repeating they do the same as before.

The word left/right keys behave a bit unexpectedly

I don't think you can simply fine-tune this behavior. To me it doesn't seem that bad actually.

In any case I would recommend to use the simle approach first, as it's very easy to make things overly complicated and then end up with a ton of weird edge cases.
A later PR can still be done if users complain, or if your really dislike the default behavior (hint: there is always someone who doesn't like whatever behavior is implemented).

The arrow keys have a 1.2x scale factor in the toolbar. This would also be beneficial in the regular keyboard.

Possibly the drawable could be adjusted, not sure what it's used for other than the toolbar keys.

@devycarol
Copy link
Contributor Author

devycarol commented Jun 29, 2024

Arrow keys are all rotations of arrow_left. The material/rounded shift key icons and a few others are also derived from the arrows. I could do a PR that creates new drawables dpad_[direction] that are just an upscale of the arrows, while preserving the others.

I've tried that, with difficulties. I'm not sure if/how you can make <scale> drawables at minSdk 21.

Basically you can't.

You can approximate it for the lefters (word-left, up, etc.) with the canDeleteCharacters method. It'll false-positive in cases where the left/up arrow would leave the input field, but this is negligible since it'd only be used to prevent audio clicks FTTB. It's an unexpected edge-case anyways, so it doesn't need that fine-grain of polish.

It's more complicated for the righters. A parallel "canForwardDeleteCharacters" (which will be helpful later) can be implemented by testing if the text after the cursor is empty, but it's less reliable than its counterpart since, as you say, you don't always have access to the full text.

Strange how mExpectedSelStart seems reliable in terminals—and you just compare it to zero. No reliable way to compare it against the total length of a text, right?

@Helium314
Copy link
Owner

Arrow keys are all rotations of arrow_left. The material/rounded shift key icons and a few others are also derived from the arrows. I could do a PR that creates new drawables dpad_[direction] that are just an upscale of the arrows, while preserving the others.

I've tried that, with difficulties. I'm not sure if/how you can make <scale> drawables at minSdk 21.

This is imo pretty horrible to handle in Android.
My best approach was using insets, e.g. ic_settings_advanced. You can also set negative insets

Strange how mExpectedSelStart seems reliable in terminals—and you just compare it to zero. No reliable way to compare it against the total length of a text, right?

As far as I remember it only works if you typed the text and do not close the keyboard between typing and checking.
If you keep the keyboard open, don't switch apps, and the terminal does not in some way change the text, it's reliable.
Does it also work in combination with up/down keys?

Since terminal emulators usually don't report information about the existing text, I don't think you can get information on how many characters are available to the right.

But you're right,when it's just about haptic/audio feedback it's fine when it doesn't work correctly in some special cases.

@devycarol
Copy link
Contributor Author

devycarol commented Jun 29, 2024

Yeah, since it's just for feedback the methods can be recycled for the arrow keys. The arrows want to repeat sound when there are characters to the right/left of the cursor. The vertical arrows will move the cursor to the start/end of the input field before exiting it, so the same checks as the horizontals can be used and it should be consistent.

The canForwardDeleteCharacters check should only fail slightly more often than its counterpart, and only in those primitive cases.

@devycarol
Copy link
Contributor Author

Splitting this in two, to make things more legible.

@devycarol devycarol closed this Jun 29, 2024
@devycarol devycarol deleted the more-cursor-keys branch June 29, 2024 22:25
This was referenced Jun 29, 2024
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.

2 participants