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

Use builtin Clear function for screen clearing #1169

Closed
wants to merge 2 commits into from
Closed

Use builtin Clear function for screen clearing #1169

wants to merge 2 commits into from

Conversation

joelim-work
Copy link
Collaborator

Fixes #845

Credit to @horriblename for finding this.

@joelim-work joelim-work mentioned this pull request Mar 21, 2023
@gokcehan
Copy link
Owner

@joelim-work @horriblename Thanks for coming up with this patch. It seems that the change from Clear to manual filling was introduced with the change from termbox to tcell in #439 . In the discussion it says Clear has an extra flush so it was changed to manual filling to avoid the flush. I'm not sure if it is still the case with recent versions of tcell. There is also mention of a flicker issue on Windows so this patch needs to be tested on Windows as well. All in all, we need to make sure this patch does not change the behavior of the program in any way.

@joelim-work
Copy link
Collaborator Author

@gokcehan Thanks for mentioning this, I didn't realise there that the behaviour would be different on Windows. I did a test on Windows and I can confirm that the slowness is still an issue.

It looks like using Clear/Fill is semantically different to just using SetContent, it sets s.clear = true which eventually ends up flushing the screen by calling clearScreen: https://github.com/gdamore/tcell/blob/c951371181bb4b1387d9010b8081590ae6a48152/console_win.go#L1159-L1170

I don't think there's any good way around this so I'm going to just close this pull request. Maybe it might be worth adding a comment to explain the Windows issue, but I'll leave that up to you to decide.

@joelim-work joelim-work deleted the simple-clear branch March 22, 2023 03:40
gokcehan added a commit that referenced this pull request Mar 25, 2023
@gokcehan
Copy link
Owner

@joelim-work I have now added a comment, thanks.

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.

simplify code
2 participants