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

[SDK-697] Feature/remove permission override #26

Merged
merged 6 commits into from
Jan 3, 2024

Conversation

HarrisonHough
Copy link
Collaborator

@HarrisonHough HarrisonHough commented Dec 5, 2023

SDK-697

Description

  • This PR includes changes to prevent forcing automatic manifest generation (for device permissions). It hides it now instead behind a define symbol.
    RPM_WEBVIEW_PERMISIONS
    With this update it will be disabled by default but they can re-enable by adding the above define symbol into player settings.
    This will help developers when submitting to marketplaces that don't allow camera or other specific permissions.

Changes

Added

  • List your additions and new features here.

Updated

  • List your updates and changes here.

Removed

  • List what is removed here.

How to Test

  • Add steps to locally test these changes

Checklist

  • Tests written or updated for the changes.
  • Documentation is updated.
  • Changelog is updated.

@HarrisonHough HarrisonHough requested a review from a team December 5, 2023 09:54
@HarrisonHough HarrisonHough requested a review from rYuuk as a code owner December 5, 2023 09:54
@@ -1,4 +1,4 @@
#if UNITY_EDITOR
#if UNITY_EDITOR && RPM_WEBVIEW_PERMISIONS
Copy link
Contributor

Choose a reason for hiding this comment

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

Small typo on the permissions

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

oh damn nice catch!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed

Copy link
Contributor

@rk132 rk132 left a comment

Choose a reason for hiding this comment

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

Overally looks good, but do we want this to be disabled by default? Or should we still add the macro to the unity, when project is imported? we don't know how many people don't need the permissions and how many do right?

@HarrisonHough
Copy link
Collaborator Author

Overally looks good, but do we want this to be disabled by default? Or should we still add the macro to the unity, when project is imported? we don't know how many people don't need the permissions and how many do right?

It's a good question. Generally I would say that people would only need the permissions if targeting mobile AND they don't want to use the CAC.
If they are targeting VR however (which includes mobile/android) then permissions need to be removed if they plan to release to the quest store.
If we still wanted it enabled by default we can either:

  1. force add the symbol when the package is installed
  2. OR rework the symbol to do the opposite. EG RPM_DISABLE_WEBVIEW_PERMISSIONS so that by default it would be enabled (As there would be no symbol) then they just add it if they want to disable it.

The main problem I have with having permissions enabled by default is that it's forcing things that may not be need that can cause bugs, and could be overriding some of their own build post processors. However on the flipside if they do actually need the permissions for their app then of course being enabled by default would be useful

@HarrisonHough HarrisonHough requested a review from rk132 January 3, 2024 10:45
Copy link
Contributor

@rYuuk rYuuk left a comment

Choose a reason for hiding this comment

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

🟢

@HarrisonHough HarrisonHough merged commit eafcd1b into develop Jan 3, 2024
@HarrisonHough HarrisonHough deleted the feature/remove-permission-override branch January 3, 2024 11:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants