Skip to content

feat: add --initial option for cursor positioning and fix filter --selected bug #916

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.

Sign up for GitHub

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 1 commit into
base: main
Choose a base branch
from
Open

feat: add --initial option for cursor positioning and fix filter --selected bug #916

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
64 changes: 64 additions & 0 deletions choose/choose_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
package choose

import (
"testing"
)

func TestInitialPositioning(t *testing.T) {
tests := []struct {
name string
options []string
initial string
expected int // expected starting index
}{
{
name: "initial option exists",
options: []string{"Apple", "Banana", "Cherry", "Date"},
initial: "Cherry",
expected: 2,
},
{
name: "initial option doesn't exist",
options: []string{"Apple", "Banana", "Cherry", "Date"},
initial: "Orange",
expected: 0, // should default to first item
},
{
name: "empty initial option",
options: []string{"Apple", "Banana", "Cherry", "Date"},
initial: "",
expected: 0, // should default to first item
},
{
name: "initial option is first",
options: []string{"Apple", "Banana", "Cherry", "Date"},
initial: "Apple",
expected: 0,
},
{
name: "initial option is last",
options: []string{"Apple", "Banana", "Cherry", "Date"},
initial: "Date",
expected: 3,
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
// Simulate the logic from command.go
Copy link
Contributor

@sadamczyk sadamczyk Jun 14, 2025

Choose a reason for hiding this comment

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

I don't think this test is very useful as it is, because it doesn't actually test the code running in the command, it just "simulates" it. If the command changes, this test will happily keep reporting a successful test.

Honestly, I would probably just remove both of the tests. There currently aren't many tests in this repo to begin with and adding them in this PR doesn't feel like the right place to start, especially because the structure of the code for the commands makes it kind of difficult to test only small parts of the functionality like this.
There is an open issue regarding adding tests: #818

startingIndex := 0
if tt.initial != "" {
for i, option := range tt.options {
if option == tt.initial {
startingIndex = i
break
}
}
}

if startingIndex != tt.expected {
t.Errorf("expected starting index %d, got %d", tt.expected, startingIndex)
}
})
}
}
10 changes: 10 additions & 0 deletions choose/command.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,16 @@ func (o Options) Run() error {
items[i] = item{text: option, selected: isSelected, order: order}
}

// Handle initial cursor position if specified
if o.Initial != "" {
for i, option := range o.Options {
if option == o.Initial {
startingIndex = i
break
}
}
}

// Use the pagination model to display the current and total number of
// pages.
pager := paginator.New()
Expand Down
1 change: 1 addition & 0 deletions choose/options.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ type Options struct {
SelectedPrefix string `help:"Prefix to show on selected items (hidden if limit is 1)" default:"✓ " env:"GUM_CHOOSE_SELECTED_PREFIX"`
UnselectedPrefix string `help:"Prefix to show on unselected items (hidden if limit is 1)" default:"• " env:"GUM_CHOOSE_UNSELECTED_PREFIX"`
Selected []string `help:"Options that should start as selected (selects all if given *)" default:"" env:"GUM_CHOOSE_SELECTED"`
Initial string `help:"Option that should start with the cursor positioned on it" default:"" env:"GUM_CHOOSE_INITIAL"`
Copy link
Contributor

@sadamczyk sadamczyk Jul 12, 2025

Choose a reason for hiding this comment

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

I personally feel like --cursor might be the better name for this new option.
Especially in combination with --selected it communicates very clearly that it only affects the cursor position, whereas --inital feels a little ambigous to me.
But as always naming things is one of the hardest problems :), so I'd be curious to hear other people's input on this.

