Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Allow specifying/retaining names of downloaded files. #488

Merged
merged 3 commits into from
Sep 16, 2023

Conversation

foodelevator
Copy link
Contributor

This allows the name of downloaded files to be specified by prepending base64(filename): to the file contents after a ^[[5i.

It seems the implementation allows the file contents to not be base64 encoded, and in that case downloading a file containing a : would break with these changes. In other cases, such as when using the documented bash function, this should be backwards compatible.

fileName = window.atob(fileNameBase64);
}
} catch (err) {
// Filename wasn't base64-encoded so let's ignore it
Copy link
Owner

Choose a reason for hiding this comment

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

Is there a reason to base64 encode the filename?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If it is not encoded/escaped and contains a :, using : as a separator would not work correctly. Base64-encoding seemed like the easiest option that is guaranteed to work, but there are certainly other options that would work too!

// Filename wasn't base64-encoded so let's ignore it
}

if (typeof fileName !== "string") {
Copy link
Owner

Choose a reason for hiding this comment

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

Im not a big fan of typeof checks as it implies the type can change.

Given the type is string|undefined could the check be is fileName === undefined

Copy link
Owner

@butlerx butlerx left a comment

Choose a reason for hiding this comment

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

thanks for the PR some question and nitpicks

@butlerx butlerx merged commit 05b74e0 into butlerx:main Sep 16, 2023
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants