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.

Sign up for GitHub

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

Jump to bottom

Move cursor position to internal state #7988

Merged
merged 2 commits into from
Mar 9, 2023
Merged

Move cursor position to internal state #7988

merged 2 commits into from
Mar 9, 2023

Conversation

Aceeri
Copy link
Member

@Aceeri Aceeri commented Mar 8, 2023

Objective

Fixes #7941

Setting cursor position on initialization probably isn't wanted anyways, but this does hurt discoverability because it moves cursor position to the Window api level. But I think this is overall a win in terms of usability.

Solution

Move cursor position to internal window state so we can use ..default() in Cursor

Changelog

  • Moved cursor position to InternalWindowState

@Aceeri Aceeri requested a review from mockersf March 8, 2023 23:12
@Aceeri Aceeri added 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 X-Controversial There is active debate or serious implications around merging this PR labels Mar 8, 2023
@james7132 james7132 added the M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Mar 9, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Mar 9, 2023

It looks like your PR is a breaking change, but you didn't provide a migration guide.

Could you add some context on what users should update when this change get released in a new version of Bevy?
It will be used to help writing the migration guide for the version. Putting it after a ## Migration Guide will help it get automatically picked up by our tooling.

@lewiszlw
Copy link
Member

lewiszlw commented Mar 9, 2023

I like this solution more than making physical_position field public or creating a big constructor or CursorBuilder.

james7132
james7132 approved these changes Mar 9, 2023
View reviewed changes
Copy link
Member

@james7132 james7132 left a comment

Choose a reason for hiding this comment

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

I agree, this seems like a much more appropriate approach to this.

@james7132 james7132 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 Mar 9, 2023
@james7132 james7132 requested a review from cart March 9, 2023 03:27
@lewiszlw lewiszlw mentioned this pull request Mar 9, 2023
Closed
alice-i-cecile
alice-i-cecile approved these changes Mar 9, 2023
View reviewed changes
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.

Agreed, this is very reasonable.

Removing controversial: we have strong consensus here (at least so far) and the impact is extremely scoped.

@alice-i-cecile alice-i-cecile removed the X-Controversial There is active debate or serious implications around merging this PR label Mar 9, 2023
@alice-i-cecile alice-i-cecile added this to the 0.11 milestone Mar 9, 2023
@james7132 james7132 removed the request for review from cart March 9, 2023 03:33
@Aceeri
Copy link
Member Author

Aceeri commented Mar 9, 2023 •
edited
Loading

@james7132 I'm not sure this is even a breaking change as it has the same api surface I believe.

Since you couldn't access the cursor position directly, only through Window::(physical)_cursor_position/and the respective setters

@james7132
Copy link
Member

Oh yeah you're right. Whoops.

@james7132 james7132 removed the M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Mar 9, 2023
@james7132 james7132 added this pull request to the merge queue Mar 9, 2023
Merged via the queue into bevyengine:main with commit 3cfcc66 Mar 9, 2023
@Aceeri Aceeri deleted the cursor-position-internal branch March 9, 2023 06:15
Shfty pushed a commit to shfty-rust/bevy that referenced this pull request Mar 19, 2023
@Aceeri @Shfty
Move cursor position to internal state (bevyengine#7988)
41c14e8
Shfty pushed a commit to shfty-rust/bevy that referenced this pull request Mar 19, 2023
@Aceeri @Shfty
Move cursor position to internal state (bevyengine#7988)
cfb9a68
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alice-i-cecile alice-i-cecile alice-i-cecile approved these changes

@james7132 james7132 james7132 approved these changes

@mockersf mockersf Awaiting requested review from mockersf

Assignees
No one assigned
Labels
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
Milestone
0.11
Development

Successfully merging this pull request may close these issues.

window::Cursor does not support ..default() initialization pattern
4 participants
@Aceeri @lewiszlw @james7132 @alice-i-cecile