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

Implement Clone for 'static events #1478

Merged
merged 3 commits into from
May 15, 2020

Conversation

semtexzv
Copy link
Contributor

  • Tested on all platforms changed
  • Compilation warnings were addressed
  • cargo fmt has been run on this branch
  • cargo doc builds successfully
  • Added an entry to CHANGELOG.md if knowledge of this change could be valuable to users
  • Updated documentation to reflect any user-facing changes, including notes of platform-specific behavior
    Not applicable:
  • Created or updated an example program if it would help users understand this functionality
  • Updated feature matrix, if new features were added or implemented

There is no reason why 'static events can't be Clone. This unblocks most use cases for cloning safe events while not changing current design significantly.

Blocking WASM + OpenGL implementation for Amethyst ( See amethyst/amethyst#2040)

@semtexzv semtexzv mentioned this pull request Feb 21, 2020
15 tasks
@semtexzv semtexzv requested review from ryanisaacg and Osspial and removed request for ryanisaacg February 22, 2020 10:17
@est31
Copy link
Contributor

est31 commented Feb 24, 2020

Ideally, we should wait for #1456 and then just add a derive impl.

@semtexzv
Copy link
Contributor Author

I agree, current design is not ideal, but if no other approach is found, this is a way to get Clone safely for events.

src/event.rs Outdated Show resolved Hide resolved
@azriel91
Copy link
Contributor

azriel91 commented May 2, 2020

Heya, is it okay to get this merged (and perhaps published) before a big Event refactor?

Since a refactor would be a breaking change anyway, I don't see any harm in allowing downstream projects to depend on a published version with this change, with the prospect that they have to migrate to the new Event structure (which they have to do anyway).

Copy link
Contributor

@Osspial Osspial left a comment

Choose a reason for hiding this comment

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

Sorry about the delay. This looks good to merge - thanks for pulling it together!

@Osspial Osspial merged commit 878c179 into rust-windowing:master May 15, 2020
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.

5 participants