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

Backspace swipe right to delete word and Slide gestures improvements #439

Merged
merged 24 commits into from
Oct 20, 2023

Conversation

sslater11
Copy link
Contributor

@sslater11 sslater11 commented Sep 23, 2023

Summary of changes

  • Right swipe on backspace deletes a word to the right.
  • Better 'swipe selection' toggle and better 'swipe delete' toggle.
  • Added up and down swipes to spacebar to move cursor up and down.
  • The Copy/Cut actions will now select all and copy/cut if there is nothing currently selected.

The text selection is far too trigger happy when we have a low slide sensitivity number.
I've changed it so that text selection will only be triggered when we go up to around the bottom line of the middle key. The backspace key also has the same distance applied to it before it's triggered.

@dessalines I know in my previous PR I wanted to make slide gestures enabled by default, but I don't have time to figure out the SQL at the moment. Please look to merge this if it all looks good to you 😄

@storvik
Copy link
Contributor

storvik commented Sep 24, 2023

Fixed the bug where you could swipe down to enable text selection. Now you must swipe up to enter selection mode.

I actually did this on purpose. This way it is possible to swipe down instead of up if you are using Thumb Key with bottom offset.

Copy link
Owner

@dessalines dessalines left a comment

Choose a reason for hiding this comment

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

Looks good, thx! Once you address @storvik 's comment, then we can merge.

@sslater11
Copy link
Contributor Author

sslater11 commented Sep 26, 2023

Haha one man's feature is another's bug.
Should I just change the offset to allow up and down(using the abs() function again) to trigger the selection?
They type/split keyboards have spacebars in the center of the keyboard, so I can see how swiping down could be natural for them too.

The distance I've set is 1.25x key length upwards to activate text selection. Should it be the same for downwards? On my phone I can trigger text selection by reaching the bottom of my screen as there's the home, back and app switch buttons to push the keyboard up higher. This only works when I swipe downwards from the very top of the spacebar though.

EDIT: @storvik Any idea why adding slide gestures to the typesplit keyboard disables the use of normal swipes? Notice we can't swipe up for settings or o Swiping works correctly on the normal spacebar and backspace key. Swiping works on the backspace key on the typesplit layout.
I added this line to SPACEBAR_TYPESPLIT_TOP_KEY_ITEM IN CommonKeys.kt:
"slideType = SlideType.MOVE_CURSOR," after the swipeNWay.FOUR_WAY_CROSS
It now looks like this. Slide gestures work perfectly, but swipes don't which is weird.

