Repository navigation
[cross_file] Updates cross_file to a package separated federated plugin - #11010
Conversation
bparrishMines
added
the
federated: all_changes
bparrishMines
changed the title
|
@stuartmorgan-g This is ready for initial review. The tests are mostly failing on missing integration tests, missing examples in |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request is a significant and well-executed refactoring of cross_file into a federated plugin structure. The new architecture is clean and aligns with modern Flutter plugin conventions. I've identified a few areas for improvement related to dependency version consistency, native code safety, and asynchronous programming best practices, but overall this is a solid contribution.
| func tryCreateBookmarkedUrl(url: String) throws -> String? { | ||
| let nativeUrl = URL(string: url)! | ||
| let data = try nativeUrl.bookmarkData( | ||
| options: [], | ||
| includingResourceValuesForKeys: nil, | ||
| relativeTo: nil | ||
| ) | ||
|
|
||
| var isStale: Bool = true | ||
| let bookmarkedUrl: URL = try URL(resolvingBookmarkData: data, bookmarkDataIsStale: &isStale) | ||
|
|
||
| if !isStale { | ||
| return bookmarkedUrl.absoluteString | ||
| } | ||
|
|
||
| return nil | ||
| } | ||
|
|
||
| func startAccessingSecurityScopedResource(url: String) throws -> Bool { | ||
| return URL(string: url)!.startAccessingSecurityScopedResource() | ||
| } | ||
|
|
||
| func stopAccessingSecurityScopedResource(url: String) throws { | ||
| URL(string: url)!.stopAccessingSecurityScopedResource() | ||
| } |
There was a problem hiding this comment.
The use of force unwrapping (!) when creating a URL from a string is unsafe and can lead to a runtime crash if the url string is malformed. It's better to handle this potential failure gracefully, for example by using guard let and throwing an error.
func tryCreateBookmarkedUrl(url: String) throws -> String? {
guard let nativeUrl = URL(string: url) else {
throw PigeonError(code: "invalid_url", message: "Invalid URL string: \(url)", details: nil)
}
let data = try nativeUrl.bookmarkData(
options: [],
includingResourceValuesForKeys: nil,
relativeTo: nil
)
var isStale: Bool = true
let bookmarkedUrl: URL = try URL(resolvingBookmarkData: data, bookmarkDataIsStale: &isStale)
if !isStale {
return bookmarkedUrl.absoluteString
}
return nil
}
func startAccessingSecurityScopedResource(url: String) throws -> Bool {
guard let nativeUrl = URL(string: url) else {
throw PigeonError(code: "invalid_url", message: "Invalid URL string: \(url)", details: nil)
}
return nativeUrl.startAccessingSecurityScopedResource()
}
func stopAccessingSecurityScopedResource(url: String) throws {
guard let nativeUrl = URL(string: url) else {
throw PigeonError(code: "invalid_url", message: "Invalid URL string: \(url)", details: nil)
}
nativeUrl.stopAccessingSecurityScopedResource()
}| ext.kotlin_version = "2.2.21" | ||
| repositories { | ||
| google() | ||
| mavenCentral() | ||
| } | ||
|
|
||
| dependencies { | ||
| classpath("com.android.tools.build:gradle:8.13.1") | ||
| classpath("org.jetbrains.kotlin:kotlin-gradle-plugin:$kotlin_version") |
There was a problem hiding this comment.
There are inconsistent versions of Gradle, the Android Gradle Plugin (AGP), and Kotlin across the new packages and examples. For instance, this file uses AGP 8.13.1 and Kotlin 2.2.21, while other parts of the PR use older versions (e.g., AGP 8.9.1, Kotlin 2.1.0). To improve maintainability and prevent potential build issues, it would be beneficial to align these versions to a consistent, recent set across all the new Android projects.
|
Thanks for getting this done! |
stuartmorgan-g
left a comment
stuartmorgan-g
left a comment
There was a problem hiding this comment.
Sorry for the mix of comment levels; some are high-level questions, and others are minor things that I wanted to flag before I forgot, that will apply regardless of whether bigger picture pieces change.
| /// macOS. | ||
| @immutable | ||
| base class DarwinScopedStorageXFileCreationParams | ||
| extends PlatformScopedStorageXFileCreationParams { |
There was a problem hiding this comment.
Don't the file and directory classes need to inherit from the IO versions as well?
There was a problem hiding this comment.
No these are there own implementation. The IO implementations are designed to not be permissions based and can be accessed freely. A URI passed to a ScopedStorage file implementation are considered to be temporarily accessible.
| # NOTE: We strongly prefer non-breaking changes, even at the expense of a | ||
| # less-clean API. See https://flutter.dev/go/platform-interface-breaking-changes | ||
| version: 0.0.1 | ||
| publish_to: none |
There was a problem hiding this comment.
This is temporary because it was causing an failure in the CI
There was a problem hiding this comment.
Hm, new packages shouldn't fail CI. What was the error?
|
@stuartmorgan-g For 1 final review. |
|
Cool, thank you for the work! 🎉 |
5 participants
Updates
cross_fileto a package separated federated plugin.Keeping the package separation is still debatable.Follows the general design outlined at https://flutter.dev/go/flutter-file-system
Fixes flutter/flutter#91869
Implementation is split into
dart:io(all platforms except Web)package:webFiles (subclass ofBlob.Pre-Review Checklist
[shared_preferences]pubspec.yamlwith an appropriate new version according to the pub versioning philosophy, or I have commented below to indicate which version change exemption this PR falls under1.CHANGELOG.mdto add a description of the change, following repository CHANGELOG style, or I have commented below to indicate which CHANGELOG exemption this PR falls under1.///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.Footnotes
Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling. ↩ ↩2 ↩3