-
Notifications
You must be signed in to change notification settings - Fork 773
[Android] Savefile issues with MimeType #1822
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
[Android] Savefile issues with MimeType #1822
Conversation
@@ -29,7 +29,7 @@ void main() { | |||
expect( | |||
dialogHandler.fileTypeToFileFilter(FileType.image, null), | |||
equals( | |||
'Image File (*.[bB][mM][pP] *.[gG][iI][fF] *.[jJ][pP][eE][gG] *.[jJ][pP][gG] *.[pP][nN][gG])'), | |||
'Image File (*.[bB][mM][pP] *.[gG][iI][fF] *.[jJ][pP][eE][gG] *.[jJ][pP][gG] *.[pP][nN][gG] *.[wW][eE][bB][pP])'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vicajilau Does MacOS & Windows also need an update for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They were added in #1815
@@ -8,7 +8,7 @@ plugins { | |||
android { | |||
namespace = "com.mr.flutter.plugin.filepicker.file_picker_example" | |||
compileSdk = flutter.compileSdkVersion | |||
ndkVersion = flutter.ndkVersion | |||
ndkVersion = "27.0.12077973" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the reason for this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is within the example project, because some dependencies are ahead of Flutter's own NDK version. It compiles without this, but on Android it gives a warning indicating that it will upgrade to the version to make it work
android/src/main/kotlin/com/mr/flutter/plugin/filepicker/FileUtils.kt
Outdated
Show resolved
Hide resolved
Co-authored-by: Navaron Bracke <[email protected]>
Co-authored-by: Navaron Bracke <[email protected]>
…tils.kt Co-authored-by: Navaron Bracke <[email protected]>
…n-using-files-with-non-standard-mimetype
Everything ready @navaronbracke 🚀 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vicajilau One minor nit, also can you update the referenced issues to be like
Resolves #1789
Resolves #1818
Resolves #1819
Resolves #1820
since Github only recognizes the first issue number as fixed.
@@ -1,3 +1,9 @@ | |||
## 10.2.0 | |||
### Desktop | |||
- Added support for webp images on Desktop platforms. [#1491](https://github.com/miguelpruivo/flutter_file_picker/issues/1491) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: This is only updated for Linux, per your comment in https://github.com/miguelpruivo/flutter_file_picker/pull/1822/files#r2142435446
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I meant was that the tests were correctly added to the PR itself on MacOS and Windows. That's why I only added them on Linux.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vicajilau I meant to put the Resolves <link>
in the OP of this PR, not in the changelog (the original changelog entry was fine).
This is purely to adhere to Github auto-closing principles when a PR is merged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okey @navaronbracke, restored and changed!! 💪
Can you have a look again? @navaronbracke |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
@@ -42,7 +42,7 @@ android { | |||
implementation 'androidx.core:core:1.15.0' | |||
implementation 'androidx.annotation:annotation:1.9.1' | |||
implementation "androidx.lifecycle:lifecycle-runtime:2.8.7" | |||
implementation "org.apache.tika:tika-core:3.1.0" | |||
implementation "org.apache.tika:tika-core:3.2.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vicajilau Tika 3.0 requires Java 11. Are you sure that is going to work on Android 5?
There should be a more lightweight way to get the file mime type than dragging in Tika core... E.g. jmimemagic is 44k vs tika-core 700k
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...verified on Android 5. Oddly it does work. Though I still wonder if 3rd party dependency size for detecting mime types could be reduced
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The explanation is simple @ekuleshov, if at any time Tika fails in its attempt to detect it, it will always take */*
as you can see here.
On the other hand, while there is no official Android solution for this task, in my opinion, if we have to use a third-party solution, I prefer the Apache foundation despite its size difference < 1Mb. (People contributing, popularity, etc..)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The explanation is simple @ekuleshov, if at any time Tika fails in its attempt to detect it, it will always take
*/*
as you can see here.
Ouch! That is going to hurt as files will be always saved with a wrong mime type on Android 5, e.g. these files won't be openable by other apps as expected.
On the other hand, while there is no official Android solution for this task, in my opinion, if we have to use a third-party solution, I prefer the Apache foundation despite its size difference < 1Mb. (People contributing, popularity, etc..)
Adding 1mb is a lot for a Flutter app... There is also a pure Dart library maintained by the Dart team that also supports resolving mime types by file content. https://pub.dev/packages/mime
Resolves #1789
Resolves #1818
Resolves #1819
Resolves #1820