val SPACEBAR_TYPESPLIT_TOP_KEY_ITEM =
    KeyItemC(
        center = KeyC(
            display = KeyDisplay.TextDisplay(" "),
            action = KeyAction.CommitText(" "),
        ),
        swipeType = SwipeNWay.FOUR_WAY_CROSS,
        slideType = SlideType.MOVE_CURSOR,
        swipes = mapOf(

@storvik
Copy link
Contributor

storvik commented Oct 3, 2023

Sorry for the late reply @sslater11, I've been very busy lately!

I think the distance to trigger selection should be the same upwards and downwards. Personally I have an offset to the bottom, as this feels more natural and I don't have to reach for the bottom keys with my thumb. When using an bottom offset there's no problem sliding down to activate selection.

The issue with the typesplit keyboard is strange. This should work the same way as the delete key, where I can slide to mark-delete backwards, and swipe to delete word. Does this work for you?

EDIT: I think I understand what's going on. Seems like the issue might be that the if statement in KeyboardKey.kt (line 283) will be false, and action is not set properly. One solution is to add a boolean cursorHaveMoved, which is set to true whenever slide distance is larger than slide sensitivity. And then add || !cursorHaveMoved to the if statement. Remember to reset the variable around line 365. This is pretty much how it's solved with the delete key. Do you understand or did I explain myself poorly?

EDIT 2: Maybe the value of 1.25 should be configurable? When it's too small it's hard to get it to differentiate between slide and swipe. This may not be an issue though, opinions @sslater11 or @dessalines ?

@dessalines
Copy link
Owner

I'd rather have a sane default than over-configuring it, unless its absolutely necessary.

@sslater11
Copy link
Contributor Author

sslater11 commented Oct 5, 2023

Thanks Storvik, I asked before I managed to dive into the code and I came to a similar conclusion as you and have it working just like the backspace key works.

I've added an X offset trigger to the cursor moving, and this allows us to use the spacebar for swipes as well.
Both delete key and spacebar now share an X offset of "keySizeDp.toPx() * 0.75". The reason why I've chose 0.75, is because when we do a normal swipe, we're in the middle of the key and swiping a short length. For me 0.75 seems to be working well.

I'm going to give it a few days of testing to make sure everything is working fine before I do another commit.

You mentioned the number 1.25, this is for triggering selection mode. I've use 0.75 for triggering cursor moving and backspace selection-delete.

For triggering selection mode, I chose "1.25 * keySizeDp.toPx()" because this takes our thumb up to roughly the middle key's bottom line, which is easier to remember to go to roughly the middle of the keyboard and to also explain to new users.

Oh I've also added a swipe up and down to the normal spacebar using normal swipes, not gesture swipes. I was really missing the ability to go up and down a line.

I can fix the merge conflict as well. I know why it happened and it's an easy fix :). The settings got refactored and I just need to move my 2 small changes to it's new location.

@storvik
Copy link
Contributor

storvik commented Oct 5, 2023

Glad you figured it out @sslater11 !

Just one thought: Should minimum swipe length be a part of the equation here? Ex, min swipe length + some sane offset. Does it still work if you set min swipe length to max value?

@sslater11
Copy link
Contributor Author

No... my code doesn't work with a large minimum swipe length, your logic broke it 😆. I forgot all about that setting!
We could possibly add the minimum swipe length to the offset I've settled on.

val slideOffsetTrigger = (keySizeDp.toPx() * 0.75) + minSwipeLength
val slideSelectionOffsetTrigger = (keySizeDp.toPx() * 1.25) + minSwipeLength

I've tested it and these work alright, but with a large minSwipeLength of 200, slide gestures on the spacebar or backspace key are triggered really close to the swipe length, because the swipe length takes me a full key away(I use the default key size of 64). I don't think this would be a problem as I doubt anyone is using those higher numbers, and if they are, they've probably made the key size larger, giving more distance.

Thank you for noticing that I'd not accounted for the minimum swipe length 😄.

@sslater11
Copy link
Contributor Author

I've tested this and it seems to be working fine now.
Please test yourself, as you may not like the changes!
The spacebar cursor sliding gesture has a deadzone to allow for normal swipe actions, this does however mean the slide gesture doesn't feel as smooth to start with.
We could possibly make it so that there's 2 different kind of slide gesture modes, one which allows normal swipes, and one that doesn't.

I do still need to add changes to the settings file, because of the recent refactor my changes were lost.I can either update my branch and do a final commit, or do another PR for it. I'm not sure whether updating my branch will cause any issues for you.

@KraXen72
Copy link
Contributor

KraXen72 commented Oct 8, 2023

i think the spacebar shouldn't have any deadzone, since the slide gesture completely replaces the original functionality. it would much worsen the ux.

@sslater11
Copy link
Contributor Author

@dessalines please merge WadeWT's Key Customization before this as I can easily fix any merge conflicts for you 😄

@KraXen72 What are your thoughts on the deadzone on the backspace key? It's already there in the current version of thumbkey, you can swipe left to delete a whole word or a longer swipe to select and delete text.

After using my code, I do dislike the spacebar deadzone, but it is actually pretty handy. I can move the cursor quickly to roughly where I want, then I can swipe to move the cursor by 1 character up, down, left, and right.
The backspace deadzone is great though, I use both the swipe to delete whole words and slide gesture delete.

The reason why I added swipes for the spacebar is because of the Type-split layout, so this functionality is needed, but whether it's in all layouts or just type-split is up to all of you.

@storvik
Copy link
Contributor

storvik commented Oct 9, 2023

Just my two cents. I do not like deadzone for the spacebar, just like I didn't like it for the backspace key. I did however get used to it for backspace key, and I see how it's handy with swipe actions on the spacebar when it comes to swipe for line up / down etc. Personally I have min swipe length set pretty low. I figured that I would type faster if I got used to small swipes.

Thank you for noticing that I'd not accounted for the minimum swipe length

I was aware of this issue with the backspace key, I'm addressing it for the spacebar because in my opinion the deadzone is a bigger "issue" when moving the cursor.

Maybe swipe actions could be configurable? Move by character, move by word, etc.

I will try to get the time to test your code later today!

@KraXen72
Copy link
Contributor

KraXen72 commented Oct 9, 2023

i see how the spacebar deadzone could be useful, however, i don't use the up/down swipes on spacebar, and my proposed solution to the issue of not being able to reliably swipe by 1 character is #410 , not a deadzone. I would have to test it for myself to see.
the deadzone on backspace is fine, i like the gesture, but i don't think it should be a coefficient of the slide sensitivity, rather a constant. Until #410 or a different solution is implemented, i have to set the slide sens pretty high (38), which makes the backspace gesture trigger very late.

Could you create a test apk for the spacebar deadzone? or can i somewhere find one (like a ci pipeline artifact)? it'd be good if it had a different package name, since i don't want to install it over normal thumb key, because settings import/export is not yet implemented.

@sslater11
Copy link
Contributor Author

sslater11 commented Oct 9, 2023

Here's a test version. I changed the package name, so you can have it installed along with your normal thumbkey. Both have the same app name though, so maybe change the colour to know the difference haha.
https://drive.google.com/file/d/1YGUvnF4ynyjiY2Vrm0hH-DKLHnNJw3tk/view?usp=sharing

I do dislike the deadzone, but I really like the backspace functionality and the spacebar cursor moving.
I think spacebar acceleration would be great! I'd still miss being able to move the cursor up and down though haha.

@storvik
Copy link
Contributor

storvik commented Oct 9, 2023

I've been playing around with this version today. Everything seems to be working great!

Regarding the deadzone I've found it hard to find a generic expression that works nicely despite the magnitude of min swipe length. I tried to use a factor of min swipe length, but that wasn't too flexible either.

Is it possible to change min swipe length completely? For instance we could use a range slider instead of a normal slider. This way the user can set the "swipe" area, and everything beyond this area is considered a slide.

Screenshot 2023-10-09 220824

Maybe this is just making things more complicated than necessary, not sure. It's hard to keep things simple and easy to configure, but at the same time flexible.

@KraXen72
Copy link
Contributor

KraXen72 commented Oct 9, 2023

it's kind of hard to say, to be honest. i don't necessarily hate it, but i think it would be much better with #410 being implemented and that range slider conf like you suggested.

@sslater11
Copy link
Contributor Author

sslater11 commented Oct 10, 2023

EDIT/UPDATE:
Accelerating cursor.

I've pushed this update to this PR.

I have it working with text selection and the backspace selection delete. It works really well. It's working with the deadzone that I've added to the spacebar and backspace key, so normal swipes are preserved.
Tomorrow I'll look to get it working without any deadzone, and make it so the keyboard layout designer can choose between a spacebar with or without normal swipes. I'll do the same for the backspace key.
The slide gesture sensitivity setting no longer works because it has been replaced with the acceleration. I'd like to make a settings slider to configure the acceleration, but am looking for extra help with that one to make a nice acceleration curve(I'm using a basic quadratic formula).

Will eventually fix #410
accelerating_cursor

Someday this PR will be finish lmao.

@KraXen72
Copy link
Contributor

@sslater11 , please, for #480 , make it configurable/for some layouts only. it was like this in the past but got removed from the default layouts (#195), because it happened often than people would accidentally swipe slightly up in an app that doesen't have multi-line text input (any messaging app, or even when you're writing a single line), and it would focus some ui button outside of the ui element. It can be useful but shouldn't be the default in my opinion, since it already got removed once.

also,the english programmer layout already can move lines up and down, just not with slide gestures on.

@dessalines
Copy link
Owner

2

The deadzone should come from the minSwipeLength.

3

I'd really like it to be a sane default if possible, but still be affected by the already existing sensitivity setting.

Lets not overconfigure this, lets just get something working with existing settings, then tweak it with better acceleration functions in the future.

@sslater11
Copy link
Contributor Author

sslater11 commented Oct 16, 2023

Should users or layout designers be the ones to decide whether to have swipes with slide gestures? I think it's best if users have the choice.

I've wrote the code to toggle the spacebar/backspace deadzone in the settings(one checkbox each), just waiting to hear all of your views.
If we give the user the power, then it makes the keyboard more flexible, but it does mean the user can break some layouts. The type-split layouts require swiping with slide gestures, because the fullstop and comma are on the spacebars. Maybe we could make a popup to warn the user they need to enable swipes?

EDIT:
What do you think the defaults should be? Deadzone enabled or disabled? I prefer it to be disabled, since slide gestures would feel smoother. I'm not certain how I'll use this yet, gonna see how I get on with no deadzone, but I'm expecting to want the deadzone on the backspace for deleting words..

@sslater11
Copy link
Contributor Author

@KraXen72 regarding swiping up/down on the spacebar to have a settings button to toggle it. I can add it to the behaviour settings. I think swiping up/down should be enabled by default, since it's a natural thing to expect. Whilst typing this, I was going to say I've not accidentally triggered it, but I have recently had the same error, where the cursor jumps to the beginning of the text box. I wasn't sure how the error was occuring lol.

@storvik

  1. Yes, slide sensitivity is a dead variable at this point, We could still use it if I implement a choice for the user to enable/disable cursor acceleration.
  2. I guess we could implement a range slider, but I don't really see a need to as I think the values are good as they are. I think it's something to work on after this PR has been merged, because it's starting to get too old now and already had 2 merge conflicts 😆.
  3. As stated in number 1, I could make a settings for the user to choose whether they want to slide with acceleration on/off and to then display the relevant sliders.
    The idea with acceleration is to add a slider for changing how fast/slow it moves. @WadeWT has been working on a better acceleration curve, and I can make a slider for it now. The acceleration is good, but the accuracy at slow speeds isn't so it's still a work in progress to find the right curve and numbers.

@KraXen72
Copy link
Contributor

  • @sslater11 Regarding slide sensitivity, i agree that once acceleration get's implemented, it's useless.
    • If user turns off acceleration, a default slide sensitivity (not configurable) of 13/14 should generally make it so the cursor is directly above the finger when swiping (1:1 ratio), or 26/28 for a 2:1 ratio (tested with default font size, obviously will differ for larger/smaller text).
    • This should be a sensible default. Current slide sensitivity slider would be the best solution, but introduces unnecessary complexity once accel is added.
  • regarding acceleration and deadzone:
    • I've gone camp deadzone, even if it takes getting used to. To preserve useful & fequently used actions of moving a single character/line back/forth, there obviously should be some deadzone. This deadzone may be configurable if required, but i think a sensible default should do too.
    • new idea: once you go past the deadzone, the cursor should "catch up" for all the swiping you've already done. That way it only lags behind in response time, and not distance (lagging in distance is imo more annoying)
    • if the acceleration is tuned more or less so that in "normal speed" (what is normal speed anyway lol) swiping roughly makes the cursor appear above your finger when you go past deadzone, i think that could be good ux.

My reasoning is that when you'd drag the cursor by the native android handle, you obviously want the cursor to be where your finger is. The spacebar should try to emulate this, even though it's not completely possible (different font sizes and letter widths etc), with some extra niceities like flicking the spacebar to jump long distances (like to the start of a url). i think the "catching up" of the cursor could help with this.

what do you think about this, @sslater11 & @storvik ? (also anyone else, too)

@storvik
Copy link
Contributor

storvik commented Oct 16, 2023

@sslater11 I agree with @dessalines, the deadzone should be dependent on min swipe length, so maybe we should remove keySize.dp.toPx() from the equation?

Regarding the sensitivity it probably should set the sensitivity, but I think it's ok to merge this and figure it out later. Not sure, but maybe we should just remove the slider while figuring stuff out? Uncomment it for example.

@dessalines wants to avoid more settings, that's ok for me. We can always reevaluate based on feedback later.

My suggestion is to find an equation that is more dependent on min swipe length. Maybe minSwipeLengh x 4? Fix merge conflicts and get this merged. 🎉

@sslater11
Copy link
Contributor Author

Well after reading "dessalines doesn't want too many settings", I went and added loads of settings 😆 .
There's lots of opinions about how this should work, so I think having the ability to control it yourself is just better.

There's 3 slide cursor movement modes. Experimental acceleration, Quadratic acceleration(which you've all probably been using) and Constant speed(works the same way before acceleration was added)
The Slide Sensitivity slider now works for all 3 modes.

