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

Macos fullscreen & dialog support with run_return #1581

Merged
merged 15 commits into from
Jun 9, 2020
Merged

Macos fullscreen & dialog support with run_return #1581

merged 15 commits into from
Jun 9, 2020

Conversation

VZout
Copy link
Contributor

@VZout VZout commented May 28, 2020

Previously the run_return example would freeze if you switch to fullscreen. This fixes it.
It is not a clean solution but if anybody knows more about mac than I do let me know if there is a better way.

  • Tested on all platforms changed
  • Compilation warnings were addressed
  • cargo fmt has been run on this branch
  • cargo doc builds successfully
  • 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

@VZout VZout changed the title Mac fullscreen Macos fullscreen support with run_return May 28, 2020
@VZout VZout marked this pull request as draft May 28, 2020 15:42
@VZout
Copy link
Contributor Author

VZout commented May 28, 2020

Rewrote my approach since I noticed I caused a bug with the control flow.
Should work as expected now.

@VZout VZout marked this pull request as ready for review June 2, 2020 14:50
@VZout
Copy link
Contributor Author

VZout commented Jun 4, 2020

Also added experimental dialog/child window support tested with https://github.com/EmbarkStudios/nfd2.

@VZout VZout changed the title Macos fullscreen support with run_return Macos fullscreen & dialog support with run_return Jun 4, 2020
@VZout
Copy link
Contributor Author

VZout commented Jun 5, 2020

@kchibisov @francesca64 You two originally authored the run_return implementation for Macos. What do you two think about this issue?

@kchibisov
Copy link
Member

I don't have native macOS machine and don't know much about macOS. The run_return impl is a copypasta of run impl. I'm not sure If I ever seen a freeze from run_return, since we're using it for so much time in alacritty, and no one ever reported it. Maybe I don't understand the conditions where it should freeze?

@VZout
Copy link
Contributor Author

VZout commented Jun 5, 2020

@kchibisov So its 2 issues. 1 is running the window_run_return sample and pressing the green fullscreen button on mac. It completely freezes the app. The second is that opening dialogs using nfd2 also freezes the app.

This PR solves both but not in a very clean way.

@francesca64
Copy link
Member

@VZout while I wrote the macOS EL2 backend, I never included a run_return implementation, since I felt it wouldn't be idiomatic on macOS and that there'd likely be complications.

My perspective might be poisoned from having to write so many workarounds in my life, but I honestly don't think this looks that unclean. While a canonical solution could exist, it'd likely involve a considerable amount of research into what other windowing implementations are doing.

@Jasper-Bekkers
Copy link
Contributor

@VZout while I wrote the macOS EL2 backend, I never included a run_return implementation, since I felt it wouldn't be idiomatic on macOS and that there'd likely be complications.

For various reasons we can't have winit really take control of our application loop like that. It would be a welcome addition to have run_return available on all platforms that would support it.

I also don't think this code is super unclean, so I'd love to see this get merged into mainline.

@francesca64
Copy link
Member

@Jasper-Bekkers

It would be a welcome addition to have run_return available on all platforms that would support it.

I believe that's already the current story.

@vbogaevsky are you around for an additional review?

Copy link
Member

@francesca64 francesca64 left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

@VZout
Copy link
Contributor Author

VZout commented Jun 9, 2020

I've fixed a minor issue. A colleague noticed that maocs dialogs were not being closes properly and always exited with the canceled event.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants