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] - Improve docs and naming for RawWindowHandle functionality #4335

Closed

Conversation

alice-i-cecile
Copy link
Member

@alice-i-cecile alice-i-cecile commented Mar 26, 2022

Objective

Solution

  • Rename HasRawWindowHandleWrapper to ThreadLockedRawWindowHandleWrapper, reflecting the primary distinction
  • Document how this design is intended to be used
  • Leave comments explaining why this design must exist

Migration Guide

  • renamed HasRawWindowHandleWrapper to ThreadLockedRawWindowHandleWrapper

@github-actions github-actions bot added the S-Needs-Triage This issue needs to be labelled label Mar 26, 2022
@alice-i-cecile alice-i-cecile added A-Windowing Platform-agnostic interface layer to run your app in C-Docs An addition or correction to our documentation C-Code-Quality A section of code that is hard to understand or change C-Breaking-Change A breaking change to Bevy's public API that needs to be noted in a migration guide and removed S-Needs-Triage This issue needs to be labelled labels Mar 26, 2022
@alice-i-cecile
Copy link
Member Author

Pulling in @cart directly for a review here as the original author of the code. I'm reasonably confident that I understand the logic behind the design now, but this is tricksy unsafe logic.

crates/bevy_window/src/raw_window_handle.rs Outdated Show resolved Hide resolved
crates/bevy_window/src/raw_window_handle.rs Outdated Show resolved Hide resolved
crates/bevy_window/src/raw_window_handle.rs Outdated Show resolved Hide resolved
Co-authored-by: Nicola Papale <nicopap@users.noreply.github.com>
@cart
Copy link
Member

cart commented May 25, 2022

bors r+

bors bot pushed a commit that referenced this pull request May 25, 2022
# Objective

- As noticed in #4333 by @x-52, the exact purpose and logic of `HasRawWIndowHandleWrapper` is unclear
- Unfortunately, there are rather good reasons why this design is needed (and why we can't just `impl HasRawWindowHandle for RawWindowHandleWrapper` 

## Solution

- Rename `HasRawWindowHandleWrapper` to `ThreadLockedRawWindowHandleWrapper`, reflecting the primary distinction
- Document how this design is intended to be used
- Leave comments explaining why this design must exist


## Migration Guide

- renamed `HasRawWindowHandleWrapper` to `ThreadLockedRawWindowHandleWrapper`
@bors
Copy link
Contributor

bors bot commented May 25, 2022

Build failed:

@cart
Copy link
Member

cart commented May 26, 2022

bors r+

bors bot pushed a commit that referenced this pull request May 26, 2022
# Objective

- As noticed in #4333 by @x-52, the exact purpose and logic of `HasRawWIndowHandleWrapper` is unclear
- Unfortunately, there are rather good reasons why this design is needed (and why we can't just `impl HasRawWindowHandle for RawWindowHandleWrapper` 

## Solution

- Rename `HasRawWindowHandleWrapper` to `ThreadLockedRawWindowHandleWrapper`, reflecting the primary distinction
- Document how this design is intended to be used
- Leave comments explaining why this design must exist


## Migration Guide

- renamed `HasRawWindowHandleWrapper` to `ThreadLockedRawWindowHandleWrapper`
@bors bors bot changed the title Improve docs and naming for RawWindowHandle functionality [Merged by Bors] - Improve docs and naming for RawWindowHandle functionality May 26, 2022
@bors bors bot closed this May 26, 2022
james7132 pushed a commit to james7132/bevy that referenced this pull request Jun 7, 2022
…#4335)

# Objective

- As noticed in bevyengine#4333 by @x-52, the exact purpose and logic of `HasRawWIndowHandleWrapper` is unclear
- Unfortunately, there are rather good reasons why this design is needed (and why we can't just `impl HasRawWindowHandle for RawWindowHandleWrapper` 

## Solution

- Rename `HasRawWindowHandleWrapper` to `ThreadLockedRawWindowHandleWrapper`, reflecting the primary distinction
- Document how this design is intended to be used
- Leave comments explaining why this design must exist


## Migration Guide

- renamed `HasRawWindowHandleWrapper` to `ThreadLockedRawWindowHandleWrapper`
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
…#4335)

# Objective

- As noticed in bevyengine#4333 by @x-52, the exact purpose and logic of `HasRawWIndowHandleWrapper` is unclear
- Unfortunately, there are rather good reasons why this design is needed (and why we can't just `impl HasRawWindowHandle for RawWindowHandleWrapper` 

## Solution

- Rename `HasRawWindowHandleWrapper` to `ThreadLockedRawWindowHandleWrapper`, reflecting the primary distinction
- Document how this design is intended to be used
- Leave comments explaining why this design must exist


## Migration Guide

- renamed `HasRawWindowHandleWrapper` to `ThreadLockedRawWindowHandleWrapper`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Windowing Platform-agnostic interface layer to run your app in C-Breaking-Change A breaking change to Bevy's public API that needs to be noted in a migration guide C-Code-Quality A section of code that is hard to understand or change C-Docs An addition or correction to our documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants