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

[Merged by Bors] - Derived Copy trait for bevy_input events, Serialize/Deserialize for events in bevy_input and bevy_windows, PartialEq for events in both, and Eq where possible in both. #6023

Closed
wants to merge 10 commits into from

Conversation

targrub
Copy link
Contributor

@targrub targrub commented Sep 19, 2022

Objective

Add traits to events in bevy_input and bevy_windows: Copy, Serialize/Deserialize, PartialEq, and Eq, as requested in #6022, #6023, #6024.

Solution

Added the traits to events in bevy_input and bevy_windows. Added dependency of serde in Cargo.toml of bevy_input.

Migration Guide

If one has been .clone()'ing bevy_input events, Clippy will now complain about that. Just remove .clone() to solve.

Other Notes

Some events in bevy_input had f32 fields, so Eq trait was not derived for them.
Some events in bevy_windows had String fields, so Copy trait was not derived for them.

@targrub targrub changed the title Derived Copy trait for bevy_input events. Derived Copy trait for bevy_input events, Serialize/Deserialize for events in bevy_input and bevy_windows, PartialEq for events in both, and Eq where possible in both. Sep 19, 2022
…erde an optional dependency. Added conditional #derives for necessary types in bevy_window.
@mockersf
Copy link
Member

now you need to add bevy_window/serialize to

serialize = ["bevy_input/serialize"]

@alice-i-cecile alice-i-cecile added A-Input Player input via keyboard, mouse, gamepad, and more A-Windowing Platform-agnostic interface layer to run your app in C-Usability A targeted quality-of-life change that makes Bevy easier to use labels Sep 20, 2022
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

All the bevy_window events need PartialEq impls :) The general strategy LGTM now though.

Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

Awesome, LGTM now!

@alice-i-cecile alice-i-cecile added this to the Bevy 0.9 milestone Sep 20, 2022
@targrub
Copy link
Contributor Author

targrub commented Sep 20, 2022

This PR's code should have ended with 29ebbd9

Copy link
Contributor

@bzm3r bzm3r left a comment

Choose a reason for hiding this comment

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

Looks good to me! I just have one question.

@@ -4,7 +4,8 @@ use super::{WindowDescriptor, WindowId};
use bevy_math::{IVec2, Vec2};

/// A window event that is sent whenever a window's logical size has changed.
#[derive(Debug, Clone)]
#[derive(Debug, Clone, PartialEq)]
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to derive Copy for the items in this file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A number of them have String or PathBuf fields, so Copy cannot be derived for those.

@bzm3r
Copy link
Contributor

bzm3r commented Sep 20, 2022

Is it worth deriving Hash too, right now, or should that be left for a separate issue? For bevy_input items in particular, I can see that being useful.

EDIT: nvm, just saw this: #6024 (comment)

@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Sep 20, 2022
Copy link
Contributor

@bzm3r bzm3r left a comment

Choose a reason for hiding this comment

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

Looks good to me! (Leaving an actual approval this time!)

@alice-i-cecile
Copy link
Member

bors r+

bors bot pushed a commit that referenced this pull request Sep 20, 2022
…e` for events in `bevy_input` and `bevy_windows`, `PartialEq` for events in both, and `Eq` where possible in both. (#6023)

# Objective

Add traits to events in `bevy_input` and `bevy_windows`: `Copy`, `Serialize`/`Deserialize`, `PartialEq`, and `Eq`, as requested in #6022, #6023, #6024.

## Solution

Added the traits to events in `bevy_input` and `bevy_windows`.  Added dependency of `serde` in `Cargo.toml` of `bevy_input`.


## Migration Guide

If one has been `.clone()`'ing `bevy_input` events, Clippy will now complain about that.  Just remove `.clone()` to solve.

## Other Notes

Some events in `bevy_input` had `f32` fields, so `Eq` trait was not derived for them.
Some events in `bevy_windows` had `String` fields, so `Copy` trait was not derived for them.

Co-authored-by: targrub <62773321+targrub@users.noreply.github.com>
@bors
Copy link
Contributor

bors bot commented Sep 20, 2022

Build failed:

@alice-i-cecile
Copy link
Member

Flaky failure, see #6036

bors retry

bors bot pushed a commit that referenced this pull request Sep 20, 2022
…e` for events in `bevy_input` and `bevy_windows`, `PartialEq` for events in both, and `Eq` where possible in both. (#6023)

# Objective

Add traits to events in `bevy_input` and `bevy_windows`: `Copy`, `Serialize`/`Deserialize`, `PartialEq`, and `Eq`, as requested in #6022, #6023, #6024.

## Solution

Added the traits to events in `bevy_input` and `bevy_windows`.  Added dependency of `serde` in `Cargo.toml` of `bevy_input`.


## Migration Guide

If one has been `.clone()`'ing `bevy_input` events, Clippy will now complain about that.  Just remove `.clone()` to solve.

## Other Notes

Some events in `bevy_input` had `f32` fields, so `Eq` trait was not derived for them.
Some events in `bevy_windows` had `String` fields, so `Copy` trait was not derived for them.

Co-authored-by: targrub <62773321+targrub@users.noreply.github.com>
@bors bors bot changed the title Derived Copy trait for bevy_input events, Serialize/Deserialize for events in bevy_input and bevy_windows, PartialEq for events in both, and Eq where possible in both. [Merged by Bors] - Derived Copy trait for bevy_input events, Serialize/Deserialize for events in bevy_input and bevy_windows, PartialEq for events in both, and Eq where possible in both. Sep 20, 2022
@bors bors bot closed this Sep 20, 2022
alice-i-cecile added a commit to alice-i-cecile/bevy that referenced this pull request Oct 4, 2022
…e` for events in `bevy_input` and `bevy_windows`, `PartialEq` for events in both, and `Eq` where possible in both. (bevyengine#6023)

Add traits to events in `bevy_input` and `bevy_windows`: `Copy`, `Serialize`/`Deserialize`, `PartialEq`, and `Eq`, as requested in bevyengine#6022, bevyengine#6023, bevyengine#6024.

Added the traits to events in `bevy_input` and `bevy_windows`.  Added dependency of `serde` in `Cargo.toml` of `bevy_input`.

If one has been `.clone()`'ing `bevy_input` events, Clippy will now complain about that.  Just remove `.clone()` to solve.

Some events in `bevy_input` had `f32` fields, so `Eq` trait was not derived for them.
Some events in `bevy_windows` had `String` fields, so `Copy` trait was not derived for them.

Co-authored-by: targrub <62773321+targrub@users.noreply.github.com>
james7132 pushed a commit to james7132/bevy that referenced this pull request Oct 19, 2022
…e` for events in `bevy_input` and `bevy_windows`, `PartialEq` for events in both, and `Eq` where possible in both. (bevyengine#6023)

# Objective

Add traits to events in `bevy_input` and `bevy_windows`: `Copy`, `Serialize`/`Deserialize`, `PartialEq`, and `Eq`, as requested in bevyengine#6022, bevyengine#6023, bevyengine#6024.

## Solution

Added the traits to events in `bevy_input` and `bevy_windows`.  Added dependency of `serde` in `Cargo.toml` of `bevy_input`.


## Migration Guide

If one has been `.clone()`'ing `bevy_input` events, Clippy will now complain about that.  Just remove `.clone()` to solve.

## Other Notes

Some events in `bevy_input` had `f32` fields, so `Eq` trait was not derived for them.
Some events in `bevy_windows` had `String` fields, so `Copy` trait was not derived for them.

Co-authored-by: targrub <62773321+targrub@users.noreply.github.com>
james7132 pushed a commit to james7132/bevy that referenced this pull request Oct 28, 2022
…e` for events in `bevy_input` and `bevy_windows`, `PartialEq` for events in both, and `Eq` where possible in both. (bevyengine#6023)

# Objective

Add traits to events in `bevy_input` and `bevy_windows`: `Copy`, `Serialize`/`Deserialize`, `PartialEq`, and `Eq`, as requested in bevyengine#6022, bevyengine#6023, bevyengine#6024.

## Solution

Added the traits to events in `bevy_input` and `bevy_windows`.  Added dependency of `serde` in `Cargo.toml` of `bevy_input`.


## Migration Guide

If one has been `.clone()`'ing `bevy_input` events, Clippy will now complain about that.  Just remove `.clone()` to solve.

## Other Notes

Some events in `bevy_input` had `f32` fields, so `Eq` trait was not derived for them.
Some events in `bevy_windows` had `String` fields, so `Copy` trait was not derived for them.

Co-authored-by: targrub <62773321+targrub@users.noreply.github.com>
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
…e` for events in `bevy_input` and `bevy_windows`, `PartialEq` for events in both, and `Eq` where possible in both. (bevyengine#6023)

# Objective

Add traits to events in `bevy_input` and `bevy_windows`: `Copy`, `Serialize`/`Deserialize`, `PartialEq`, and `Eq`, as requested in bevyengine#6022, bevyengine#6023, bevyengine#6024.

## Solution

Added the traits to events in `bevy_input` and `bevy_windows`.  Added dependency of `serde` in `Cargo.toml` of `bevy_input`.


## Migration Guide

If one has been `.clone()`'ing `bevy_input` events, Clippy will now complain about that.  Just remove `.clone()` to solve.

## Other Notes

Some events in `bevy_input` had `f32` fields, so `Eq` trait was not derived for them.
Some events in `bevy_windows` had `String` fields, so `Copy` trait was not derived for them.

Co-authored-by: targrub <62773321+targrub@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Input Player input via keyboard, mouse, gamepad, and more A-Windowing Platform-agnostic interface layer to run your app in C-Usability A targeted quality-of-life change that makes Bevy easier to use S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants