Skip to content

Respect TERM=dumb for cursor position queries - #1183

Open
catlover-bot wants to merge 3 commits into
nushell:mainfrom
catlover-bot:fix-term-dumb
Open

Respect TERM=dumb for cursor position queries#1183
catlover-bot wants to merge 3 commits into
nushell:mainfrom
catlover-bot:fix-term-dumb

Conversation

@catlover-bot

@catlover-bot catlover-bot commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

Summary

Respect TERM=dumb when Reedline queries the terminal for the cursor position.

TERM=dumb does not provide cursor-position reporting, but Painter currently calls crossterm::cursor::position() from several paths. In particular, initialize_prompt_position() propagates a failed query directly.

This PR centralizes cursor-position queries behind a small wrapper:

  • when TERM=dumb, skip cursor::position() and return no measured position
  • otherwise preserve the existing Crossterm behavior
  • read TERM for each query rather than caching it, so runtime changes to TERM are respected
  • use the painter's existing cached/count-based fallback when no cursor position is available

TERM=dumb also disables ANSI coloring while preserving the configured with_ansi_colors value. TERM is read dynamically, so runtime changes are respected for both cursor-position queries and ANSI output.

Testing

  • regression tests for the TERM=dumb cursor-position path
  • ANSI-policy tests for TERM=dumb, regular terminals, and an unset TERM
  • full test suite with all features
  • cargo clippy --locked --all-targets --all-features -- -D warnings
  • git diff --check

Fixes #1009

@fdncred

fdncred commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

I wonder if it's worth centralizing all interactions with the env in calls like std::env::var_os("TERM");? It may be nice to have one function to rule them all since dealing with the env isn't threadsafe. Thoughts?

@catlover-bot

Copy link
Copy Markdown
Contributor Author

That makes sense to me.

I checked the current codebase and found only three direct process environment-variable reads: TERM, USERPROFILE, and HOME, so the scope is small enough to centralize here.

I pushed a follow-up commit that routes those reads through a single utils::environment::var_os helper. I left APIs such as current_dir() out since those are process-state operations rather than environment-variable reads.

One nuance is that the wrapper itself does not make concurrent external environment mutation thread-safe. The benefit is that Reedline now has one place where a future synchronization or snapshot policy can be implemented instead of having environment-variable reads scattered around the crate.

The full library tests and clippy are still green after the refactor.

@fdncred

fdncred commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

The benefit is that Reedline now has one place where a future synchronization or snapshot policy can be implemented instead of having environment-variable reads scattered around the crate.

yup, that's exactly what I was asking.

Thanks for the update. We'll wait to see what @kronberger-droid thinks. I'm not sure it's a regression so I doubt this will be for the upcoming patch.

@kronberger-droid

Copy link
Copy Markdown
Collaborator

Definitely not a regression, thats a longstanding issue.
I would not rush it in before the patch.

I will take a closer look tomorrow.

@catlover-bot

Copy link
Copy Markdown
Contributor Author

Thanks!

@kronberger-droid

Copy link
Copy Markdown
Collaborator

Well I think it would make sense to thread in the ansi-coloring removal here to.
It should be easy enough since we already handle it consistently through the config.

@catlover-bot

Copy link
Copy Markdown
Contributor Author

Thanks, I threaded TERM=dumb through the existing ANSI-color configuration as well.

The configured with_ansi_colors value is preserved, while the effective value is disabled when the current TERM is dumb. I compute that effective value once per paint operation, so the same render uses a consistent setting while later runtime changes to TERM are still respected.

I also rebased onto current main and kept the newer W::cursor_position() abstraction, so non-dumb terminals continue through the existing testable terminal-I/O path.

I added coverage for TERM=dumb, a regular terminal, and an unset TERM; the full test suite and clippy are green.

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.

Respect TERM=dumb

3 participants