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

Draft prototype using traits #23

Closed
wants to merge 1 commit into from

Conversation

hecrj
Copy link

@hecrj hecrj commented Aug 28, 2019

As discussed in #16, an approach using traits instead of non-exhaustive enums/structs. I also added an example showcasing how a create_surface implementation for MacOS and Linux would look with this approach.

This is probably a controversial change. I think we should dismiss this idea if there is no clear consensus.

@Osspial
Copy link
Contributor

Osspial commented Sep 4, 2019

cc @kvark @Ralith

Looking over the changes for this, I've got a couple points of feedback:

  1. The biggest downside to this is that it makes the documentation significantly less readable. There are probably ways we can compensate for that, but I don't think we can get rustdoc to render this as intuitively as we have set up now.
  2. I can see the current solution for the X11/Wayland backend on Unix being difficult to implement, since it requires auxiliary types for the HasX11Handle and HasWayland handle traits. That won't be a problem for Winit, but I assume it'll be more of a hassle for SDL wrappers.

One solution that I believe would largely keep the benefits of both the current design and this redesign would be to expose all the window-handle-getters in the top-level trait, then conditionally compile each of those functions so that they're only available on their target platform. I'd write up a sketch of that design, but I just looked back through the original thread and... it looks like that's pretty much the original design I proposed. Welp.

@Osspial
Copy link
Contributor

Osspial commented Sep 26, 2019

I'm going to close this, since it's not an unambiguously good change to the codebase. Thank you for pulling this together, though!

@Osspial Osspial closed this Sep 26, 2019
@Osspial Osspial mentioned this pull request Sep 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants