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

feat: sdk7 renderer restricted actions #5033

Merged
merged 16 commits into from
Apr 26, 2023

Conversation

pbosio
Copy link
Contributor

@pbosio pbosio commented Apr 21, 2023

What does this PR change?

*add openExternalUrl
*add openNftDialog

fixes decentraland/sdk#665

How to test the changes?

openExternalUrl playground test link

openNftDialog playground test link

Our Code Review Standards

https://github.com/decentraland/unity-renderer/blob/master/docs/code-review-standards.md

Copilot summary

🤖 Generated by Copilot at 1c37bd5

This pull request adds a new service called the restricted actions service, which allows the renderer to request the user's permission to open external URLs or NFT dialogs. The pull request implements the service using the RPC framework and the protocol buffer syntax. The pull request also modifies the external URL prompt HUD to use the service, and registers the service in the renderer port and the RPC server. The pull request affects several files in the browser-interface and unity-renderer packages.

@pbosio pbosio added the No QA Needed Issues which do not require QA testing label Apr 21, 2023
@pbosio pbosio self-assigned this Apr 21, 2023
@pbosio pbosio marked this pull request as ready for review April 25, 2023 21:37
@pbosio pbosio requested a review from a team as a code owner April 25, 2023 21:37
@pbosio pbosio requested review from kuruk-mm and AjimenezDCL April 25, 2023 21:38
Comment on lines +71 to +74
private static int GetCurrentFrameCount()
{
return Time.frameCount;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is a temporal solution, I will change renderer's frame count for scene's tick later on another PR so we avoid having issues with renderer update vs scene update

Copy link
Member

Choose a reason for hiding this comment

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

Can you add a comment with this on the code?

Copy link
Contributor

@AjimenezDCL AjimenezDCL left a comment

Choose a reason for hiding this comment

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

Looks good, left a comment about exception handling.

Comment on lines 38 to 40
catch (Exception _)
{ // ignored
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I find this dangerous. I think we should catch cancellation exceptions to ignore them but log the rest.

string urn = 1;
}

message SuccessResponse {
Copy link
Member

Choose a reason for hiding this comment

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

Are we not saying the same thing twice? Maybe renaming to Response would be better because it can be a non-successful response. Right?

Comment on lines +71 to +74
private static int GetCurrentFrameCount()
{
return Time.frameCount;
}
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a comment with this on the code?

@pbosio pbosio merged commit 2d31cd0 into dev Apr 26, 2023
@pbosio pbosio deleted the feat/sdk7-renderer-restricted-actions branch April 26, 2023 19:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
No QA Needed Issues which do not require QA testing
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Open external link not working on SDK7
3 participants