thumbkey_settings_1
thumbkey_settings_2

I need to tidy the code, then settle on some sane defaults and finally commit and that should be us! The rest can be figured out later haha.
I think it's best to enable the spacebar and backspace deadzones, because they allow support for other layouts that have swipes on the spacebar. Maybe later in life we can add a warning to the user that they've disabled the deadzone, but their layout requires it.

Once the defaults are set in the DB, it's trickier to change them.
I think the defaults should be
Cursor movement mode: Experimental acceleration
Slider default is already 9, easier to leave it at that.
spacebar deadzone: enabled
backspace deadzone: enabled

@KraXen72
Copy link
Contributor

looks awesome!!! how does experimental acceleration differ from quadratic? it would be good to probably pick a better name for it, since you want to make it the default. but otherwise, can't wait!!!

@sslater11
Copy link
Contributor Author

sslater11 commented Oct 18, 2023

@storvik I see what you're saying now, yeah you're right, the key size shouldn't affect the deadzone. I am however going to leave the code as it is for now. I tried to change it to "2 * minSwipeLength", but it's way too trigger happy on the default settings. Also, when we get to a really long swipe length, our thumb goes off the keyboard and we can't trigger slide selection. Using they key size allows us to limit the distance.
I'm happy to see this changed to something that works just as well, but for now, it's working and I don't want to play around with numbers to get it perfect, because it seems pretty good at the moment.

