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

Accepts first mouse #2457

Closed
wants to merge 10 commits into from
Closed

Accepts first mouse #2457

wants to merge 10 commits into from

Conversation

mgax
Copy link

@mgax mgax commented Sep 2, 2022

  • Tested on all platforms changed
  • 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
  • Created or updated an example program if it would help users understand this functionality
  • Updated feature matrix, if new features were added or implemented

Quoting #1882 (review):

At some point we should consider making it user-configurable, but let's wait until someone comes with a concrete use-case!

The use case is for Neovide (neovide/neovide#1343), so it behaves more like a native MacOS application.

@mgax mgax requested a review from madsmtm as a code owner September 2, 2022 11:55
@mgax
Copy link
Author

mgax commented Sep 2, 2022 •
edited
Loading

CI is failing with a type mismatch between bool and BOOL. I've added as BOOL to the return value; I hope it's the right thing to do, and the unsafe block isn't hiding anything unsafe :)

Edit: I'm running on arm64 where it seems bool and BOOL are the same type, but the CI is running on x86_64.

madsmtm
madsmtm reviewed Sep 2, 2022
View reviewed changes
Copy link
Member

@madsmtm madsmtm left a comment

Choose a reason for hiding this comment

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

This looks good, thanks!

If you don't mind, I think I'll wait with this 'till #2427 is done, at which point the bool stuff should also be easier.

@mgax
Copy link
Author

mgax commented Sep 2, 2022

If you don't mind, I think I'll wait with this 'till #2427 is done, at which point the bool stuff should also be easier.

I don't mind. But it's sorted out – at least, the CI is happy :)

@mgax
Copy link
Author

mgax commented Sep 6, 2022

I've updated the branch. The new #[sel(acceptsFirstMouse:)] syntax is pretty neat.

I think the tests failed because of GitHub; one just says Error: Internal Server Error. I'd re-run it but I don't have permission. @madsmtm can you trigger a re-run? Or should I just push an empty commit?

madsmtm
madsmtm requested changes Sep 8, 2022
View reviewed changes
Copy link
Member

@madsmtm madsmtm left a comment

Choose a reason for hiding this comment

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

Apologies, I should have been more clear, I wasn't actually ready with #2427 yet.

Could I get you to merge the latest master, and then move accepts_first_mouse from ViewState to WinitView (would do it myself, but I don't have write access to your branch).

@mgax
Copy link
Author

mgax commented Sep 9, 2022

Apologies, I should have been more clear, I wasn't actually ready with #2427 yet.

No worries! I'm not used to working with Rust or Objective-C and it's fun to follow along while you refactor the internals :)

I hope I got it right with storing the flag on SharedState, that's what other flags seem to be doing.

madsmtm
madsmtm requested changes Sep 9, 2022
View reviewed changes
src/platform_impl/macos/window.rs Outdated Show resolved Hide resolved
madsmtm
madsmtm requested changes Sep 12, 2022
View reviewed changes
Copy link
Member

@madsmtm madsmtm left a comment

Choose a reason for hiding this comment

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

Wonderful, only small nits

src/platform_impl/macos/view.rs Outdated Show resolved Hide resolved
src/platform_impl/macos/view.rs Outdated Show resolved Hide resolved
src/platform_impl/macos/window.rs Outdated Show resolved Hide resolved
@madsmtm madsmtm mentioned this pull request Sep 12, 2022
Closed
@mgax mgax requested a review from madsmtm September 13, 2022 14:10
madsmtm
madsmtm approved these changes Sep 13, 2022
View reviewed changes
Copy link
Member

@madsmtm madsmtm left a comment

Choose a reason for hiding this comment

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

Thanks!

madsmtm added a commit that referenced this pull request Sep 13, 2022
@madsmtm
Accepts first mouse (#2457)
48b843e 
* MacOS: set value for `accepts_first_mouse`

* Update CHANGELOG and FEATURES

* Field doesn't need to be public

* Convert `bool` to `BOOL`

* Fix formatting

* Move flag from window state to view instance

* Feedback from PR

* Fix changelog location
@madsmtm
Copy link
Member

madsmtm commented Sep 13, 2022

I had to fix the changelog entry beforehand but still didn't seem to have access to the branch, idk. maybe I'm doing something wrong, so ended up merging this manually in 48b843e instead.

@madsmtm madsmtm closed this Sep 13, 2022
@madsmtm madsmtm reopened this Sep 13, 2022
@madsmtm madsmtm closed this Sep 13, 2022
@mgax mgax deleted the accepts-first-mouse branch September 14, 2022 10:53
@mgax
Copy link
Author

mgax commented Sep 14, 2022

@madsmtm thanks for seeing this through!

@mgax mgax mentioned this pull request Sep 21, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@madsmtm madsmtm madsmtm approved these changes

Assignees
No one assigned
Labels
DS - macos
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@mgax @madsmtm