Skip to content

feat: enable terminal providers to implement ensure_visible #103

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

Merged

Conversation

jdurand
Copy link
Contributor

@jdurand jdurand commented Aug 3, 2025

Allow terminal providers to implement a custom ensure_visible method.
This provides more flexibility for custom terminal providers to handle
visibility logic.

@jdurand
Copy link
Contributor Author

jdurand commented Aug 3, 2025

@ThomasK33 I'd like to get your thoughts on this behavioral change before opening a PR. The new workflow seems more intuitive to me, but I'd appreciate your input to make sure we're aligned.

@ThomasK33
Copy link
Member

@ThomasK33 I'd like to get your thoughts on this behavioral change before opening a PR. The new workflow seems more intuitive to me, but I'd appreciate your input to make sure we're aligned.

Hey @jdurand, thanks for reaching out.
I would prefer to have an optional function field on the TerminalProvider class.

--- @class TerminalProvider
--- @field setup fun(config: TerminalConfig)
--- @field open fun(cmd_string: string, env_table: table, config: TerminalConfig, focus: boolean?)
--- @field close fun()
--- @field toggle fun(cmd_string: string, env_table: table, effective_config: TerminalConfig)
--- @field simple_toggle fun(cmd_string: string, env_table: table, effective_config: TerminalConfig)
--- @field focus_toggle fun(cmd_string: string, env_table: table, effective_config: TerminalConfig)
--- @field get_active_bufnr fun(): number?
--- @field is_available fun(): boolean
--- @field _get_terminal_for_test fun(): table?

Something like:

--- @field ensure_visible? fun()

Then we could update the ensure_terminal_visible_no_focus to have a simple check for

  if provider.ensure_visible then
    provider.ensure_visible()
    return true
  end

Each custom provider can implement the function as needed, or leave it as a noop.

@jdurand jdurand force-pushed the fix/external-claude-terminal-handling branch 2 times, most recently from 297b2f1 to 904b6a6 Compare August 4, 2025 14:39
@jdurand jdurand changed the title fix: skip terminal open when claude is connected externally feat: enable terminal providers to implement ensure_visible Aug 4, 2025
@jdurand jdurand marked this pull request as ready for review August 4, 2025 14:40
Copy link
Member

@ThomasK33 ThomasK33 left a comment

Choose a reason for hiding this comment

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

LGTM. The only thing I would ask before merging is to make the field optional in the LuaLS annotations. (link for anyone seeing this in the future: https://luals.github.io/wiki/annotations/#field)

Allow terminal providers to implement a custom ensure_visible method.
This provides more flexibility for custom terminal providers to handle
visibility logic.
@jdurand jdurand force-pushed the fix/external-claude-terminal-handling branch from 98aab3c to fa2dde8 Compare August 4, 2025 19:21
@ThomasK33 ThomasK33 added this pull request to the merge queue Aug 4, 2025
@ThomasK33
Copy link
Member

Thanks @jdurand 👍

Merged via the queue into coder:main with commit 4770090 Aug 4, 2025
3 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