@dessalines
Copy link
Owner

Cool. In that case just resolve the conflicts and I'll take a look.

@sslater11
Copy link
Contributor Author

@dessalines please be aware of the DB migration commit :).

This is now ready to merge after we fix the merge conflicts. For some reason my git says that I can't resolve the conflicts due to not having write access 😭
I'd be happy to look at the merge conflicts and save them to a file to then send it over to you?

@storvik
Copy link
Contributor

storvik commented Oct 19, 2023

@sslater11 this sounds strange. Not sure how you work, but you should be able to add upstream remote and resolve merge conflicts without issues with write access. What I do:

Add upstream remote: git remote add upstream git@github.com:dessalines/thumb-key.git
Fetch upstream: git fetch upstream
Merge upstream/main into your branch (slide-gestures): git merge upstream/main

Then you should have one big mess of a merge conflict to enjoy 😆

@sslater11
Copy link
Contributor Author

Cool, I'll try that tomorrow. Thank you!!!! 😄.
Yeah I can imagine it will be a big mess lmao. Oh the fun I'm going to have! 😆

I was thinking I had to do it from this page, but using the terminal will be so much easier.

@dessalines
Copy link
Owner

Merge hell is my worst nightmare, I've run into some terrible ones in my day. I highly recommend using meld, its the only one I've found that's decent.

