Skip to content
This repository was archived by the owner on Feb 22, 2023. It is now read-only.

[image_picker] remove unnecessary file path for video. #1315

Merged

Conversation

cyanglaz
Copy link
Contributor

@cyanglaz cyanglaz commented Mar 5, 2019

Simplified version of #1245 fix.
When pick videos, we used to create an unnecessary extra file. This PR removes the extra temp file logic.

Copy link
Contributor

@mklim mklim left a comment

Choose a reason for hiding this comment

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

LGTM, but I'm a little wary of the code really handling some edge case that we're not seeing. /cc @amirh in case he has more info, looks like he wrote these lines originally.

@cyanglaz cyanglaz requested a review from amirh March 5, 2019 23:27
@ivk1800
Copy link
Contributor

ivk1800 commented Mar 6, 2019

seems related #1288

@cyanglaz
Copy link
Contributor Author

cyanglaz commented Mar 6, 2019

seems related #1288

Actually this PR is for iOS and #1288 is for Android. So although both are trying to solve similar problems but they are separate fixes.

@amirh
Copy link
Contributor

amirh commented Mar 6, 2019

The tmp path logic was added in https://github.com/flutter/image_picker/commit/5409baccb75680547b1a799f6853da423e5fd9cb

@collinjackson do you know whether we still need it?

@cyanglaz
Copy link
Contributor Author

cyanglaz commented Mar 6, 2019

The original videoURL is already a tmpPath. I thought adding another tmpPath did not make sense. I guess it could be a copy paste error (copy pasted from the picking image logic) when we introduced the video capability in the plugin.

@cyanglaz cyanglaz requested a review from collinjackson March 12, 2019 02:08
@cyanglaz cyanglaz added needs love submit queue The Flutter team is in the process of landing this PR. bugfix labels Mar 15, 2019
@collinjackson
Copy link
Contributor

lgtm

@cyanglaz cyanglaz merged commit c78b4e5 into flutter:master Mar 19, 2019
@cyanglaz cyanglaz deleted the image_picker_remove_additional_temp_filepath branch March 19, 2019 18:44
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bugfix cla: yes submit queue The Flutter team is in the process of landing this PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants