-
-
Notifications
You must be signed in to change notification settings - Fork 5
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
Implement interactive mode #29
Conversation
Because getTerminalSize from ansi-terminal expects not only stdout, but also stdin to be a terminal, which is too restrictive.
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.
Actually, this looks pretty good! 👏🏻
I have only one minor refactoring suggestion but otherwise, I'm glad that multiple parts of the code could be reused 🙂
src/DrCabal/Interactive.hs
Outdated
Copyright : (c) 2019 Alexander Gugel | ||
(c) 2022 Dmitrii Kovanikov |
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.
I have no complaints on the current copyright, you can even put your name here if you want 🙂
But I'm curious about the original code 🤔 Where a similar code was already written?
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.
My bad, I just copied header from DrCabal.Watch
.
src/DrCabal/Interactive.hs
Outdated
-- https://github.com/UnkindPartition/ansi-terminal/issues/141 | ||
when (chartHeight > 0) $ | ||
cursorUp chartHeight | ||
setCursorColumn 0 | ||
clearFromCursorToScreenEnd |
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.
Let's move this into a separate function clearScreen
or something like that for readability.
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.
Done.
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.
Thanks! 👏🏻
That's really great!
Closes #28.