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 console mode restoration #31

Merged
merged 1 commit into from
Apr 14, 2024

Conversation

martindisch
Copy link
Contributor

While using Jujutsu on Windows 10/11 in PowerShell, I discovered a bug that took me forever to find but was very easy to fix. Whenever I enter the scm-record view (for example with jj diffedit) for the second time in a row, it fails to properly repaint and finally errors out with "Initial console modes not set" from the underlying crossterm.

In order to paint the view, ui::set_up_crossterm will first enable raw mode and then mouse capture. Once done, ui::clean_up_crossterm disables raw mode and then mouse capture. The problem is that the EnableMouseCapture command in crossterm stores the current mode and DisableMouseCapture restores it.

This means with this order of operations

  1. Enable raw mode
  2. Enable mouse capture (store mode)
  3. Disable raw mode
  4. Disable mouse capture (restore mode)

the disable mouse capture call will re-enable the stored raw mode just after it was supposed to be disabled. Upon entering scm-record for the second time, raw mode is still enabled, which means it will skip properly initializing everything.

I only observed this behavior in PowerShell so far, not cmd.exe. Maybe only PowerShell respects these accidentally mixed up mode settings between invocations of jj while cmd.exe restores the original mode somehow.

The fix was simply to make the whole dance symmetrical, meaning

  1. Enable mouse capture (store mode)
  2. Enable raw mode
  3. Disable raw mode
  4. Enable mouse capture (restore mode)

That's what I did in the first commit. Then I realized that enabling mouse capture in crossterm actually already sets the mode to something that satisfies the conditions for being raw mode. Therefore I think it's not necessary to set raw mode ourselves and I removed it completely in the second commit. Feel free to drop that in case there's a reason for it, both approaches solve the issue for me.

@arxanas arxanas enabled auto-merge (rebase) April 14, 2024 21:48
@arxanas arxanas merged commit 6a04d37 into arxanas:main Apr 14, 2024
2 checks passed
@arxanas
Copy link
Owner

arxanas commented Apr 14, 2024

Thanks for the fix! I dropped the second commit. I think it's not obvious that enabling mouse mode will prepare the terminal just from looking at the code, and a future change might unintentionally remove mouse mode might not know to add back the necessary setup code.

@martindisch martindisch deleted the fix-mode-restoration branch April 15, 2024 05:29
@martindisch
Copy link
Contributor Author

Excellent, 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.

2 participants