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

calyx-pass [NAME tbd] #2108

Merged
merged 25 commits into from
Jun 12, 2024
Merged

calyx-pass [NAME tbd] #2108

merged 25 commits into from
Jun 12, 2024

Conversation

ethanuppal
Copy link
Member

Some things to figure out:

  • What should it be called?
  • Do we like the TUI?
  • Do we like how it finds the path to the calyx driver executable?
  • Have we implemented all the TODO.md features?

@ethanuppal ethanuppal requested a review from sampsyo June 5, 2024 23:28
@ethanuppal ethanuppal self-assigned this Jun 5, 2024
@ethanuppal ethanuppal changed the title Calyx pass [NAME tbd] calyx-pass [NAME tbd] Jun 5, 2024
@ethanuppal
Copy link
Member Author

I think I have a fix for the scrollback buffer issue -- I will manage the scrollback buffer myself, and use an alternative screen escape code (supported on xterm so supported everywhere). I don't really want to bring in something like curses since this is really lightweight (store lines to a vector),

@ethanuppal ethanuppal added Priority: Low Don't do these during a deadline S: In progress Issue is being worked on labels Jun 6, 2024
@ethanuppal
Copy link
Member Author

The only two pressing questions that remain are:

  • What should it be called?
  • Do we like how it finds the path to the calyx driver executable?

@ethanuppal ethanuppal marked this pull request as ready for review June 6, 2024 07:13
@ethanuppal ethanuppal removed the S: In progress Issue is being worked on label Jun 6, 2024
@ethanuppal ethanuppal requested review from sampsyo and removed request for sampsyo June 6, 2024 20:31
Copy link
Contributor

@rachitnigam rachitnigam left a comment

Choose a reason for hiding this comment

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

Left a bunch of comments but a higher-level thought: why are we implementing scrollback buffers from scratch instead of using a TUI library that handles all of the edge cases and platform differences for us?

I bet there are edge cases where things don't work on Windows machines and when terminals are resized.

@ethanuppal
Copy link
Member Author

Left a bunch of comments but a higher-level thought: why are we implementing scrollback buffers from scratch instead of using a TUI library that handles all of the edge cases and platform differences for us?

I felt that using two TUI libraries was overkill for just a scrollback buffer, but if you think it's a good idea, I can switch to using one.

I bet there are edge cases where things don't work on Windows machines and when terminals are resized.

Windows should take the same ANSI escape codes, but I think there might be some SetConsoleMode wizardry that needs to be done (though colored probably does that under the scene, so ANSI codes should be working). I'd have to test this though because I don't have access to a Windows machine.

@rachitnigam
Copy link
Contributor

I felt that using two TUI libraries was overkill for just a scrollback buffer

There might be some library that supports everything you need. In general, when working on Calyx tools, I do not enforce the "minimize dependencies" principle because the tool should be small, contained, and easy-to-maintain.

@ethanuppal
Copy link
Member Author

The ideal is some crate that gives me an interface like less where can type in key commands (no line buffering) and control scroll (so it scrolls to the top every time the screen is refreshed) that can also parse ANSI escape codes so that weird output doesn't appear. I wasn't able to find such a crate, but I probably haven't looked hard enough yet.

@ethanuppal
Copy link
Member Author

ethanuppal commented Jun 8, 2024

I've switched everything over to crossterm and refactored the TUI as well as addressed the other comments you made -- hopefully can commit soon. Happy to look into any other changes you feel should be made!

Calyx IR is not deserializable so I worked around that
Implemented changes from Rachit's review:

- Use a separate terminal library to handle most of the stuff for me so I don't have to worry about cross platform issues (I used `crossterm`)
- Updated scrollback buffer to respond to terminal resizes
- Other fixes and improvements
@rachitnigam rachitnigam removed the Priority: Low Don't do these during a deadline label Jun 10, 2024
Copy link
Contributor

@rachitnigam rachitnigam left a comment

Choose a reason for hiding this comment

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

Couple more low-level things! Fix and merge at your discretion! Awesome work!!

@ethanuppal ethanuppal merged commit 81c8a7f into main Jun 12, 2024
18 checks passed
@ethanuppal ethanuppal deleted the calyx-pass branch June 12, 2024 04:40
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.

2 participants