Skip to content

fix(tuikit): reset attrs on exit #784

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

LoricAndre
Copy link
Contributor

@LoricAndre LoricAndre commented May 24, 2025

Checklist

check the box if it is not applicable to your changes

  • I have updated the README with the necessary documentation
  • I have added unit tests
  • I have added end-to-end tests
  • I have linked all related issues or PRs

Description of the changes

@LoricAndre LoricAndre requested a review from Copilot May 28, 2025 21:43
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR ensures terminal attributes are reset on exit in the TUI backend and adds coverage through an ANSI-focused end-to-end test, along with a new example for skim.

  • Reset terminal attributes after exiting TermLock to avoid leftover styling.
  • Add reset_after_exit_height e2e test and extend TmuxController to capture ANSI output with a configurable timeout.
  • Introduce a default.rs example demonstrating basic skim usage.

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File Description
skim/examples/default.rs New CLI example illustrating basic skim usage
skim-tuikit/src/term.rs Added output.reset_attributes() after flushing on exit
e2e/tests/ansi.rs New e2e test to verify ANSI attribute reset after exit
e2e/src/lib.rs Defined TIMEOUT, refactored wait, and added ANSI capture
Comments suppressed due to low confidence (3)

e2e/src/lib.rs:17

  • Consider renaming TIMEOUT to TIMEOUT_SECS (or similar) to explicitly convey that this constant represents seconds, improving clarity when it's used in timing calculations.
const TIMEOUT: usize = 30; // seconds

e2e/tests/ansi.rs:21

  • This assertion only checks the foreground reset. To fully cover reset_attributes(), consider also verifying that the background and other style attributes are cleared (e.g., check for \u{1b}[49m).
assert!(lines[1].contains("\u{1b}[39m2") || lines[1].contains("\u{1b}[0m2"));

skim/examples/default.rs:1

  • In Rust 2018 edition and later, extern crate skim; is redundant—consider removing it to align with modern conventions.
extern crate skim;

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
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.

Terminal colors are not restored after Skim::run_with returns
1 participant