Always reset cursor correctly on exit. #7931
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
When Application::run is exiting, either Terminal::restore or Terminal::force_restore will be called depending on if a panic occured or not. Both of these functions will reset the cursor to terminal's default. After this is done, Terminal::drop will be called. If terminal.cursor_kind == Hidden, then the cursor will be reset to a CursorKind::Block, undoing the work of restore or force_restore. This commit just removes the drop implementation, as its job is already better handled in restore and force_restore.
the-mikedavis
approved these changes
Aug 15, 2023
There was a problem hiding this comment.
Good catch!
For context: we use the terminal's (block) cursor in the Prompt component used to enter typable commands, so when you use :quit you're exiting from the prompt with Terminal.cursor_kind == Block. With a keybind you can exit from normal mode where the cursor is hidden (done by default, so we can color the cursor), so the Drop calls show_cursor. restore and force_restore should always be called even if we panic so no need to reset the cursor in the Drop anyways.
the-mikedavis
added
C-bug
dgkf
pushed a commit
to dgkf/helix
that referenced
this pull request
Jan 30, 2024
…-editor#7931) When Application::run is exiting, either Terminal::restore or Terminal::force_restore will be called depending on if a panic occured or not. Both of these functions will reset the cursor to terminal's default. After this is done, Terminal::drop will be called. If terminal.cursor_kind == Hidden, then the cursor will be reset to a CursorKind::Block, undoing the work of restore or force_restore. This commit just removes the drop implementation, as its job is already better handled in restore and force_restore.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is intended to be a fix for #7404. I was able to replicate on MacOS by setting Terminal.app's default cursor to something other than Block, and then binding a key to
:quitas described in the issue. Via debugging, I found thatTerminal::restoreactually does reset the cursor correctly regardless of if a keybind is used or if:quitis used.However, the cursor gets set back to
CursorKind::Blockwhen the process exits if a keybinding was used.It turns out that the
Dropimpl forTerminalwill put the cursor back in block mode ifterminal.cursor_kindisHidden. Via debugging, I found that this was not the case if:quitwas used but was the case if the keybind was used.My understanding is that either
Terminal::restoreorTerminalBackend::force_restorewill be called before exit, and both reset the cursor. Thus it seems to me that theDropimpl forTerminalis redundant and does not have the information needed to do the reset properly. Thus this PR removes theDropimpl.