longbridgeapp / gpui-component

UI components write in GPUI.
Apache License 2.0
150 stars 20 forks source link

`ui::table::TableDelegate::rows_count` should probably accept a `Context` as an argument. #359

Open julien-leclercq opened 2 hours ago

julien-leclercq commented 2 hours ago

Hello,

Thanks for all the nice work, loving it so far, components are really well thought and quite straight forward to integrate.

I encountered a small issue while integrating the table component.

I have the following struct for my table delegate

use gpui::Model; 

struct MyTableDelegate { 
  items: Model<Vec<Item>>
}

I am good for every other methods in the TableDelegate trait, but for rows_count I cannot access the underlying Vec on the fly. As an unsatisfying workaround I could cache the length of the vec in the table delegate, but this seems to be a bit footgun-ish. Holding the raw vector in the TableDelegate is also out of question because I need to be able to access this data in other places and I would rather not clone that as far as I can avoid it.

I am trying it on a fork, I'll provide a PR if it works as intended.

huacnlee commented 1 hour ago

In our use case, the items is not a Model, that just a Vec.

Maybe you can try to change it. There is not need it to be a Model, because the Table is a View, we can update the Vec.

julien-leclercq commented 1 hour ago

Holding the raw vector in the TableDelegate is also out of question because I need to be able to access this data in other places and I would rather not clone that as far as I can avoid it.

That is what I am trying to say here. The table is not the only place where I would need to access the Vec and I don't think it's a good idea for me to move it every time the user wants to change view layout. If you don't want that change, fine by me, I can live with a fork :).

huacnlee commented 1 hour ago

I need think about of this. Maybe we can add cx to those methods. But there have too many method need to change.

huacnlee commented 1 hour ago

You can make a PR first, but you need wait me to think a time.

Change API is ok, but I need consider that is the right change.