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

IOS ScreenEdge Corrected Default #1843

Closed
wants to merge 1 commit into from
Closed

IOS ScreenEdge Corrected Default #1843

wants to merge 1 commit into from

Conversation

bigmstone
Copy link

@bigmstone bigmstone commented Jan 27, 2021

With IOS' PlatformSpeccificWindowBuilderAttributes the
preferred_screen_edges_deferring_system_gestures value was previously
set to the default (u8 default). This was an invalid value and caused
different crashes under screen edge events. Changing this value to a
valid value from the ScreenEdge enum fixes these issues.

Fixes #1705 and fixes #1613

  • 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

With IOS' PlatformSpeccificWindowBuilderAttributes the
preferred_screen_edges_deferring_system_gestures value was previously
set to the default (u8 default). This was an invalid value and caused
different crashes under screen edge events. Changing this value to a
valid value from the ScreenEdge enum fixes these issues.
@bigmstone
Copy link
Author

bigmstone commented Jan 27, 2021

I have this as draft while I get more 👀 on it. It's a fairly low risk change for the repo since it only affects IOS and has a pretty obvious/easy to understand outcome. I have tested it on iPhone 12 pro or whatever they're calling it these days with iOS 14.3 and soon to be 14.4. I can't test on simulator on my mac since it's M1 and that workflow is broken right now (last I put any effort into it) w/ Rust and cargo-lipo. If anyone else can test that would be great. If not I can take this out of draft if maintainers are 👍 for that.

@MichaelHills
Copy link
Contributor

Nice, works for me my iPhone 11 on Bevy. Changing it to ScreenEdge::NONE works too. Why is the derived default invalid?

@bigmstone
Copy link
Author

I am somewhat forced to guess why since we don't have the source for the portion of code that triggers the issue. It's entirely possible that this is a bug on Apple's side. Before I take this out of draft I need to create a blank view purely in objective-c or swift with UIRectEdge set to 0. Also my description/title in the PR actually needs to be changed a bit. Since the default value of u8 is 0 it's still a valid value of UIRectEdge. The Apple code is opaque -- so I haven't found a clear understanding yet of why this value results in an index offset in what appears to be they system gesture handler. Happy to have some extra context there if anyone has it.

@bigmstone
Copy link
Author

Thinking through this a bit more I think this PR is only masking the problem. Not solving it. Without this change I notice that if I touch on a non-edge then later edge touches are handled correctly. As though something was uninitialized and that first touch initializes things. In my search for a solution to this problem I looked a bit at the touch handler but didn't see anything obvious that stood out as being initialized there. This change is essentially making all of the initial touches (even those on the edge) handled by winit first. Then deferred to the system. Making my hypothesis that something is uninitialized stronger.

Anyone have thoughts on what on the winit side is either uninitialized or a call we make to initialize something when a touch is handled?

@bigmstone
Copy link
Author

@MichaelHills changing to NONE doesn't work on my end. I overlooked that part of your comment somehow at first. It fails in the same way. Can you double check that on your end?

@MichaelHills
Copy link
Contributor

I'll try to reproduce the original problem again tonight using Default::default() and verify

@tachyon-ops
Copy link
Contributor

This PR fixes my iOS crashes whenever I touch my app.

@madsmtm
Copy link
Member

madsmtm commented Aug 6, 2021

Default::default() returns ScreenEdge::NONE, see the relevant bitflags documentation together with the the ScreenEdge definition - so it's not because it's an invalid value!

So as you said, this PR is only masking the problem, but nice find, it gets us closer to the root of the problem!

The documentation on preferredScreenEdgesDeferringSystemGestures says:

Whenever possible, you should allow the system gestures to take precedence. However, immersive apps can use this property to allow app-defined gestures to take precedence over the system gestures.

So I'm weary against changing the default to making the application take precedence, but if we can't find the root cause it could be a temporary solution until we do?

@bigmstone
Copy link
Author

I ended up discovering the same docs back when I was looking at this too and reached the same conclusion -- the default behavior is what you'd expect "out of the box" from winit or similar. I wasn't able to find a clear smoking gun on this one. Ended up just using the release version of winit with the config for ScreenEdge set. I'm of the mind that I should close this PR. The discussion is likely useful to help whomever ends up solving it, but it should be linked in the open issues tracking this.

@bigmstone bigmstone closed this Aug 11, 2021
DAddYE added a commit to DAddYE/winit that referenced this pull request Sep 29, 2021
Currently whenever I touch the screen the app crashes.

Note, that this isn't the real culprit but rather a workaround, for more info see:
rust-windowing#1843
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5 participants