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!: Decouple breakwater-parser to make it easier to integrate elsewhere #35

Closed
wants to merge 5 commits into from

Conversation

bits0rcerer
Copy link
Contributor

This pull request hides breakwaters framebuffer implementation behind a trait and decouples breakwater-parser from breakwater-core (in fact reverses the dependency).
This allows other project to use the breakwater magic but implement their own framebuffer.

These change do not introduce dynamic dispatch so runtime performance should™ not be affected.

@sbernauer
Copy link
Owner

sbernauer commented Aug 3, 2024

Hi @bits0rcerer nice to hear from you and thanks for the proposal! Looking forward to a fancy new framebuffer ;)

I think it makes total sense to put Framebuffer behind a trait. However I noticed a few things:

  • tests and benches fail to compile, clippy and rustfmt are failing
  • struct Framebuffer still implements functions which I would like to see on the Trait (fn as_bytes(&self) -> &[u8]; to be precise)
  • Once we added the needed as_bytes function the VNC and ffmpeg sinks can use the Framebuffer trait instead of one specific implementation , so that you can use your own for that. All you need to provide is the as_bytes fn, which I hope you can do.
  • Looking at what remains in breakwater-core I'm wondering if we need it at all. I think I would be in favor of removing it, what would be your opinion?

I did some formatting changes in ebfa821 and made a proposal how the proposed changes above would look in 408aee7.

It would be awesome if you could have a look and provide feedback what you think about that.
The second commit also fixes compilation errors in tests and benches.

@sbernauer sbernauer changed the title Decouple breakwater-parser to make it easier to integrate elsewhere refactor!: Decouple breakwater-parser to make it easier to integrate elsewhere Aug 4, 2024
@sbernauer
Copy link
Owner

Small update: I merged #34 first, as I think this way we will get less merge conflicts.
However, I did address all conflicts in https://github.com/sbernauer/breakwater/tree/decouple-parser-review, so this should not put additional work on your end. Hope that works for you!

@sbernauer
Copy link
Owner

I merged #37 containing your commits, so that I can use the new trait for a feature I'm working on (native windows in addition to VNC). Happy about feedback if the trait works for you!

@sbernauer sbernauer closed this Aug 8, 2024
@bits0rcerer bits0rcerer deleted the decouple-parser branch August 13, 2024 14:48
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