Skip to content
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

Fix missing .json file extension when exporting notes #14

Merged

Conversation

tom93
Copy link
Contributor

@tom93 tom93 commented Jan 27, 2024

This is a fix for #13.

Update: #13 (comment) caught me by surprise, need to test that this works properly on more devices (don't want to end up with .json.json as the file extension), so I'm marking as Draft for now. Should also update the commit message to mention that this fix is only required in some circumstances (Android < 10?).

Update 2: Done (tested and updated commit message).

@tom93 tom93 marked this pull request as draft January 27, 2024 17:16
@Aga-C
Copy link
Member

Aga-C commented Jan 27, 2024

It works fine on Android 6, 13 and 14 (emulator).

@tom93
Copy link
Contributor Author

tom93 commented Jan 27, 2024

Thanks. I tried digging through the Android code to make sense of the automatic extensions but couldn't crack it. However I did find this which mentions

Intent.ACTION_CREATE_DOCUMENT requires that developers specify a concrete mime type - using "*/*" breaks Storage Storage Access Framework's handling of file extensions (e.g., automatically adding the correct file extension or positioning a
"(1)" suffix right before the file extension in cases of a duplicate name).

Duplicate names are interesting. I tested what happens on Android 9 when the file already exists (with this PR applied):

  • If I use the native Files app (com.android.documentsui), it generates a name such as "Notes.json (1)". This is bad, but I'd argue that it's better than not having .json in the filename at all.
  • If I use Fossify File Manager, it silently overwrites the existing file (!). I think that should be considered a bug in File Manager; I'll open an issue.

I can't test what happens on newer Android versions, but if this PR breaks the location of the "(1)" then I guess we'll have to add a check -- hopefully a simple Android version check will tell us whether .json will be appended automatically; if not, I'm not sure what to do.

The input from the user doesn't have a .json suffix. Previously, we
passed that value to ActivityResultContracts.CreateDocument as is
(without an extension), and relied on the file picker to automatically
append the extension based on the MIME type.

But some file pickers don't automatically append the extension, e.g.
the native Files app (com.android.documentsui) on Android <= 9, and
FossifyOrg/File-Manager. This results in the exported file not having
a .json extension.

Fix this by manually appending the .json extension before passing the
file name to CreateDocument.

Manually appending the extension shouldn't cause a problem for file
pickers that can automatically add an extension. For example the
native Files app doesn't add a second extension, and if the file
already exists it generates a unique name correctly, with the "(1)"
inserted before the extension, e.g. "Notes (1).json".

Fixes FossifyOrg#13.
@tom93 tom93 force-pushed the fix-export-missing-file-extension branch from c5a27ee to 433a7c1 Compare January 31, 2024 06:55
@tom93
Copy link
Contributor Author

tom93 commented Jan 31, 2024

I did more testing (see conclusions) and this PR does not cause issues when using the native Files app on Android 9, 10, 14 (including when the file already exists).

If people want to test other file managers please do, otherwise I think this PR is ready to merge.

@tom93 tom93 marked this pull request as ready for review January 31, 2024 07:03
tom93 added a commit to tom93/FossifyOrg-Messages that referenced this pull request Mar 16, 2024
Very similar to FossifyOrg/Notes#14 (which has a more detailed commit
message). Basically, some file pickers don't automatically append the
extension, so we should do it ourselves. It doesn't cause problems.

Reported in FossifyOrg#88.
tom93 added a commit to tom93/FossifyOrg-Messages that referenced this pull request Mar 16, 2024
Very similar to FossifyOrg/Notes#14 (which has a more detailed commit
message). Basically, some file pickers don't automatically append the
extension, so we should do it ourselves. It doesn't cause problems.

Reported in FossifyOrg#88.
@naveensingh
Copy link
Member

I'll trust you guys on this one, thanks :)

@naveensingh naveensingh merged commit 80bc94c into FossifyOrg:master Mar 26, 2024
tom93 added a commit to tom93/FossifyOrg-Messages that referenced this pull request May 20, 2024
Very similar to FossifyOrg/Notes#14 (which has a more detailed commit
message). Basically, some file pickers don't automatically append the
extension, so we should do it ourselves. It doesn't cause problems.

Reported in FossifyOrg#88.
tom93 added a commit to tom93/FossifyPhone that referenced this pull request Feb 14, 2025
Very similar to FossifyOrg/Notes#14 (which has a more detailed commit
message). Basically, some file pickers don't automatically append the
extension, so we should do it ourselves. It doesn't cause problems.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants