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

scrolling #7

Closed
wants to merge 15 commits into from
Closed

scrolling #7

wants to merge 15 commits into from

Conversation

TheTrio
Copy link

@TheTrio TheTrio commented May 1, 2022

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 TheTrio changed the title implemented basic scrolling implemented basic scrolling #6 May 1, 2022
@jrnxf jrnxf linked an issue May 1, 2022 that may be closed by this pull request
@jrnxf jrnxf self-requested a review May 1, 2022 19:56
@TheTrio
Copy link
Author

TheTrio commented May 4, 2022

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.

@jrnxf
Copy link
Owner

jrnxf commented May 5, 2022

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!

@TheTrio
Copy link
Author

TheTrio commented May 28, 2022

hey @coloradocolby could you take a look at this?

@jrnxf
Copy link
Owner

jrnxf commented May 28, 2022

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

Copy link
Owner

@jrnxf jrnxf left a comment

Choose a reason for hiding this comment

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

Left a few comments!

src/main.rs Outdated Show resolved Hide resolved
src/thok.rs Outdated Show resolved Hide resolved
src/thok.rs Outdated Show resolved Hide resolved
src/ui.rs Outdated Show resolved Hide resolved
TheTrio and others added 2 commits May 29, 2022 03:58
Co-authored-by: Colby Thomas <coloradocolby@gmail.com>
@jrnxf
Copy link
Owner

jrnxf commented May 29, 2022

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!

I just realized that even when the number of characters is equal to the max length, the next word won't fit on that line. So there's no need to treat this case separately
@TheTrio
Copy link
Author

TheTrio commented May 29, 2022

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.

@jrnxf
Copy link
Owner

jrnxf commented May 30, 2022

@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! 🍻

@TheTrio
Copy link
Author

TheTrio commented May 31, 2022

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 🤷

@jrnxf
Copy link
Owner

jrnxf commented Jun 5, 2022

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!

@TheTrio
Copy link
Author

TheTrio commented Jun 5, 2022

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!

@jrnxf
Copy link
Owner

jrnxf commented Jan 7, 2023

@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 jrnxf changed the title implemented basic scrolling #6 scrolling #6 Jan 7, 2023
@TheTrio
Copy link
Author

TheTrio commented Jan 7, 2023

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.

@jrnxf jrnxf changed the title scrolling #6 scrolling Jan 7, 2023
@TheTrio
Copy link
Author

TheTrio commented Dec 16, 2023

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

@TheTrio TheTrio closed this Dec 16, 2023
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.

continually load new words to meet time limit / scroll lines
3 participants