[GH-ISSUE #206] Pass option details into selection() callback when using (*DropDown).AddOption() #162

New issue

Closed

opened 2026-03-04 01:02:30 +03:00 by kerem · 3 comments

kerem commented

2026-03-04 01:02:30 +03:00

Owner

Originally created by @ardnew on GitHub (Dec 15, 2018).
Original GitHub issue: https://github.com/rivo/tview/issues/206

When using the (*DropDown).SetOptions() function, you can provide a selected() callback that is shared among all options because it passes in both the option text and index to the callback.

But if you use (*DropDown).AddOption(), the selected() callback is specific to that option, making it a little annoying when dynamically constructing the options.

I recommend also passing in both option text and index into the selected() callback when using (*DropDown).AddOption(). The convenience of this outweighs the risk of potentially having the wrong index passed into the callback if the user goes to the effort of manually moving options around after constructing the dropdown with AddOption() calls. Also it's consistent.

I think it's sufficient to change the following from:

func (d *DropDown) AddOption(text string, selected func()) *DropDown {
	d.options = append(d.options, &dropDownOption{Text: text, Selected: selected})
	d.list.AddItem(text, "", 0, nil)
	return d
}

to something like (warning: untested):

func (d *DropDown) AddOption(text string, selected func(string, int)) *DropDown {
	index := len(d.options)
	d.options = append(d.options, 
		&dropDownOption{
			Text: text, 
			Selected: func() {
				if selected != nil {
					selected(text, index)
				}
			}
		})
	d.list.AddItem(text, "", 0, nil)
	return d
}

And then update (*DropDown).SetOptions() to just pass it's selected argument directly into (*DropDown).AddOption() now that their prototypes are compatible.

Originally created by @ardnew on GitHub (Dec 15, 2018). Original GitHub issue: https://github.com/rivo/tview/issues/206 When using the `(*DropDown).SetOptions()` function, you can provide a `selected()` callback that is shared among all options because it passes in both the option text and index to the callback. But if you use `(*DropDown).AddOption()`, the `selected()` callback is specific to that option, making it a little annoying when dynamically constructing the options. I recommend also passing in both option text and index into the `selected()` callback when using `(*DropDown).AddOption()`. The convenience of this outweighs the risk of potentially having the wrong index passed into the callback if the user goes to the effort of manually moving options around after constructing the dropdown with `AddOption()` calls. Also it's consistent. I think it's sufficient to change the following from: ```go func (d *DropDown) AddOption(text string, selected func()) *DropDown { d.options = append(d.options, &dropDownOption{Text: text, Selected: selected}) d.list.AddItem(text, "", 0, nil) return d } ``` to something like (**warning**: untested): ```go func (d *DropDown) AddOption(text string, selected func(string, int)) *DropDown { index := len(d.options) d.options = append(d.options, &dropDownOption{ Text: text, Selected: func() { if selected != nil { selected(text, index) } } }) d.list.AddItem(text, "", 0, nil) return d } ``` And then update `(*DropDown).SetOptions()` to just pass it's `selected` argument directly into `(*DropDown).AddOption()` now that their prototypes are compatible.

kerem closed this issue

2026-03-04 01:02:31 +03:00

kerem commented

2026-03-04 01:02:31 +03:00

Author

Owner

@rivo commented on GitHub (Jan 13, 2019):

I appreciate you offering a solution already and I realize my response comes quite late. However, it's usually better to have a discussion first as I may want/need to come up with a different solution (requiring me to reject your PR).

In this specific case, as I understand it, you're looking for an item-independent "selected" callback which gives you the text and the item's index. Currently, you can only set this using SetOptions() but that doesn't work well when you want to modify options afterwards.

The callback provided with AddOption() is meant to allow you to provide specific functionality for that one option. The idea isn't for users to install the same callback for all options using AddOption().

So instead of breaking backwards compatibility (and changing the general idea of this per-item callback), I suggest that I add a new function SetSelectedFunc() which installs a "global" handler for all options. It would receive the text and the option's index. Also, SetOptions() would set that handler instead of overriding all item's own handlers. And it would be in line with how List is implemented, for example.

Let me know if this helps you and I'll get to it then.

@rivo commented on GitHub (Jan 13, 2019): I appreciate you offering a solution already and I realize my response comes quite late. However, it's usually better to have a discussion first as I may want/need to come up with a different solution (requiring me to reject your PR). In this specific case, as I understand it, you're looking for an item-independent "selected" callback which gives you the text and the item's index. Currently, you can only set this using `SetOptions()` but that doesn't work well when you want to modify options afterwards. The callback provided with `AddOption()` is meant to allow you to provide specific functionality for *that one option*. The idea isn't for users to install the same callback for all options using `AddOption()`. So instead of breaking backwards compatibility (and changing the general idea of this per-item callback), I suggest that I add a new function `SetSelectedFunc()` which installs a "global" handler for all options. It would receive the text and the option's index. Also, `SetOptions()` would set that handler instead of overriding all item's own handlers. And it would be in line with how `List` is implemented, for example. Let me know if this helps you and I'll get to it then.

kerem commented

2026-03-04 01:02:31 +03:00

Author

Owner

@ardnew commented on GitHub (Jan 13, 2019):

Yeah, I kinda went bananas with all the pull requests. But feel free to reject any or all of them. You certainly aren't hurting my feelings -- it's your code after all! I can fork if we reach an impasse.

The "global" callback handler is a perfectly sufficient alternative. I like the idea of being able to define a selection handler on a per-DropDown basis. Furthermore, if this behaved like your primitives' InputHandler(), it would be neat to let the callback provided to AddOption() have the option of trapping or forwarding the event before it gets passed onto this new DropDown-specific handler.

-- The following below just clarifies my original intent, strictly for your reading pleasure :) --

