Skip to content

[Merged by Bors] - Improve docs and naming for RawWindowHandle functionality#4335

Closed
alice-i-cecile wants to merge 4 commits intobevyengine:mainfrom
alice-i-cecile:rawest-of-window-handles
Closed

[Merged by Bors] - Improve docs and naming for RawWindowHandle functionality#4335
alice-i-cecile wants to merge 4 commits intobevyengine:mainfrom
alice-i-cecile:rawest-of-window-handles

Conversation

@alice-i-cecile
Copy link
Copy Markdown
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 M-Migration-Guide 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 alice-i-cecile requested a review from cart March 26, 2022 01:04
@alice-i-cecile
Copy link
Copy Markdown
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.

@cart
Copy link
Copy Markdown
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
Copy Markdown
Contributor

bors bot commented May 25, 2022

Build failed:

@cart
Copy link
Copy Markdown
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-Code-Quality A section of code that is hard to understand or change C-Docs An addition or correction to our documentation M-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants