-
Notifications
You must be signed in to change notification settings - Fork 18
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
scrolling #7
Conversation
Here's how it looks now Also, there are some restrictions presently and I think they make sense but I'd like to hear your opinion as well -
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. |
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! |
hey @coloradocolby could you take a look at this? |
Yes — I will take a look today! Thank you for the reminder 🙏 |
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.
Left a few comments!
Co-authored-by: Colby Thomas <coloradocolby@gmail.com>
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
This is how it works now - 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.
Great! I think I have a working version, but there might be some issues I haven't caught or considered. |
@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! 🍻 |
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! |
Hmm I guess that's true.
Great! |
@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! |
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. |
Don't think there's much left to do here, closing. Thanks for all your help! |
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 theget_skip_count
function. Once we have calculated the amount of characters to skip, the implementation is relatively straightforward.Checklist
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.