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

Implemented advanced text functions #78

Merged
merged 4 commits into from Sep 9, 2019

Conversation

@kugiigi
Copy link
Contributor

commented Jun 16, 2019

Implementations

  • Added a new toolbar with functions such as copy, cut, and paste (available in cursor swipe mode)
  • Added a way to select texts in the cursor (double-tapping while in cursor swipe mode)
  • Added floating buttons to exit cursor swipe mode and place the cursor in the start/end of line/document (available in cursor swipe mode)

Changes/Fixes

  • Added audio and haptic feedback to the space key and word ribbon
  • Changed cursor swipe mode timeout from 400ms to 5000ms. This is to give users enough time for the new toolbar. The done button gives users a way to immediately exit the cursor swipe mode.
  • Makes background of the cursor swipe mode fully solid instead of 0.5 opacity.
  • Fixes issues with the cursor swipe mode in QtWebEngine text fields. This was done by commenting out input_method.surroundingRight and input_method.surroundingLeft from the criteria for moving the cursor. This however has an effect which now allows moving the cursor between lines with horizontal swipes. This needs to be revisited once the issue with QtWebEngine gets fixed.
  • Moved audio and haptic feedback components from KeyboardComponent.qml to Keyboard.qml and created a function that can be used to enable both feedback easily.

Screenshots

Ambiance Theme:

Cursor Swipe Mode
screenshot20190617_174058792

Selection Mode
screenshot20190617_174105031

Fixes #20

qml/Keyboard.qml Show resolved Hide resolved
qml/Keyboard.qml Show resolved Hide resolved
qml/Keyboard.qml Show resolved Hide resolved
src/view/abstracttexteditor.cpp Outdated Show resolved Hide resolved
@UniversalSuperBox
Copy link
Member

left a comment

Wow, that's quite a change. There's a lot going on here, so I think that reviews will find different points as they continue. Remember that it's not because we want to keep beating on you, but because we keep finding more stuff that we can fix to make the best keyboard around! :)

src/view/abstracttexteditor.cpp Outdated Show resolved Hide resolved
qml/Keyboard.qml Show resolved Hide resolved
qml/Keyboard.qml Show resolved Hide resolved
Kugi Eusebio Kugi Eusebio
@UniversalSuperBox

This comment has been minimized.

Copy link
Member

commented Jun 30, 2019

Unfortunately I think we'll need to wait to merge this until after OTA-10 is released so it can have more testing in the devel/rc channels. This is our fault, the release date has been a little up in the air as we've been working through issues on the edge channel. So, let's keep looking at and working through this up until the merge window opens again.

@kugiigi

This comment has been minimized.

Copy link
Contributor Author

commented Jun 30, 2019

Yeah I figured that would happen and I think it's actually better to do it that way.
This is quite a change and needs testing and perhaps someone else can improve it before landing.
Or maybe I can include my bottom edge gesture 😝

Or should we land this on edge first to get more inputs and opinions?

@UniversalSuperBox UniversalSuperBox added this to In progress in OTA-11 via automation Jul 3, 2019

@UniversalSuperBox

This comment has been minimized.

Copy link
Member

commented Jul 11, 2019

Nah, no need to bring it in on edge. It'll be fine on devel.

@myii

This comment has been minimized.

Copy link

commented Aug 6, 2019

I've been a very long-term user of these advanced text functions (since November 2018 or thereabouts), perhaps I've used it even more than @kugiigi himself! I'm planning to rebuild again at some point, using this PR as the basis. I'll post any feedback that I've got.

@myii

This comment has been minimized.

Copy link

commented Aug 7, 2019

Rebuilt again and it is looking and feeling more polished than ever, great job @kugiigi!

One thing I've noticed for now:

  1. When (left/backwards) scrolling across multiple lines of text, when it gets to the next line up, it has a habit of jumping back to the line it was already on -- you have to almost swipe diagonally upwards to keep it moving in the right direction. It applies (but not so pronounced) when swiping right/forwards as well. It could be the natural reach of the thumb when swiping but then that's always going to catch people out, so the interface should consider that.
@UniversalSuperBox

This comment has been minimized.

Copy link
Member

commented Aug 26, 2019

@myii, do you mean when releasing the held point? I'm finding that as well. When I release my thumb after a leftward swipe, the cursor moves down. Likely because of anatomy and stuff.

@kugiigi, this seems to be something we won't get around. What feels like a straight swipe left really isn't. Here's me, holding my Nexus 5 with only my right hand, doing a swipe from right to left. It really felt straight.

image

@myii

This comment has been minimized.

Copy link

commented Aug 26, 2019

@UniversalSuperBox My workaround (to get around this anatomical issue) is to keep my swipe motion along the very bottom edge. That avoids scrolling up or down a line by mistake. Not sure how this can be used to reach a solution, though.

@kugiigi

This comment has been minimized.

Copy link
Contributor Author

commented Aug 28, 2019

I am not sure if I really understand your observations but I think those behaviors are not impacted by my changes and perhaps worth having a separate issue to maybe improve in the future.

As for my changes, the real known behavioral change is that horizontal swipes will now move the cursor upwards or downwards when reaching the end of a line. This is just to make cursor movements work in QtWebEngine and should be changed again in the future.

@UniversalSuperBox

This comment has been minimized.

Copy link
Member

commented Sep 2, 2019

The thing is, with the current design people usually are near the bottom of the screen and make an effort not to move off the screen, so there's little vertical movement of the cursor. With this change it's possible to start a swipe further from the bottom of the screen and cause the cursor to jump between lines.

@kugiigi

This comment has been minimized.

Copy link
Contributor Author

commented Sep 2, 2019

Sorry but I don't see how the changes affect this specific use case. The trigger is still the same which is the space key. I haven't included yet the bottom swipe gesture 😁
Maybe you are specifically pertaining to selection mode?

And personally, I don't encounter this issue as often to the point that it's counter-intuitive and annoying. But perhaps I have smaller fingers and might have used the feature for so long that I kinda mastered it already 😄

In any case, suggestions are welcome to improve the cursor mover. On top of my head, maybe we can lower the sensitivity of vertical movements. At the moment, it is already lower the horizontal but making it lower might lessen the accidental vertical movements.

@UniversalSuperBox

This comment has been minimized.

Copy link
Member

commented Sep 2, 2019

To summarize what we've found, your thumb curves down as you move left (assuming you're right handed). That's not unexpected, with the thumb's range of motion. This occurs more if you start your gesture further away from the bottom of the screen, which you can do once you've activated cursor moving mode.

Maybe making the vertical movement less sensitive would help, but I think the problem will still happen at different screen sizes. It's hard to draw straight lines.

Instead, maybe it'd make more sense to have distinct "move left/right" and "move up/down" areas (like old touchpads), or to find out how Flickables reject off-axis movement and steal that. As it is, anatomy makes this difficult to interact with.

@UniversalSuperBox UniversalSuperBox self-requested a review Sep 9, 2019

@UniversalSuperBox
Copy link
Member

left a comment

Okay, I think we've done enough deliberation. Let's get some wider feedback.

@UniversalSuperBox UniversalSuperBox merged commit 1e4bc26 into ubports:xenial Sep 9, 2019

1 check passed

continuous-integration/jenkins/pr-merge This commit looks good
Details

OTA-11 automation moved this from In progress to QA Sep 9, 2019

@Fuseteam

This comment has been minimized.

Copy link
Contributor

commented Sep 9, 2019

yay

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
4 participants
You can’t perform that action at this time.