search

arxanas / scm-record

`scm-record` is a UI component to interactively select changes to include in a commit. It's meant to be embedded in source control tooling.
23 stars 9 forks source link

Fix console mode restoration #31

Closed martindisch closed 4 months ago

martindisch commented 5 months ago

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 commented 4 months ago

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 commented 4 months ago

Excellent, thanks!