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

feat(viewport): horizontal scroll #240

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

tty2
Copy link

@tty2 tty2 commented Sep 14, 2022

implement #235

@meowgorithm
Copy link
Member

meowgorithm commented Sep 14, 2022

Thanks for the PR! One immediate things is jumping out here:

len() counts bytes and not runes, which are not always the same. Similarly, some runes occupy two cells. For example:

// 3 bytes, 3 cells
Hi!

// 7 bytes, 5 cells:
世界!

So, in short, keep in mind that horizontal scrolling will need to handle double-width runes. Here’s a more in-depth example that breaks it down further:

https://go.dev/play/p/7lPoU2SMjn5

@tty2
Copy link
Author

tty2 commented Sep 14, 2022

Thanks for the PR! One immediate things is jumping out here:

len() counts bytes and not runes, which are not always the same. Similarly, some runes occupy two cells. For example:

// 3 bytes, 3 cells
Hi!

// 7 bytes, 5 cells:
世界!

So, in short, keep in mind that horizontal scrolling will need to handle double-width runes. Here’s a more in-depth example that breaks it down further:

https://go.dev/play/p/7lPoU2SMjn5

It's a good point. I guess I have to use lipgloss.Width here.

@tty2
Copy link
Author

tty2 commented Sep 15, 2022

Waiting for this PR merge and tagging.
Will continue after that.

@tty2
Copy link
Author

tty2 commented Sep 20, 2022

Don't merge it yet, please.
I'm gonna write some simple tests later this evening.

@tty2
Copy link
Author

tty2 commented Sep 20, 2022

Done!
@meowgorithm could you check it, please?
I can suppose that last test case (list: with y offset: horizontal scroll) can be too complicated. But I'm not sure it it's better to change it.
WDYT?

@meowgorithm
Copy link
Member

Thanks, @tty2. Before we get to this, do you mind adding some tests with double-width runes? That is going to be the number one thing we'll want to verify. In particular, cases where there's a mix of single and double width runes.

@tty2
Copy link
Author

tty2 commented Sep 20, 2022

These tests I've written inside go-runewidth library. For TruncateLeft function I use here.
But ok I'll write them here as well. I'll do it tomorrow.

@tty2
Copy link
Author

tty2 commented Sep 21, 2022

@meowgorithm
Done! Check the last test case.

@tty2
Copy link
Author

tty2 commented Sep 26, 2022

Hi, @meowgorithm
Should I change anything?

@meowgorithm
Copy link
Member

meowgorithm commented Sep 26, 2022

Hey, @tty2! Thanks for all your work on this so far. It's going to be a little bit of time until we can give this a proper review, but I promise we'll get to it.

@bobziuchkovski
Copy link

We'd absolutely love this functionality as well!

However, I tested this branch and found a potential issue: the rune trimming also trims color codes. The below are a series of screens showing horizontal scroll of a terraform plan that includes colored text:

Screenshot 2023-09-17 at 10 10 17 AM
Screenshot 2023-09-17 at 10 10 25 AM
Screenshot 2023-09-17 at 10 10 37 AM
Screenshot 2023-09-17 at 10 10 45 AM

@Sojamann Sojamann mentioned this pull request Sep 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants