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(android): make getUri and isResource available for override #46152

Conversation

shubhamguptadream11
Copy link
Collaborator

@shubhamguptadream11 shubhamguptadream11 commented Aug 22, 2024

@shubhamguptadream11
Copy link
Collaborator Author

Hi @cortinico,

Could you please take a moment to review this PR when you get a chance? Given its impact, it would be great if we could prioritise it.

Thank you!

@deepanshushuklad11
Copy link
Contributor

@cortinico Please get this PR merged.

@facebook-github-bot
Copy link
Contributor

@cortinico has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

Comment on lines +6723 to +6725
public fun getUri ()Landroid/net/Uri;
public fun hashCode ()I
public final fun isResource ()Z
public fun isResource ()Z
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a small heads up that this is not the right fix. This file is generated from the Kotlin code, so the code needs to be updated as well. I'll take care of it

Copy link
Contributor

Choose a reason for hiding this comment

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

@cortinico Can you also explain how this file gets generated for future reference ?

Copy link
Contributor

Choose a reason for hiding this comment

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

This file gets generated with ./gradlew apiDump (see https://github.com/Kotlin/binary-compatibility-validator)
Also we regenerate the file internally with Buck so even if you edit it, we'll always regenerate it once we import the PR

Copy link
Contributor

Choose a reason for hiding this comment

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

So ideally it should run after your PR .
But didn't happen .

Choose a reason for hiding this comment

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

How about a "thank you" also there @deepanshushuklad11

Thanks @cortinico

Copy link
Contributor

Choose a reason for hiding this comment

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

@bradfloodx Yes a big thanks to @cortinico.
Just wanted to clarify my doubts.

@shubhamguptadream11
Copy link
Collaborator Author

shubhamguptadream11 commented Aug 27, 2024

@cortinico I understand how the .api file is generated from the Kotlin code now. It looks like I need to mark both the uri and isResource properties as open in ImageSource.kt to ensure they are not marked as final in the .api file.

Here's the change I'm planning to make:

/** Get the URI for this image - can be either a parsed network URI or a resource URI. */
public open val uri: Uri = computeUri(context)

/** Get whether this image source represents an Android resource or a network URI. */
public open var isResource: Boolean = false

I pushed the changes. Let me know if there is any other concern.
Thanks for your guidance.

@facebook-github-bot
Copy link
Contributor

@cortinico merged this pull request in 8c81ffa.

@facebook-github-bot facebook-github-bot added the Merged This PR has been merged. label Aug 27, 2024
@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @shubhamguptadream11 in 8c81ffa

When will my fix make it into a release? | How to file a pick request?

cipolleschi pushed a commit that referenced this pull request Sep 11, 2024
Summary:
Fixes following issues:
- #46150
- #46155

## Changelog:

<!-- Help reviewers and the release process by writing your own changelog entry.

Pick one each for the category and type tags:

[ANDROID|GENERAL|IOS|INTERNAL] [BREAKING|ADDED|CHANGED|DEPRECATED|REMOVED|FIXED|SECURITY] - Message

For more details, see:
https://reactnative.dev/contributing/changelogs-in-pull-requests
-->

[ANDROID] [FIXED] - Make getUri and isResource open

Pull Request resolved: #46152

Reviewed By: NickGerleman, rshest, blakef

Differential Revision: D61845164

Pulled By: cortinico

fbshipit-source-id: 88ccdad92423b5add9b2fad4c98f296b6cbfb27d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants