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

reverse the dependency between helix-tui and helix-view #366

Conversation

Kethku
Copy link
Contributor

@Kethku Kethku commented Jun 24, 2021

Does step one of #39 (comment)

"helix-view needs to be refactored to have it's own generic key/style/color structs. Fix previous broken refactor key into helix-view #345 is a start but it depends on crossterm. Using generic structs would let us drop the crossterm dependency from helix-view."

I did this by lifting CursorKind, Margin, Rect, Color, Modifier, and Style into helix-view. I then reversed the dependency making helix-tui depend on helix-view rather than the other way around. I also made helix-view's dependency on crossterm optional in order to allow impl From<crossterm::style::Color> on the generic Color type.

Your comment suggested that the lifted types should be "generic". I think the types in the tui crate were pretty generic, but if you have ideas for how these should be modified to make them more "generic", I'd be happy to do it. I just didn't know what you were thinking.

Let me know if this is what you had in mind, it didn't take too long to do, and I'm looking to do these types of refactorings as a way to get to know the codebase better, so I'm not at all attached to this particular PR. Just trying to pitch in.

@archseer
Copy link
Member

Note: this probably conflicts #345. Could you maybe copy over the changes from there so we subsume that PR?

I think this is a pretty good idea. It initially wasn't possible because we depended on tui-rs externally, but now that it's forked as helix-tui it's doable. I have a crossterm->termwiz port in progress that will remove even more code from helix-tui so we'll likely be able to drop the crate completely: https://github.com/helix-editor/helix/tree/termwiz

@Kethku
Copy link
Contributor Author

Kethku commented Jun 24, 2021

Yes I will copy the changes from #345 over to this PR sometime tomorrow as its late today.

@archseer
Copy link
Member

No rush :)

@Kethku Kethku force-pushed the dev/kethku/remove-tui-dependency-from-helix-view branch from 76806e0 to 3e0b7fe Compare June 24, 2021 22:35
@Kethku
Copy link
Contributor Author

Kethku commented Jun 24, 2021

Rebased on the merged keybinding PR. Also fixed a bunch of tests. This is ready for review

@Kethku
Copy link
Contributor Author

Kethku commented Jun 24, 2021

I had to pull the KeyCode and KeyModifiers types out of crossterm to be compatible with the previous PR. I did the same thing I did with the tui types and added from and into implementations to go back and forth between the originals.

Copy link
Member

@archseer archseer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great, thanks!

Comment on lines +13 to +14
default = []
term = ["crossterm"]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍🏻

@archseer archseer merged commit 4418e17 into helix-editor:master Jun 25, 2021
cessen pushed a commit to cessen/helix that referenced this pull request Jun 26, 2021
…#366)

* reverse the dependency between helix-tui and helix-view by moving a fiew types to view

* fix tests

* clippy and format fixes

Co-authored-by: Keith Simmons <keithsim@microsoft.com>
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.

None yet

3 participants