Maybe a contrived example better describes why I wanted this API changed. Consider a program that spawns multiple goroutines to watch/scan multiple file systems simultaneously. As these watchers/scanners discover a candidate directory's addition/removal from the file system, a corresponding item is added/removed from the DropDown. These items in the DropDown should share a common callback handler, but with the current API there isn't a way to distinguish which item was selected in the callback. Instead, you have to either (a.) pass in AddOption() a caller-defined closure that captures an index, or (b.) reset all of the items using SetOptions(). Both methods just felt a little awkward or unnecessary and could be resolved if only AddOption() and SetOptions() used the same (consistent) signature.

Nothing prevents the user from still defining a per-item callback with this approach. So even if it is arguably "changing the general idea" of the current design, it isn't reducing or eliminating any of the existing functionality.

The biggest problem, as you and I both acknowledged, is breaking backwards compatibility. Even if it's trivial to correct, I have to agree the benefit of the change isn't worth this breakage.

@ardnew commented on GitHub (Jan 13, 2019): Yeah, I kinda went bananas with all the pull requests. But feel free to reject **any or all of them**. You certainly aren't hurting my feelings -- it's your code after all! I can fork if we reach an impasse. The "global" callback handler is a perfectly sufficient alternative. I like the idea of being able to define a selection handler on a per-DropDown basis. Furthermore, if this behaved like your primitives' `InputHandler()`, it would be neat to let the callback provided to `AddOption()` have the option of trapping or forwarding the event before it gets passed onto this new DropDown-specific handler. _-- The following below just clarifies my original intent, strictly for your reading pleasure :) --_ Maybe a _contrived_ example better describes why I wanted this API changed. Consider a program that spawns multiple goroutines to watch/scan multiple file systems simultaneously. As these watchers/scanners discover a candidate directory's addition/removal from the file system, a corresponding item is added/removed from the `DropDown`. These items in the `DropDown` should share a common callback handler, but with the current API there isn't a way to distinguish which item was selected in the callback. Instead, you have to either (a.) pass in `AddOption()` a caller-defined closure that captures an index, or (b.) reset all of the items using `SetOptions()`. Both methods just felt a little awkward or unnecessary and could be resolved if only `AddOption()` and `SetOptions()` used the same (consistent) signature. Nothing prevents the user from still defining a per-item callback with this approach. So even if it is arguably "changing the general idea" of the current design, it isn't reducing or eliminating any of the existing functionality. The biggest problem, as you and I both acknowledged, is breaking backwards compatibility. Even if it's trivial to correct, I have to agree the benefit of the change isn't worth this breakage.

kerem commented

2026-03-04 01:02:32 +03:00

Author

Owner

@rivo commented on GitHub (Jan 23, 2019):

I added SelectedFunc() as discussed. But there is no way to stop the propagation of the event to the individual handlers as that would break backwards compatibility. But I think if this was really needed, it could be implemented outside of this class.

Anyway, if there are any issues with this solution, let me know.

@rivo commented on GitHub (Jan 23, 2019): I added `SelectedFunc()` as discussed. But there is no way to stop the propagation of the event to the individual handlers as that would break backwards compatibility. But I think if this was really needed, it could be implemented outside of this class. Anyway, if there are any issues with this solution, let me know.