SelectIfOne bool `help:"Select the given option if there is only one" group:"Selection"`
InputDelimiter string `help:"Option delimiter when reading from STDIN" default:"\n" env:"GUM_CHOOSE_INPUT_DELIMITER"`
OutputDelimiter string `help:"Option delimiter when writing to STDOUT" default:"\n" env:"GUM_CHOOSE_OUTPUT_DELIMITER"`
Expand Down
13 changes: 12 additions & 1 deletion filter/command.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,15 +132,26 @@ func (o Options) Run() error {
continue
}
if o.Limit == 1 {
// When the user can choose only one option don't select the option but
// start with the cursor hovering over it.
Comment on lines 134 to +136
Copy link
Contributor

@sadamczyk sadamczyk Jul 12, 2025

Choose a reason for hiding this comment

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

Suggested change
if o.Limit == 1 {
// When the user can choose only one option don't select the option but
// start with the cursor hovering over it.
if o.Initial != "" && option.Str == o.Initial {

Currently --selected has two different behaviors.

  1. If --no-limit or --limit=<any value above 1> then the cursor position isn't changed at all. This can stay.
  2. If --limit=1 (the default), then it only sets the initial cursor position. But that's exactly what the new option --initial does, so it does not make much sense to me to keep this behavior of --selected around. Because then there would be two ways to do the exact same thing, which seems confusing.
    This also saves us from having to loop over all the options again in the second loop, so that can be removed.

This would be a significant BC break though, because I imagine a lot of people rely on this behavior of --selected because it's the default. So it might be better to deprecate this behavior of --selected first, before just outright removing it.

m.cursor = i
m.selected[option.Str] = struct{}{}
} else {
currentSelected++
m.selected[option.Str] = struct{}{}
}
}
}

// Handle initial cursor position if specified
if o.Initial != "" {
for i, match := range matches {
if match.Str == o.Initial {
m.cursor = i
break
}
}
}

tm, err := tea.NewProgram(m, options...).Run()
if err != nil {
return fmt.Errorf("unable to run filter: %w", err)
Expand Down
66 changes: 66 additions & 0 deletions filter/filter_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"testing"

"github.com/charmbracelet/x/ansi"
"github.com/sahilm/fuzzy"
)

func TestMatchedRanges(t *testing.T) {
Expand Down Expand Up @@ -57,3 +58,68 @@ func TestByteToChar(t *testing.T) {
t.Errorf("expected %+q, got %+q", expect, got)
}
}

func TestInitialCursorPositioning(t *testing.T) {
tests := []struct {
name string
options []string
initial string
expected int // expected cursor position
}{
{
name: "initial option exists",
options: []string{"Apple", "Banana", "Cherry", "Date"},
initial: "Cherry",
expected: 2,
},
{
name: "initial option doesn't exist",
options: []string{"Apple", "Banana", "Cherry", "Date"},
initial: "Orange",
expected: 0, // should default to first item
},
{
name: "empty initial option",
options: []string{"Apple", "Banana", "Cherry", "Date"},
initial: "",
expected: 0, // should default to first item
},
{
name: "initial option is first",
options: []string{"Apple", "Banana", "Cherry", "Date"},
initial: "Apple",
expected: 0,
},
{
name: "initial option is last",
options: []string{"Apple", "Banana", "Cherry", "Date"},
initial: "Date",
expected: 3,
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
// Create matches like in the actual filter command
matches := make([]fuzzy.Match, len(tt.options))
for i, option := range tt.options {
matches[i] = fuzzy.Match{Str: option}
}

// Simulate the logic from command.go
cursor := 0
if tt.initial != "" {
for i, match := range matches {
if match.Str == tt.initial {
cursor = i
break
}
}
}

if cursor != tt.expected {
t.Errorf("expected cursor position %d, got %d", tt.expected, cursor)
}
})
}
}
1 change: 1 addition & 0 deletions filter/options.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ type Options struct {
NoLimit bool `help:"Pick unlimited number of options (ignores limit)" group:"Selection"`
SelectIfOne bool `help:"Select the given option if there is only one" group:"Selection"`
Selected []string `help:"Options that should start as selected (selects all if given *)" default:"" env:"GUM_FILTER_SELECTED"`
Initial string `help:"Option that should start with the cursor positioned on it" default:"" env:"GUM_FILTER_INITIAL"`
ShowHelp bool `help:"Show help keybinds" default:"true" negatable:"" env:"GUM_FILTER_SHOW_HELP"`
Strict bool `help:"Only returns if anything matched. Otherwise return Filter" negatable:"" default:"true" group:"Selection"`
SelectedPrefix string `help:"Character to indicate selected items (hidden if limit is 1)" default:" ◉ " env:"GUM_FILTER_SELECTED_PREFIX"`
Expand Down