Evertras / bubble-table

A customizable, interactive table component for the Bubble Tea framework
MIT License
454 stars 27 forks source link

Feature request: have `Row` hold arbitrary data #74

Closed inkel closed 2 years ago

inkel commented 2 years ago

Hi! Let me start by saying this is a great project, fabulous work! 👏🏽

I was looking at the docs because I wanted to know if it were possible to associate any arbitrary data other than the column values, but according to the Row documentation, that's not possible 😞

This got me thinking that perhaps the following would be a better definition for Row plus a new method:

type Row struct {
    Style lipgloss.Style
    Cells RowData
    Data  interface{}
}

func (r Row) WithData(data interface{}) Row {
    r.Data = data
    return r
}

(I'm leaving the unexported fields untouched)

It is a breaking change, but not too big IMO, and the benefits would be great, as I hope to show in the following example: say you have a struct like the following that you'd like to display as a table:

type Person struct {
    Name string
    Age  int
}

func (p Person) Delete() error { /* … */ }

Then in your UI, after some key is pressed, you would like to delete the selected rows from the table. As it currently is, it I call SelectedRows the return slice will only have the columns information for each row, and and would then need to use that for some logic within my app to find each Person. However, with my proposed changes, the code would then be:

for _, row := range table.SelectedRows() {
    p := row.Data.(Person)
    p.Delete() // sample method call
}

I was willing to send a PR with this change, but given the blast radius it might have, that is, renaming a field in Row and adding a new field with the same name as the existing one, I thought it'd be better to ask first 😸

PS: after writing this I think you could reduce the impact if instead of reusing the Data name for the field, just add a new one such as:

type Row struct {
    Style lipgloss.Style
    Data RowData
    Item  interface{}
}

Then you could even get rid of the `Row.WithData method I believe, although it's nice syntactic sugar.

Anyway, happy to discuss it!

Evertras commented 2 years ago

Hey there, thanks for the interest and the ideas! First to clarify something, there's actually a way you could do this that isn't well documented. You can attach whatever you want to RowData, even if it doesn't match an existing column. This is an easy way to attach "metadata" to the rows. It's kind of sneaky, but you can see a hint of it in the main README with the "somethingelse" key. You could use some hidden column key to store the Person object or whatever other reference you wanted, and just pull that from the data without having to worry about it being visible. Let me know if that would work as a short term fix, and I think there could definitely be some documentation improvements to be made around this pattern. I actually intended this to be used as some hidden id columns originally as an option and never really made it explicit...

That aside, there's still an interesting discussion to be had here!

I'm very wary of doing the big breaking change version, but I do appreciate the general thought behind having a bound item to each row as a data source. I was actually considering doing something like this and having generation funcs that would fill in the column data, which would generally make the table more data-oriented. Maybe something like:

type Person struct {
    Name string
    Age  int
}

// This is a signature matching func(data interface{}) table.RowData
func rowDataFromPerson(data interface {}) table.RowData {
  p := data.(Person)
  return RowData {
    colKeyName: p.Name,
    colKeyAge: p.Age,
  }
}

table := table.New(cols).WithRowsFromData(people, rowDataFromPerson)

This would be way nicer with generics, but I'm also unsure of forcing a bump to 1.18 right now. Still, this would be an easy extension if we were to add Item to the Row struct as you point out at the end, where we could fill in Item and use the provided generation function for filling in RowData, and save the user some boilerplate. I'm not 100% sure if it's worth adding potential confusion. What if Item is nil? What if they're different types? Should it regenerate the row data? When? What triggers it? Is Item immutable? If not, is the table no longer immutable? Technically you could put some changing data into the rows right now, but this makes it a much more prominent gun to shoot yourself in the foot with that I want to handle carefully.

It may make sense to consider an adjacent API here instead of trying to cram them together. There may be "Bubble-table Classic" with the current system, and a much more data-defined version (maybe even with generics as a separate package!) that you could opt into that reuses a lot of the functionality under the hood but differs in how it handles rows.

Evertras commented 2 years ago

I've added an example in #75 that shows my first point in action, which I think should cover your Person example.

inkel commented 2 years ago

Thanks for the quick response! I haven't got the time yet to properly look at your answer, but I think your vision makes sense.

Regarding generics: I couldn't agree more, it seems like a great use case but is not worth forcing users to bump unnecessarily to 1.18+ of can be solved some other way.

inkel commented 2 years ago

Hi again!

You could use some hidden column key to store the Person object or whatever other reference you wanted, and just pull that from the data without having to worry about it being visible. Let me know if that would work as a short term fix

Yes, this definitely works as a short, medium, and long term fix. Once you mentioned it, it makes total sense, so perhaps there isn't a real need for an additional field for holding arbitrary data. In fact, with this solution you could have "infinitely" additional data associated with a row without having to resort to creating a structure to hold all this data.

I think there could definitely be some documentation improvements to be made around this pattern

I believe that with #75 this is already covered in a simple and neat way, although if it were part of the README (or go doc) it would make discovery easier perhaps, otherwise one needs to go and read the whole example.

It may make sense to consider an adjacent API here instead of trying to cram them together. There may be "Bubble-table Classic" with the current system, and a much more data-defined version (maybe even with generics as a separate package!) that you could opt into that reuses a lot of the functionality under the hood but differs in how it handles rows.

After reading your response a couple more times, I'm now on this camp too. The functionality I was requesting it is already there, and while it would be nice to use generics for a "cleaner" API I don't think this one is messy either. In fact, I wouldn't go down the generics route until is really, really need it.

Thanks again for your detailed response and for the nice design discussion! Feel free to close this one 😸

Evertras commented 2 years ago

Finally got around to adding the last few docs, as I agree with you in discoverability. To me, if a feature is difficult or confusing to use, that's a bug in itself, so it's worth improving. Thanks again for the discussion and feedback!