Skip to content
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: tabs rewrite to clean up API #1324

Merged
merged 1 commit into from
Dec 3, 2023
Merged

Conversation

ThomasFrans
Copy link
Contributor

Tabs relied heavily on ViewExt's title() function, while also requiring a separate id. The id, if used, should be an internal part of the struct and not part of its API. This also removes the hashmap as it will never be faster than sequentially looking up all the names, since there will most likely never be that many tabs in a TabbedView.

Tabs relied heavily on `ViewExt`'s `title()` function, while also
requiring a separate `id`. The `id`, if used, should be an internal part
of the struct and not part of its API. This also removes the hashmap as
it will never be faster than sequentially looking up all the names,
since there will most likely never be that many tabs in a `TabbedView`.
@ThomasFrans
Copy link
Contributor Author

ThomasFrans commented Oct 27, 2023

I've been wondering whether the ViewExt trait could be removed from the code as it's not being used for much but it has to be implemented everywhere. I just created this PR while I was looking around in the code what could be removed or rewritten without ViewExt. This already removes the dependency of tabs on ViewExt::title() as it doesn't make sense to force every view to have a title, especially when there is already NamedView from cursive and most views don't need a title. This PR also does some general cleanup, adds some documentation and shrinks the binary with 8Kb :p

@ThomasFrans
Copy link
Contributor Author

About the `ViewExt` thing

For the ViewExt thing, I'm looking what other UI frameworks to to make keyboard shortcuts configurable, as that was probably the major reason for going with that approach. I've been looking at libraries like ratatui and relm and they also use these 'commands', but from what I understand, they are limited to the widget itself and not sent and 'handlable' by all the widgets in the tree. I think it might make more sense to remove the ViewExt and use a big hashmap with keymaps, where every view does its own translation of the keyboard keys. I haven't experimented with this, but I think it might possibly make the code easier to read. It would maybe not be strongly typed, but there might be solutions for that.

In general it's just a bit weird that a tab can also handle Play for example, which is how it's currently implemented. Of course a tab wouldn't actually handle the Play command, but it looks like it does from the way the code is structured. The overall goal would only be to make the code more readable.

@hrkfdn
Copy link
Owner

hrkfdn commented Dec 3, 2023

Thanks, looks good to me.

Regarding ViewExt:

For the ViewExt thing, I'm looking what other UI frameworks to to make keyboard shortcuts configurable, as that was probably the major reason for going with that approach.

It's been a while, but yes, I think that was it.

@hrkfdn hrkfdn merged commit 0a1a9bd into hrkfdn:main Dec 3, 2023
5 checks passed
@ThomasFrans ThomasFrans deleted the tabs-rewrite branch December 3, 2023 13:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants