Changed progress Update method to return the specific Model in progress.go instead of the generic tea.Model

[timmattison]

NOTE: I created this PR 1 month ago but apparently it was closed automatically when I deleted my fork

The progress bar code returns a generic tea.Model instead of the specific model in progress.go. This works but it means that you end up having to do a type assertion like this:

	var cmd tea.Cmd
	var cmds []tea.Cmd
	var newModel tea.Model

	newModel, cmd = m.ProgressBar.Update(msg)
	cmds = append(cmds, cmd)

	if newModel, ok := newModel.(progress.Model); ok {
		m.ProgressBar = newModel
	}

If it used the correct Model struct you could instead do this:

	var cmd tea.Cmd
	var cmds []tea.Cmd

	m.ProgressBar, cmd = m.ProgressBar.Update(msg)
	cmds = append(cmds, cmd)

This PR changes the return type.

[meowgorithm]

Hi there! So the reason we didn't do this on progress is because getting data off the model would require an assertion:

type Model struct {
    progress tea.Model
}

var cmd tea.Cmd
m.progress, cmd = m.progress.Update(msg)

if p, ok := m.progress.(progress.Model); ok {
    fmt.Println(p.Percent())
}

Would love to hear your reasoning for storing the struct as a tea.Model — though do keep in mind that this PR will incur a breaking change which we approach cautiously.

[timmattison]

I think the opposite is happening the existing code requires that assertion because the type it returns is tea.Model instead of the progress bar type. This removes the need for the assertion.

[timmattison]

I really think this is a matter of being a typo. If I look at all of the usages of update I see that most of them return the Model struct defined in their respective file.

Example: https://github.com/charmbracelet/bubbles/blob/ec883029c8e6f38bd2abbb92714c1bcfd3cc5d84/list/list.go#L771

But progress bar instead returns a generic tea.Model that needs to be cast to the correct type. https://github.com/charmbracelet/bubbles/blob/ec883029c8e6f38bd2abbb92714c1bcfd3cc5d84/progress/progress.go#L206

By running grep I can see that most return Model:

$ grep -r "func.*Update(" *
README.md:func (m Model) Update(msg tea.Msg) (tea.Model, tea.Cmd) {
cursor/cursor.go:func (m Model) Update(msg tea.Msg) (Model, tea.Cmd) {
filepicker/filepicker.go:func (m Model) Update(msg tea.Msg) (Model, tea.Cmd) {
help/help.go:func (m Model) Update(_ tea.Msg) (Model, tea.Cmd) {
key/key.go://   func (m Model) Update(msg tea.Msg) (tea.Model, tea.Cmd) {
list/list.go:func (m Model) Update(msg tea.Msg) (Model, tea.Cmd) {
list/defaultitem.go:func (d DefaultDelegate) Update(msg tea.Msg, m *Model) tea.Cmd {
list/list_test.go:func (d itemDelegate) Update(msg tea.Msg, m *Model) tea.Cmd { return nil }
paginator/paginator.go:func (m Model) Update(msg tea.Msg) (Model, tea.Cmd) {
progress/progress.go:func (m Model) Update(msg tea.Msg) (tea.Model, tea.Cmd) {
spinner/spinner.go:func (m Model) Update(msg tea.Msg) (Model, tea.Cmd) {
stopwatch/stopwatch.go:func (m Model) Update(msg tea.Msg) (Model, tea.Cmd) {
table/table.go:func (m Model) Update(msg tea.Msg) (Model, tea.Cmd) {
textarea/textarea.go:func (m Model) Update(msg tea.Msg) (Model, tea.Cmd) {
textinput/textinput.go:func (m Model) Update(msg tea.Msg) (Model, tea.Cmd) {
timer/timer.go:func (m Model) Update(msg tea.Msg) (Model, tea.Cmd) {
viewport/viewport.go:func (m Model) Update(msg tea.Msg) (Model, tea.Cmd) {

The exceptions are only in the README.md, and progress/progress.go. I think they should all return Model for consistency and to avoid having to cast.

[meowgorithm]

Ah, I misread the PR. Yep, the change probably makes sense. It wasn't a typo per se—but I agree that we should stick with the convention of the rest of bubbles.

Another option we've been discussing internally is a convention like the following:

func (Model) Update(tea.Msg) (tea.Model, tea.Cmd)
func (*Model) UpdateInPlace(tea.Msg) tea.Cmd

// Or alternatively
func (Model) UpdateAsModel(tea.Msg) (Model, tea.Cmd)

This would allow models to be composable, but also give users the option to skip unnecessary assertions.

[timmattison]

The only comment I'd have about the method with a pointer receiver is that some linters will complain if you have mix and match pointer receivers and regular ones.

I don't know if it is valid criticism, and I'm not at my machine to double check, but I'm pretty sure Golang highlights them by default.

[maaslalani]

@meowgorithm, how do you feel about merging this one? I agree that implementing the tea.Model interface is desirable. But, I think consistency is more desirable in this case. If we decide that all bubbles should implement the tea.Model interface all of the changes can be done at the same time.