thokr icon indicating copy to clipboard operation
thokr copied to clipboard

implemented basic scrolling #6

Open TheTrio opened this issue 2 years ago • 10 comments

What

This PR aims to address #6. It adds a scrolling functionality to the text field, so that the user isn't overwhelmed by having all the words displayed at once.

Why

Besides being a better user experience, rendering fewer words at a single time makes the application faster since there aren't as many characters to render/keep track of.

How

The difficult part was figuring out when the user is at the end of a line. I don't think there's a way to do this using rust-tui so I've had to do the math myself. This logic is present in the get_skip_count function. Once we have calculated the amount of characters to skip, the implementation is relatively straightforward.

Checklist

  • [ ] Documentation added
  • [ ] Tests added
  • [ ] No linting errors / broken tests
  • [ ] Merge ready

Once this is merged, I think it would be nice to increase the HORIZONTAL_MARGIN (dynamically) to center the text. It would result in a better UI in my opinion.

TheTrio avatar May 01 '22 19:05 TheTrio

Here's how it looks now

demo

Also, there are some restrictions presently and I think they make sense but I'd like to hear your opinion as well -

  1. Once you've typed a line correctly and have moved onto the next line, you CANNOT move back to the previous line.
  2. The program won't scroll until you have fixed all your mistakes in the frame(which presently spans 3 lines)

There's another issue which isn't really related but it might make sense to fix it before this gets merged - if you type a character other than a space, it isn't displayed in red(because there isn't anything to display). Earlier, it just meant that your accuracy would go down.

But with this, since you can't move past a frame until you correct all your mistakes, the space should be highlighted in some way to make it obvious to the user that they typed something else instead.

TheTrio avatar May 04 '22 21:05 TheTrio

Wow great progress! I'm out of town for a few days visiting family but will take a look at this when I get back home and settled in!

jrnxf avatar May 05 '22 12:05 jrnxf

hey @coloradocolby could you take a look at this?

TheTrio avatar May 28 '22 14:05 TheTrio

Yes — I will take a look today! Thank you for the reminder 🙏

jrnxf avatar May 28 '22 14:05 jrnxf

Also, there are some restrictions presently and I think they make sense but I'd like to hear your opinion as well -

  1. Once you've typed a line correctly and have moved onto the next line, you CANNOT move back to the previous line.
  2. The program won't scroll until you have fixed all your mistakes in the frame(which presently spans 3 lines)

There's another issue which isn't really related but it might make sense to fix it before this gets merged - if you type a character other than a space, it isn't displayed in red(because there isn't anything to display). Earlier, it just meant that your accuracy would go down.

But with this, since you can't move past a frame until you correct all your mistakes, the space should be highlighted in some way to make it obvious to the user that they typed something else instead.

So for both 1 and 2, I'd love to see these fixed if possible. I don't imagine it will happen all that often, but restricting a user from ever going back to the previous line feels buggy to me. Same deal with number 2, not everyone cares about getting a perfect accuracy result, so forcing them to type a line correctly is something I don't want to enforce.

As for typing an incorrect character on a space, I totally agree! This is a bug I'd like to fix for sure. One approach I hadn't considered that I saw recently in this typing tui is to underline incorrect characters in addition to coloring them red. This works because obviously you can't color a space, but you can if that space is underlined. I'll work on adding this separately though, so you don't have to fuss yourself with it!

Let me know if this all makes sense. I'll see if I can't slot out some time to pair on this as well if you'd like!

jrnxf avatar May 29 '22 03:05 jrnxf

So for both 1 and 2, I'd love to see these fixed if possible. I don't imagine it will happen all that often, but restricting a user from ever going back to the previous line feels buggy to me. Same deal with number 2, not everyone cares about getting a perfect accuracy result, so forcing them to type a line correctly is something I don't want to enforce.

This is how it works now -

demo

I have swapped out the Deque in favor of a Vector since we need to keep track of the lines even after the user is past them(because they can backspace into them again). I've also added a set which keeps tracks of the end points of each line to make it easier to know when the user has backspaced into the previous line.

Let me know if this all makes sense. I'll see if I can't slot out some time to pair on this as well if you'd like!

Great! I think I have a working version, but there might be some issues I haven't caught or considered.

TheTrio avatar May 29 '22 13:05 TheTrio

@TheTrio this is looking great! I don't have time today to give it another review, but it's definitely on my todo list to get this wrapped up and merged this week! Thanks again for all your hard work! 🍻

jrnxf avatar May 30 '22 19:05 jrnxf

So replacing the space with a dot might have unintended consequences since spaces are used to determine when to wrap lines.

error

(this is on main btw)

I think underlining is a better option :shrug:

TheTrio avatar May 31 '22 01:05 TheTrio

So replacing the space with a dot might have unintended consequences since spaces are used to determine when to wrap lines.

error error

(this is on main btw)

I think underlining is a better option 🤷

I decided against underlining because I personally felt it made the virtual cursor (which just underlines your current position) have less visual importance and get lost in the midst of any mistakes you make. It seems like we should be able to easily transition your logic to adjust for this. I can take a look myself!

jrnxf avatar Jun 05 '22 03:06 jrnxf

I decided against underlining because I personally felt it made the virtual cursor (which just underlines your current position) have less visual importance and get lost in the midst of any mistakes you make

Hmm I guess that's true.

It seems like we should be able to easily transition your logic to adjust for this. I can take a look myself!

Great!

TheTrio avatar Jun 05 '22 04:06 TheTrio

@TheTrio would you mind leaving some comments on some of the methods you made? I tried looking over this last night and struggled to make heads or tails on some of the methods. I'd definitely love to workshop this together and get this functionality added!

jrnxf avatar Jan 07 '23 04:01 jrnxf

Hi! I've added some comments. Hopefully makes more sense. Been a while since I touched this code.

I still believe the simplest way to fix this issue would be to not alter the space character - like I suggested earlier, perhaps an underline. There might be a way to get this feature working without that - its just that I can't think of any.

TheTrio avatar Jan 07 '23 15:01 TheTrio

Don't think there's much left to do here, closing. Thanks for all your help!

TheTrio avatar Dec 16 '23 14:12 TheTrio