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

Fix iOS app configuration missing certain specifications #31162

Merged
merged 3 commits into from
Dec 19, 2024

Conversation

frenzibyte
Copy link
Member

After having this one simmer for some time while working on other tasks that require UIKit's document-related components, the specification in this PR here should be correct for what osu! is as an iOS app.

osu! is not a document-based app, contrary to #7443 (comment). None of its files should be accessible to the user, not even logs, that's the last kind of data I would expose to a user that wants to play a game on their phone. The only kind of files that can be exposed to the user is exported beatmaps/skins/scores, and even then that doesn't justify it to be a document-based app at all.

osu! should not support opening documents in place either, there is nothing in the game that requires opening documents in the user's storage and write to it.

In conclusion, to address ITMS-90737:

  • UISupportsDocumentBrowser should not be set to YES as osu! is not a document-based app
  • LSSupportsOpeningDocumentsInPlace should be set to NO as osu! does not open documents in place.

Tested the iOS file selector/presenter implementations to still work with this key set.

@smoogipoo
Copy link
Contributor

smoogipoo commented Dec 18, 2024

These days we also get:

ITMS-90683: Missing purpose string in Info.plist - Your app’s code references one or more APIs that access sensitive user data, or the app has one or more entitlements that permit such access. The Info.plist file for the “osu.iOS.app” bundle should contain a NSBluetoothAlwaysUsageDescription key with a user-facing purpose string explaining clearly and completely why your app needs the data. If you’re using external libraries or SDKs, they may reference APIs that require a purpose string. While your app might not use these APIs, a purpose string is still required. For details, visit: https://developer.apple.com/documentation/uikit/protecting_the_user_s_privacy/requesting_access_to_protected_resources.

Probably worth fixing here too, if possible.

@frenzibyte
Copy link
Member Author

It's weird that we require bluetooth permissions at all, and I'm not sure what kind of message would satisify this warning and not have the game being rejected so I investigated it a little. Turns out it was SDL referencing CoreBluetooth via an API it provides for HID support (named HIDAPI). To avoid unnecessary hassles, I've took the time to disable CoreBluetooth from SDL instead, as in ppy/SDL3-CS#188.

This also involved removing iOS from ppy/SDL2-CS completely, as SDL2 also references CoreBluetooth and I have no idea how to disable it there (I tried doing what I did in SDL3 but no go). PR: ppy/SDL2-CS#197

smoogipoo
smoogipoo previously approved these changes Dec 18, 2024
@smoogipoo
Copy link
Contributor

Do we lose anything by excluding hidapi? For example external keyboard support, apple pen support?

If so, maybe best to just state that in the requirement rather than disabling?

@smoogipoo smoogipoo dismissed their stale review December 18, 2024 14:27

this was a test approval

@frenzibyte
Copy link
Member Author

frenzibyte commented Dec 18, 2024

I don't think SDL's HIDAPI stuff has anything to do with functionalities already provided by Apple's native frameworks itself (hardware keyboard via GCKeyboard and apple pen via multiple UIKit APIs), but I admit I haven't tested this to be 101% sure so I'll do to confirm.

The only kind of usage I can imagine is OTD if they rely on SDL's HIDAPI and/or use CoreBluetooth. If you think that's enough reason to not disable then sure.

@smoogipoo
Copy link
Contributor

smoogipoo commented Dec 18, 2024

I believe they're doing their own stuff. That said I think the simplest path might just be to say we don't use these resources? In-case someone does use them and wonders why we don't include them in our SDL-CS releases.

If you’re using external libraries or SDKs, they may reference APIs that require a purpose string. While your app might not use these APIs, a purpose string is still required.

It's a bit wishy-washy but seems to indicate "we don't actually use this" is somewhat of an appropriate response.

@peppy
Copy link
Member

peppy commented Dec 18, 2024

As long as it doesn't end up in a user facing popup. These strings generally do, in my understanding, when they are actually accessed.

That said, trimming our SDL releases down to only what we use seems like a good direction regardless?

@smoogipoo
Copy link
Contributor

AFAICR our testflight build doesn't have any popups. If we can get away with not trimming and also not have a popup show, then that's a win-win in my book, given that these projects have external consumers. https://github.com/ppy/SDL2-CS/network/dependents https://github.com/ppy/SDL3-CS/network/dependents

@smoogipoo
Copy link
Contributor

Here's a similar issue for reference: https://stackoverflow.com/a/58009150

@frenzibyte
Copy link
Member Author

Okay, my main intention was to find which API was using CoreBluetooth so as to make sure we're not hitting a code path that uses it. Now that we're aware where it's being used and it's something that we would never hit, I'll go ahead with a "we don't use this" string for now and hope it doesn't mess up the app store review process.

@peppy peppy merged commit 239dc7d into ppy:master Dec 19, 2024
10 checks passed
@smoogipoo
Copy link
Contributor

Shall we close SDL-CS PRs then?

@frenzibyte frenzibyte deleted the ios-osu-is-not-document-app branch December 19, 2024 10:05
@frenzibyte
Copy link
Member Author

Yep.

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

Successfully merging this pull request may close these issues.

iOS app submission guideline regression
3 participants