Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
documentation changes from review comments
  • Loading branch information
Gray Mackall committed May 8, 2023
commit 167f9c262ee8ca3b5adecb6116cf62911710f465
Original file line number Diff line number Diff line change
Expand Up @@ -63,11 +63,17 @@ class SystemServices {
}

/// Returns a file path which was used to create a temporary file.
/// Prefix is a part of the file name, and suffix is the file extension.
///
/// The file and path constraints are determined by the implementation of
/// File.createTempFile(prefix, suffix, cacheDir), on the android side, where
/// where cacheDir is the cache directory identified by the current application
/// context using context.getCacheDir().
///
/// Ex: getTempFilePath('prefix', 'suffix') would return a string of the form
/// '<cachePath>/prefix3213453.suffix', where the numbers after prefix and
/// before suffix are determined by the call to File.createTempFile and
/// therefore random.
static Future<String> getTempFilePath(String prefix, String suffix,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think from the tests I read prefix is actually the file name and suffix is the extension. That came as a bit of a suprise from reading the documentation I would have expected prefix<something the caller does not control>suffix for the file name.

For example it means as a caller I think this means I need to ensure that I pass something different each time or risk overwriting the previous file.

Copy link
Member Author

@gmackall gmackall May 8, 2023

Choose a reason for hiding this comment

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

I'll clarify the documentation before merging, the way it actually works is that the file ends up being
prefix<something the caller does not control>.suffix, i.e. 'suffix' is the file extension, and prefix is part of the file name, but more gets added by createTempFile (a random sequence of numbers).

So getTempFile('MOV', '.temp'), is something like <cachedir>/MOV1231242244.temp (notably this means you do not have to pass different things to avoid overwriting).

FWIW I chose prefix and suffix because those are the variable names used by the native methods implementation/documentation.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK in that case it means the documentation is correct but that is not what the tests are testing for. I think using the same names as the native code is a reasonable choice.

Specifically in packages/camera/camera_android_camerax/test/system_services_test.dart and in packages/camera/camera_android_camerax/android/src/test/java/io/flutter/plugins/camerax/SystemServicesTest.java you are testing for the result to be equal totestPath + testPrefix + testSuffix (or the file for prefix and suffix). Instead of testing that the output had a path of testPath a file prefix of testPrefix and an extension of testSuffix. Which is fine it is just confusing when trying to understand what to expect from the tests.

{BinaryMessenger? binaryMessenger}) {
final SystemServicesHostApi api =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ class VideoCapture extends UseCase {
AndroidCameraXCameraFlutterApis.instance.ensureSetUp();
}

/// Creates a VideoCapture associated with the given [Recorder].
/// Creates a [VideoCapture] associated with the given [Recorder].
static Future<VideoCapture> withOutput(Recorder recorder,
{BinaryMessenger? binaryMessenger, InstanceManager? instanceManager}) {
AndroidCameraXCameraFlutterApis.instance.ensureSetUp();
Expand Down