-
Notifications
You must be signed in to change notification settings - Fork 76
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor: TabViewer #24
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think overall this refactor is a big improvement.
As of trait Tab
and DynamicTree
, whether we want to keep them or not depends how we want to achieve having tabs that deal with different types of data. Because in a realistic scenario, like a game engine, this is what would exactly be the case: one tab would be the scene view, another a content browser, etc etc.
Without a DynamicTree
this could be achieved by just storing all of the global state in a struct implementing TabViewer
which then deals with the content of every single tab that can be on the case-by-case basis. This isn't particularly bad as content of each tab can be put in a separate method and then ui
can simply call them in a match tab
block.
Pros:
- Tabs can be easily added and removed dynamically, without having to manually pass the data it needs into it since it already has access to everything.
- Creating a new tab type is easy for the same reason.
Cons:
- Need to update the
match
insideui
when a new kind of tab is created.
On the other hand, the advantage of using DynamicTree
is that each tab can be defined in such a way that it only has access to what it needs. However, what is needed by the tab or not can change in the development process, which means new data needs to be specified and then added to the tab when it's created, and old data needs to be removed. That leads to an overall harder and more annoying to use API.
Because of that, personally I like the idea of not using trait Tab
and DynamicTree
, but at the same time there might be other applications for them that I haven't thought of. So it's probably be safer to keep it for now.
Better to just have the one in the docs, which is tested
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contribution @emilk ❤️
🥳 |
The current
trait Tab
implementation has some limitations:A) We cannot serialize
Tree
:s.B) One must create the tabs up-front with everything they need to show themselves (with
Tab::ui
).The second point in particular impose very weird constraints on users. If a tab wants to modify some global state, it need to be initialized with a
Arc<Mutex<State>>
or similar. I don't like!This PR is a refactor that solves both these problems (and more!?).
The approach is this: instead of storing a
Box<dyn Tab>
inTree
, we instead makeTree
generic over what the user wants to identify a tab (Tree<Tab>
). For instance, a user may identify each tab by a UUID or similar. Then we use a newtrait TabView
which actually shows the tabs, fetches their titles, handles close events, and so on.The old
trait Tab
approach can still be implemented on top of this implementation, if one so desires.In general, I think this is a better approach to a generic library. I'm not sure if we should even keep the old
trait Tab
, or let users define that themselves.TODO
trait Tab
, and if so if we should add some helpers to make make the use of it similar to beforeOther changes in this PR:
struct TabIndex(pub usize)
for better typesafety (and self-documenting code).Tree::find_tab
Tree::set_active_tab
Tree::set_focused
toTree::set_focused_node
.