-
Notifications
You must be signed in to change notification settings - Fork 8
fix: Pty4j with JLine in raw mode does indeed avoid Shell echo #1416
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
Conversation
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.
Hello @vorburger, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
Summary of Changes
As gemini-code-assist, I've analyzed this pull request. The core purpose of this change is to address and demonstrate a fix for an issue where using Pty4j in conjunction with JLine resulted in unwanted double echoing of input from the shell. The PR confirms that enabling JLine's raw mode (terminal.enterRawMode()
) is the correct approach to prevent the shell from echoing, contrary to previous assumptions noted in the code's comments. This change updates the Demo.java
example to correctly use raw mode and cleans up outdated comments and TODOs.
Highlights
- Fix Shell Echo Issue: This pull request provides a fix and demonstration for the issue of double echoing when using Pty4j with JLine. It confirms that entering JLine's raw mode (
terminal.enterRawMode()
) successfully prevents the shell from echoing input, resolving issues Pty4j Shell echo #1410 and Pty4j Cursor Up/Down & Backspace etc. #1411. - Update Demo Implementation: The
Demo.java
file is updated to correctly enable raw mode for the JLine terminal and remove comments that suggested this approach was ineffective or problematic. - Code Cleanup: Outdated TODO comments and a commented-out print statement are removed from the
Demo.java
file, improving code clarity.
Changelog
- java/dev/enola/common/exec/pty/Demo.java
- Enabled JLine raw mode (
terminal.enterRawMode()
) in the main method to correctly handle shell input without double echoing. - Removed comments indicating that raw mode or
terminal.echo(false)
did not fix the echo issue. - Added a comment referencing the related pty4j issue chore(deps): update dependency mkdocs-material to v9.1.12 #170 for context.
- Removed a commented-out
System.out.println
line. - Removed TODO comments related to
system(false)
andsystemOutput
.
- Enabled JLine raw mode (
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command>
or @gemini-code-assist <command>
. Below is a summary of the supported commands.
Feature | Command | Description |
---|---|---|
Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/
folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Shell speaks too loud,
Raw mode silences the crowd,
Just one echo now.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
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.
Code Review
This pull request effectively addresses the shell echo issue when using Pty4j with JLine by correctly configuring the terminal in raw mode. The changes simplify the Demo.java
class, remove outdated TODOs and misleading comments, and introduce a clear explanation for the behavior of terminal.echo()
in raw mode. The core fix, involving the use of TerminalBuilder.builder().build()
(which defaults to system(false)
) and terminal.enterRawMode()
, appears to be well-targeted and resolves the problems mentioned in issues #1410, #1411, and the linked pty4j issue.
Summary of Findings
- Successful Fix Implementation: The primary change of enabling raw mode via
terminal.enterRawMode()
and adjusting theTerminalBuilder
configuration (by removing.system(true)
, thus defaulting tosystem(false)
) successfully resolves the reported shell echo problem. This aligns with the findings in the linked pty4j issue. - Code Clarity and Cleanup: The removal of previous TODO comments related to the echo issue, along with a commented-out debug line, improves the overall clarity and maintainability of the
Demo.java
class. The previous, apparently incorrect, comment advising against raw mode has also been removed, reflecting the new, correct approach. - Informative Comment Added: The new comment
// NB: terminal.echo() true or false makes no difference (because we're in raw mode)
is a valuable addition, clarifying the terminal's behavior in raw mode and potentially preventing future confusion.
Merge Readiness
The changes in this pull request are clear, directly address the intended fix, and improve the codebase. Based on this review, the code appears to be in good shape and ready for merging. As I am an AI, I am not authorized to approve pull requests; please ensure it undergoes any further necessary review processes before merging.
Fixes #1410 & #1411.
See JetBrains/pty4j#170.