[cross_file] Updates cross_file to a package separated federated plugin #11010
[cross_file] Updates cross_file to a package separated federated plugin #11010bparrishMines wants to merge 196 commits intoflutter:mainfrom
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()
}| IOXDirectoryExtension? get extension => this; | ||
|
|
||
| @override | ||
| Future<bool> exists() async => directory.existsSync(); |
There was a problem hiding this comment.
Using the synchronous existsSync() in an async method can block the event loop, potentially leading to UI unresponsiveness. It's recommended to use the asynchronous exists() method instead.
| Future<bool> exists() async => directory.existsSync(); | |
| Future<bool> exists() => directory.exists(); |
| Future<DateTime?> lastModified() async { | ||
| try { | ||
| return file.lastModifiedSync(); | ||
| } on FileSystemException { | ||
| return null; | ||
| } | ||
| } |
There was a problem hiding this comment.
Using the synchronous lastModifiedSync() in an async method can block the event loop. Please use the asynchronous lastModified() method to avoid potential UI jank.
| Future<DateTime?> lastModified() async { | |
| try { | |
| return file.lastModifiedSync(); | |
| } on FileSystemException { | |
| return null; | |
| } | |
| } | |
| Future<DateTime?> lastModified() async { | |
| try { | |
| return await file.lastModified(); | |
| } on FileSystemException { | |
| return null; | |
| } | |
| } |
| Future<bool> canRead() => exists(); | ||
|
|
||
| @override | ||
| Future<bool> exists() async => file.existsSync(); |
There was a problem hiding this comment.
Using the synchronous existsSync() in an async method can block the event loop. Please use the asynchronous exists() method to avoid potential UI jank.
| Future<bool> exists() async => file.existsSync(); | |
| Future<bool> exists() => file.exists(); |
| 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.
1 participant
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
Implementation is split into
dart:io(all platforms except Web)package:webFiles (subclass ofBlob.cross_file_ioimplementations to add support for following methods:Pubspec dependencies have been changed to use ones from git to have runnable example apps while in development and review. Specifically because
file_selectoruses the current version of cross_file and I didn't want to make this a file_selector + cross_file PR. Can update it to this if preferred.The reason
file_selectoris different for the example apps is because I wanted to ensue Android, iOS, macOS still work without copying the file to a file with the Apps sandbox.Example
The
cross_fileexample will run for all platforms except for linux and windows since they are simply using thedart:ioimplementation.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