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

fix: render cells that has no content but styles #182

Merged
merged 1 commit into from
May 3, 2024

Conversation

kxxt
Copy link
Contributor

@kxxt kxxt commented May 2, 2024

Fix #181

I am not confident about this fix since I only briefly looked at the code base. So this PR should be reviewed carefully to avoid breaking other things.

@a-kenji
Copy link
Owner

a-kenji commented May 2, 2024

Thank you for taking the time, creating this pr!
And sorry for the ci problems :).

On the surface this does make sense.
I will try it out for a bit to check for regressions.

@kxxt
Copy link
Contributor Author

kxxt commented May 2, 2024

On the surface this does make sense. I will try it out for a bit to check for regressions.

I think the test failure is indeed caused by this PR but I don't know if my PR is wrong or the test snapshot is wrong.

@a-kenji
Copy link
Owner

a-kenji commented May 2, 2024

I believe the snapshot needs to be updated.

The vttest script snapshots usually test very constrained functionality.
But I also added few snapshots that test a little more interaction, to check if general behavior changes.
So the test failing here is not a bad thing.

@kxxt kxxt force-pushed the fix/render-cell-with-style branch from a525733 to 66fcd7b Compare May 3, 2024 00:15
@kxxt
Copy link
Contributor Author

kxxt commented May 3, 2024

I believe the snapshot needs to be updated.

The vttest script snapshots usually test very constrained functionality. But I also added few snapshots that test a little more interaction, to check if general behavior changes. So the test failing here is not a bad thing.

Thanks for your explanation, I have updated the failing snapshot.

src/vt100_imp.rs Outdated Show resolved Hide resolved
@kxxt kxxt force-pushed the fix/render-cell-with-style branch from 66fcd7b to f3ce7a2 Compare May 3, 2024 14:10
@a-kenji a-kenji self-requested a review May 3, 2024 14:18
Copy link
Owner

@a-kenji a-kenji left a comment

Choose a reason for hiding this comment

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

LGMT, thank you for the contribution 👍 .

@a-kenji a-kenji merged commit 5b41df4 into a-kenji:development May 3, 2024
16 checks passed
@kxxt
Copy link
Contributor Author

kxxt commented May 3, 2024

Hi, @a-kenji. Could you please publish a new release(or prerelease) for this fix? Thanks!

@a-kenji
Copy link
Owner

a-kenji commented May 8, 2024

@kxxt,
Not sure if you got pinged on the release, but this fix is now in v0.11.0,

@kxxt
Copy link
Contributor Author

kxxt commented May 8, 2024

@kxxt, Not sure if you got pinged on the release, but this fix is now in v0.11.0,

Thanks a lot for your new release! I didn't get pinged but several days ago I found the bug magically disappeared, then I figured out that cargo update command I ran updated tui-term to 0.1.11: kxxt/tracexec@52a26c4

BTW there are some other issues regarding input with htop inside tui-term, which I am going to try to fix when I got more free time: kxxt/tracexec#11

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.

htop is not rendered correctly
2 participants