Skip to content

fix remove dropdown option index out of range#927

Closed
7yyo wants to merge 2 commits intorivo:masterfrom
7yyo:drop_down_index_out_of_range
Closed

fix remove dropdown option index out of range#927
7yyo wants to merge 2 commits intorivo:masterfrom
7yyo:drop_down_index_out_of_range

Conversation

@7yyo
Copy link
Copy Markdown

@7yyo 7yyo commented Dec 7, 2023

@rivo

Hi, when setSelectFunc of dropdown to remove current option, if the last option is selected, index out of range will panic. I tried to fix this bug.

The code below will reproduce this bug if select the last option Fifth.

package main

import "github.com/rivo/tview"

func main() {
	app := tview.NewApplication()
	dropdown := tview.NewDropDown()
	dropdown.SetLabel("Select an option (hit Enter): ")
	selectFunc := func(text string, index int) {
		dropdown.RemoveOption(index)
	}
	dropdown.SetOptions([]string{"First", "Second", "Third", "Fourth", "Fifth"}, selectFunc)
	if err := app.SetRoot(dropdown, true).EnableMouse(true).Run(); err != nil {
		panic(err)
	}
}

@rivo rivo closed this in bf8f1c4 Jan 16, 2024
@rivo
Copy link
Copy Markdown
Owner

rivo commented Jan 16, 2024

Thanks for catching this. Two things needed to be done here:

  1. After line 516, store d.options[d.currentOption] in a local variable, e.g. currentOption. Use it in the lines that follow (3 times). This block is just to call the "selected" callbacks, the one for all options and the one defined by one option. Your solution could lead to the wrong callback to be called because you're adjusting d.currentOption before calling the option's "selected" callback.
  2. Set d.currentOption to -1 in RemoveOption if the removed option is the current one. Document this in the function comments.

I ended up making these changes myself. I hope you don't mind.

kopecmaciej pushed a commit to kopecmaciej/tview that referenced this pull request Aug 27, 2024
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