-
Notifications
You must be signed in to change notification settings - Fork 217
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
Conversation
…der' in settings.
…heckbox is ticked/unticked.
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. |
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.
Looks good, thx! Once you address @storvik 's comment, then we can merge.
.../main/java/com/dessalines/thumbkey/ui/components/settings/lookandfeel/LookAndFeelActivity.kt
Outdated
Show resolved
Hide resolved
Haha one man's feature is another's bug. 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.
|
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 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 ? |
I'd rather have a sane default than over-configuring it, unless its absolutely necessary. |
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. 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. |
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? |
No... my code doesn't work with a large minimum swipe length, your logic broke it 😆. I forgot all about that setting!
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 😄. |
…r and improved backspace deadzone
… into slide-gestures
I've tested this and it seems to be working fine now. 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. |
i think the spacebar shouldn't have any deadzone, since the slide gesture completely replaces the original functionality. it would much worsen the ux. |
@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 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. |
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.
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! |
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. 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. |
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. I do dislike the deadzone, but I really like the backspace functionality and the spacebar cursor moving. |
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. 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. |
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. |
EDIT/UPDATE: 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. Will eventually fix #410 Someday this PR will be finish lmao. |
…n the spacebar and backspace key.
@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. |
The deadzone should come from the minSwipeLength.
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. |
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. EDIT: |
@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.
|
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) |
@sslater11 I agree with @dessalines, the deadzone should be dependent on min swipe length, so maybe we should remove 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 |
Well after reading "dessalines doesn't want too many settings", I went and added loads of settings 😆 . 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) 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. Once the defaults are set in the DB, it's trickier to change them. |
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!!! |
@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. |
Cool. In that case just resolve the conflicts and I'll take a look. |
…ion and deadzone enable/disable.
Adding threshold acceleration
@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 😭 |
@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: Then you should have one big mess of a merge conflict to enjoy 😆 |
Cool, I'll try that tomorrow. Thank you!!!! 😄. I was thinking I had to do it from this page, but using the terminal will be so much easier. |
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. |
Oh my goodness, that did take some time 😆 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. |
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. |
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.
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, |
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.
Great job on disabling this list!
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.
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.
Summary of changes
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 😄