@sslater11
Copy link
Contributor Author

sslater11 commented Oct 20, 2023

Oh my goodness, that did take some time 😆
I've used meld before, and I definitely needed it for this! I've been using github's merge with the >>>> ===== <<<<<< symbols, and that's dam hard. So happy that I can do it locally and use meld! Thanks Storvik and Dessalines, I would have been stuck without you two!

Dessalines, please merge Wade's "theme colour refactor" PR before mine and I will fix any merge conflicts, if there are any. I think that would be the easiest way for everyone as we've both edited the same files in multiple places. If you do need me to do another merge, please PM me on Element, or in the dev chatroom as github doesn't send me many notifications :).

EDIT: Oh I forgot to say, after this I will open another PR to get type-split layouts working with slide gestures(one line of code in CommonKeys.kt), along with adding the emoji key to all the type-split layouts.

@dessalines
Copy link
Owner

dessalines commented Oct 20, 2023

Its good ettiquete to try to merge conflict-ridden PRs in order, so I'll do yours first: I don't wan't you to keep getting trapped in merge hell :)

I'm testing this out now.

Copy link
Owner

@dessalines dessalines left a comment

Choose a reason for hiding this comment

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

Everything that I tested worked well, thx!

I'll get this merged since its been open so long, and if anyone finds issues later, please open them up and we'll handle them individually.

state = minSwipeLengthState,
SettingsList(
state = slideCursorMovementModeState,
enabled = slideEnabledState.value,
Copy link
Owner

Choose a reason for hiding this comment

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

Great job on disabling this list!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Woooo! finally closed it haha.
Yeah I was thinking of opening an issue following this, since there's lots of opinions and changes to make, which we can tackle one by one.
And well done to github for not telling me it's been merged, even though I have my emails set to do so lol.

@dessalines dessalines merged commit a14f7ab into dessalines:main Oct 20, 2023
1 check passed
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.

5 participants