Skip to content

Always show cursor during userconfirm - #3001

Merged
pkratoch merged 1 commit into
rpm-software-management:mainfrom
evan-goode:evan-goode/userconfirm-show-cursor
Oct 26, 2026
Merged

pkratoch merged 1 commit into
rpm-software-management:mainfrom
evan-goode:evan-goode/userconfirm-show-cursor

Conversation

@evan-goode

@evan-goode evan-goode commented Oct 22, 2026 •

Copy link
Copy Markdown
Member

If the cursor is hidden when libdnf5::cli::utils::userconfirm is called, show it, then hide it again once userconfirm returns.

This requires tracking whether the cursor is hidden in cursor_show and cursor_hide.
This requires tracking whether the cursor is hidden with new helpers hide_cursor and show_cursor.

Previously, the cursor would remain hidden if the user is prompted to import a GPG key during a transaction.

Assisted-by: GPT-6 Luna

@evan-goode
evan-goode requested a review from a team as a code owner October 22, 2026 23:32
@evan-goode
evan-goode requested review from pkratoch and removed request for a team October 22, 2026 23:32
@qodo-free-for-open-source-projects

Copy link
Copy Markdown

PR Summary by Qodo

Show the cursor while requesting user confirmation

🐞 Bug fix 🕐 10-20 Minutes


AI Description

• Show the cursor during confirmation prompts, including GPG-key import prompts during transactions.
• Restore its prior hidden state when confirmation returns, including on early returns.
Diagram

graph TD
  A["Progress bar"] -->|hides cursor| B["TTY visibility"] -->|state read by| C["User confirmation"] -->|registers restoration| D["Scope guard"]
  C -->|shows cursor| B
  D -->|rehides if needed| B
Loading
High-Level Assessment

The focused approach fits the existing cursor manipulators and scope-guard utility. Handling visibility in each caller would duplicate prompt logic and would not cover other uses of userconfirm.

Files changed (3) +36 / -2

Bug fix (3) +36 / -2
tty.hppExpose tracked cursor visibility +2/-0

Expose tracked cursor visibility

• Declares a public query for the visibility state maintained by the cursor hide and show manipulators.

include/libdnf5-cli/tty.hpp

tty.cppTrack successful cursor visibility changes +23/-2

Track successful cursor visibility changes

• Replaces the macro-generated cursor manipulators with implementations that update tracked state when an interactive-terminal escape sequence is written successfully.

libdnf5-cli/tty.cpp

userconfirm.cppShow cursor for confirmation and restore prior state +11/-0

Show cursor for confirmation and restore prior state

• Shows the cursor before confirmation processing and uses a scope guard to hide it again if it was previously hidden. Restoration also runs when confirmation returns early.

libdnf5-cli/utils/userconfirm.cpp

Comment thread libdnf5-cli/utils/userconfirm.cpp Outdated
Comment on lines +36 to +37
const bool cursor_was_hidden = libdnf5::cli::tty::is_cursor_hidden();
std::cerr << libdnf5::cli::tty::cursor_show;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟠 Medium

1. Redirected error output gains cursor codes 🐞 Bug ≡ Correctness

userconfirm() sends cursor_show to std::cerr on every call, while that manipulator decides
whether to emit its escape sequence by checking stdout rather than the destination stream. When
stdout remains a terminal but stderr is redirected, confirmations—including automatic yes/no
confirmations—write a cursor-control code into captured stderr, and a previously hidden cursor adds
a second code on return.
Agent Prompt
## Issue description
`userconfirm()` writes cursor-control sequences to redirected stderr whenever stdout is interactive, because `tty::is_interactive()` checks stdout.
## Fix Focus Areas
- libdnf5-cli/utils/userconfirm.cpp[35-42]
- libdnf5-cli/tty.cpp[48-66]
## Recommended Fix
Only issue the show and matching hide commands when the destination for those commands is a terminal. Preserve the original cursor state when no command is emitted.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

while that manipulator decides whether to emit its escape sequence by checking stdout rather than the destination stream

That's fine, not the fault of this PR, is_interactive checks only std::cout.

including automatic yes/no confirmations

Valid, I adjusted this to not hide/show during automatic confirmations.

If the cursor is hidden when libdnf5::cli::utils::userconfirm is called,
show it, then hide it again once userconfirm returns.

This requires tracking whether the cursor is hidden with new helpers
hide_cursor and show_cursor.

Previously, the cursor would remain hidden if the user is prompted to
import a GPG key during a transaction.

Assisted-by: GPT-6 Luna
@evan-goode
evan-goode force-pushed the evan-goode/userconfirm-show-cursor branch from 56b033b to a78a5ab Compare October 22, 2026 23:56
@evan-goode

Copy link
Copy Markdown
Member Author

I reworked this a bit to add new helpers, hide_cursor and show_cursor, instead of having the TTY_COMMANDs cursor_hide and cursor_show manage the static bool. It was awkward to have them update the global is_cursor_hidden since the ostream argument could be any stream, not just stderr or stdout. It also looks strange to me to have something that looks like

std::cerr << tty::cursor_show;

have side effects other than writing to std::cerr.

@pkratoch pkratoch self-assigned this Oct 26, 2026

@pkratoch pkratoch left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good, thanks!

@pkratoch
pkratoch added this pull request to the merge queue Oct 26, 2026
Merged via the queue into rpm-software-management:main with commit 4167de1 Oct 26, 2026
17 of 18 checks passed
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