Skip to content

Support more scroll & edit actions#982

Open
rudenkornk wants to merge 3 commits into
alexpasmantier:mainfrom
rudenkornk:main
Open

Support more scroll & edit actions#982
rudenkornk wants to merge 3 commits into
alexpasmantier:mainfrom
rudenkornk:main

Conversation

@rudenkornk
Copy link
Copy Markdown
Contributor

📺 PR Description

Support more scroll & edit actions, see #977

Checklist

  • my commits and PR title follow the conventional commits format
  • if this is a new feature, I have added tests to consolidate the feature and prevent regressions
  • if this is a bug fix, I have added a test that reproduces the bug (if applicable)
  • I have added a reasonable amount of documentation to the code where appropriate

Copilot AI review requested due to automatic review settings March 25, 2026 14:58
Copy link
Copy Markdown

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

Adds additional navigation/edit actions to Television, including word-wise input movement and more granular/non-cycling list navigation, aligning keybinding customization with the capabilities available in other panels.

Changes:

  • Introduce word-movement input actions (go_to_prev_word, go_to_next_word) and wire them through action → input handling.
  • Add half-page navigation and non-cycling (“nocycle”) variants for list selection; plumb a cyclic flag through picker cursor movement.
  • Update help-panel action relevance and the sample config keybindings (alt-b/alt-f), plus picker unit tests for non-cycling behavior.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
television/television.rs Extends cursor movement API with cyclic and handles new selection actions; updates render-trigger logic (but currently misses new nocycle variants).
television/picker.rs Implements cyclic vs non-cyclic selection behavior and adds tests; contains a boundary-state bug for non-cyclic movement.
television/action.rs Adds new Action variants and labels/docs for the new navigation/edit behaviors.
television/input.rs Maps new word-movement actions to InputRequest.
television/screen/help_panel.rs Includes new actions in help relevance filtering (but currently inconsistent across modes).
.config/config.toml Adds default keybindings for word-wise navigation (alt-b / alt-f).
Comments suppressed due to low confidence (1)

television/picker.rs:124

  • Similar to inner_next, in non-cyclic mode if selected is already at the first item then new_selected won’t change, but relative_select is still decremented. This can make the relative state drift even though the absolute selection did not move. Consider making the boundary case a no-op (only update the relative selection when new_selected != selected).
        let new_selected = if cyclic {
            selected.wrapping_add(total_items - 1) % total_items
        } else {
            selected.saturating_sub(1)
        };
        self.select(Some(new_selected));

        if cyclic && new_selected == total_items - 1 {
            self.relative_select(Some((height - 1).min(total_items - 1)));
        } else {
            self.relative_select(Some(relative_selected.saturating_sub(1)));
        }

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread television/television.rs
Comment thread television/screen/help_panel.rs
Comment thread television/action.rs
Comment thread television/action.rs
Comment thread television/picker.rs
@alexpasmantier
Copy link
Copy Markdown
Owner

Thanks for the PR and sorry for the delay.

I think each of these need separate PRs so we can discuss them in isolation. The current diff introduces too much logic to my taste and worsens code understandability (especially the nocycle logic).

@rudenkornk
Copy link
Copy Markdown
Contributor Author

@alexpasmantier ok, thanks

I moved out first two commits to separate PRs:
#1026
#1027

If those merged I will rebase this one, since nocycle implementation depends on first two commits.

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.

3 participants