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

new pinned toolbar & history view keys long press functions #691

Merged
merged 10 commits into from
May 1, 2024

Conversation

codokie
Copy link
Contributor

@codokie codokie commented Apr 13, 2024

Fixes #686
I added new functions on long press (credit to @Roccobot for the idea):

Key Tap Tap + Hold
Move cursor left Move cursor to the beginning of the line
Move cursor right Move cursor to the end of the line
Move cursor up Move cursor page up
Move cursor down Move cursor page down
Copy selection Copy all
Undo Redo
Redo Undo
Select word Select all

The history view now has Up, Down, Undo, and One Handed Mode (fixes #513) keys to fill the blank space, and the long press functions are available there as well.
The Select All key has been removed to preserve space and because it is now possible to select all by long pressing the select word (you can also long press the copy key to select all + copy).
The "clear clipboard" key has been moved to avoid pressing it by accident when attempting to press the "close history" key, and in order to have the Cut key closer to the Select button.

Here are old history view keys:
old

and here is the new key layout:
new

I know there are plans to make the history view customizable in the future, but in the meantime I believe this should be sufficient for most people.

On a side note, the new copyAllText() that is triggered when long pressing the Copy key may copy up to Integer.MAX_VALUE * 4 bytes to memory which is a lot.
Is there a value that is more careful and still makes sense to achieve Copy All functionality ?

Note that the old copyText() used for Copy key (short press) may copy up to Integer.MAX_VALUE * 2 bytes to memory, which can also lead to memory issues if for some reason the current input field has a lot of text

@Roccobot

This comment was marked as off-topic.

@Helium314
Copy link
Owner

This is again mixing a bunch of small and independent changes, making review harder... I updated the issue templates to hopefully make more clear that I dislike it.

Other than that, I guess it's mostly fine.
Some issues with your code (I haven't looked through everything:

  • The code for determining the keycode on long pressing a toolbar key is essentially duplicated. The keycode should be determined in a similar way as the normal code.
  • You add keycodes above the heliboard only codes section in KeyCode, with number ranges that are also used in FlorisBoard. Since these codes may get added in FlorisBoard too, but likely with different actions this is prone to breaking keyboard layout compatibility.

const val NOT_SPECIFIED = -10008 // todo: not sure if there is need to have the "old" unspecified keyCode different, just test it and maybe merge
const val NOT_SPECIFIED = -10010 // todo: not sure if there is need to have the "old" unspecified keyCode different, just test it and maybe merge
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually there is no need to have the NOT_SPECIFIED code last, you can also just leave it at -10008.
Though it would only be relevant if someone would use this code in a layout, which is quite unlikely.

@Helium314
Copy link
Owner

I agree the code seemed repetitive, so I've moved that code to a new function for better maintainability

Thank, though I think there might have been a little misunderstanding.
When I wrote "The keycode should be determined in a similar way as the normal code." I actually meant to have something like ToolbarUtils.getLongpressCodeForToolbarKey analogous to ToolbarUtils.getCodeForToolbarKey (the function name admittedly sounds awkward and could be better).
Sorry I wasn't clear enough about this.

I can revert the change to the clipboard view but it is not a major one and it partly depends on the changes of the long click functions.

No, I think it should be fine. Especially because it's all in a separate commit.

android:layout_height="0dp"
android:layout_weight="1" />
</LinearLayout>
android:visibility="gone"
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you add android:visibility="gone"? Is it necessary now for some reason?

@Helium314
Copy link
Owner

Code looks good, I hope tomorrow I finally have time for an actual test.

@Helium314
Copy link
Owner

The PR mostly does what it intends to. I find undo / redo on the same key rather weird, but i can imagine it's really convenient 👍

I found two issues though:
Pasting by long pressing the clipboard key replaces the underlined word, which is not the case without this PR.
Long pressing the copy key crashes in HeliBoard language settings text field, for any non-empty text:

java.lang.NegativeArraySizeException: -2
at dalvik.system.VMRuntime.newUnpaddedArray(Native Method)
at com.android.internal.util.ArrayUtils.newUnpaddedCharArray(ArrayUtils.java:51)
at android.text.SpannableStringBuilder.<init>(SpannableStringBuilder.java:63)
at androidx.emoji2.text.SpannableBuilder.<init>(SpannableBuilder.java:86)
at androidx.emoji2.text.SpannableBuilder.subSequence(SpannableBuilder.java:125)
at android.view.inputmethod.BaseInputConnection.getTextAfterCursor(BaseInputConnection.java:561)
at android.view.inputmethod.InputConnectionWrapper.getTextAfterCursor(InputConnectionWrapper.java:88)
at com.android.internal.view.IInputConnectionWrapper.executeMessage(IInputConnectionWrapper.java:246)
at com.android.internal.view.IInputConnectionWrapper.dispatchMessage(IInputConnectionWrapper.java:225)
at com.android.internal.view.IInputConnectionWrapper.getTextAfterCursor(IInputConnectionWrapper.java:117)
at com.android.internal.view.InputConnectionWrapper.getTextAfterCursor(InputConnectionWrapper.java:260)
at helium314.keyboard.latin.RichInputConnection.getTextAfterCursorAndDetectLaggyConnection(RichInputConnection.java:466)
at helium314.keyboard.latin.RichInputConnection.getTextAfterCursor(RichInputConnection.java:453)
at helium314.keyboard.latin.RichInputConnection.copyAllText(RichInputConnection.java:675)
at helium314.keyboard.latin.inputlogic.InputLogic.handleFunctionalEvent(InputLogic.java:733)
at helium314.keyboard.latin.inputlogic.InputLogic.onCodeInput(InputLogic.java:477)
at helium314.keyboard.latin.LatinIME.onEvent(LatinIME.java:1487)
at helium314.keyboard.latin.LatinIME.onCodeInput(LatinIME.java:1477)
at helium314.keyboard.latin.suggestions.SuggestionStripView.onLongClickToolKey(SuggestionStripView.java:380)
at helium314.keyboard.latin.suggestions.SuggestionStripView.onLongClick(SuggestionStripView.java:367)
at android.view.View.performLongClickInternal(View.java:6677)
at android.view.View.performLongClick(View.java:6635)
at android.view.View.performLongClick(View.java:6653)
at android.view.View$CheckForLongPress.run(View.java:25871)
at android.os.Handler.handleCallback(Handler.java:873)
at android.os.Handler.dispatchMessage(Handler.java:99)
at android.os.Looper.loop(Looper.java:193)
at android.app.ActivityThread.main(ActivityThread.java:6718)
at java.lang.reflect.Method.invoke(Native Method)
at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:491)
at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:858)

may copy up to Integer.MAX_VALUE * 2 bytes to memory

I would not care about this. If you already get that many characters into a text field, you probably need to have so much memory that you can spare those 4 GB anyway.

@@ -1524,6 +1524,7 @@ public void onTextInput(final String rawText) {
mInputLogic.onTextInput(mSettings.getCurrent(), event,
mKeyboardSwitcher.getKeyboardShiftMode(), mHandler);
updateStateAfterInputTransaction(completeInputTransaction);
mInputLogic.restartSuggestionsOnWordTouchedByCursor(mSettings.getCurrent(), mKeyboardSwitcher.getCurrentKeyboardScript());
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, that's a nice improvement!

@Helium314
Copy link
Owner

Everything working now!

@Helium314 Helium314 merged commit 83fc10f into Helium314:main May 1, 2024
1 check failed
Helium314 added a commit that referenced this pull request May 1, 2024
@codokie codokie deleted the long-press-functions branch May 31, 2024 22:02
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.

Long press on toolbar icons
3 participants