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.

Sign up for GitHub

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

Jump to bottom

Prevent preview binary or large file #939

Merged
merged 17 commits into from
Nov 4, 2021
Merged

Prevent preview binary or large file #939

merged 17 commits into from
Nov 4, 2021

Conversation

diegodox
Copy link
Contributor

@diegodox diegodox commented Oct 31, 2021 •
edited
Loading

This PR adds binary file and large file detection for file picker. (#847)

Use content_inspector to detect binary file (I'm new to this project, let me know any already exist way on this project to detect binary file).

@diegodox diegodox marked this pull request as draft October 31, 2021 14:00
@diegodox diegodox marked this pull request as ready for review October 31, 2021 14:06
@diegodox diegodox marked this pull request as draft October 31, 2021 14:07
@diegodox diegodox marked this pull request as ready for review October 31, 2021 14:10
archseer
archseer requested changes Oct 31, 2021
View reviewed changes
Copy link
Member

@archseer archseer left a comment

Choose a reason for hiding this comment

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

This is a good start but there's some improvements we can do:

helix-term/src/ui/picker.rs Outdated Show resolved Hide resolved
helix-term/Cargo.toml Show resolved Hide resolved
helix-term/src/ui/picker.rs Outdated Show resolved Hide resolved
helix-term/src/ui/picker.rs Outdated Show resolved Hide resolved
diegodox
diegodox commented Oct 31, 2021
View reviewed changes
helix-term/src/ui/picker.rs Outdated Show resolved Hide resolved
@archseer archseer mentioned this pull request Nov 1, 2021
Closed
@archseer
Refactor content type detection
cb1c713 
- Remove unwraps
- Reuse a single read buffer to avoid 1kb reallocations between previews
@archseer
Refactor preview rendering so we don't construct docs when not necessary
bcb3f3a
sudormrfbin
sudormrfbin reviewed Nov 1, 2021
View reviewed changes
helix-term/src/ui/picker.rs Outdated Show resolved Hide resolved
helix-term/src/ui/picker.rs Outdated Show resolved Hide resolved
helix-term/src/ui/picker.rs Outdated Show resolved Hide resolved
helix-term/src/ui/picker.rs Outdated Show resolved Hide resolved
helix-term/src/ui/picker.rs Outdated Show resolved Hide resolved
archseer
archseer requested changes Nov 2, 2021
View reviewed changes
helix-term/src/ui/picker.rs Outdated Show resolved Hide resolved
helix-term/src/ui/picker.rs Outdated Show resolved Hide resolved
@diegodox
Copy link
Contributor Author

diegodox commented Nov 2, 2021 •
edited
Loading

Sorry, I messed up this branch with execute unwanted unintended git command.
I'm trying back to what it should be.

@diegodox @archseer
Use index access to hide unwrap
2226481 
Co-authored-by: Blaž Hrastnik <blaz@mxxn.io>
@diegodox
Copy link
Contributor Author

diegodox commented Nov 2, 2021 •
edited
Loading

Sorry, I messed up this branch with execute unwanted git command. I'm trying back to what it should be.
I'm trying back to what it should be.

Now, I think it get back to what it should be.

diegodox
diegodox commented Nov 2, 2021
View reviewed changes
helix-term/src/ui/picker.rs Outdated Show resolved Hide resolved
@diegodox
Copy link
Contributor Author

diegodox commented Nov 2, 2021 •
edited
Loading

@sudormrfbin
I felt try to implement that by myself is interesting (and I thought I cannot write).
But It seems running. So can you check my commit?

PS: Any reason c91ee94 is not good, you can delete my commit.

sudormrfbin
sudormrfbin reviewed Nov 2, 2021
View reviewed changes
Copy link
Member

@sudormrfbin sudormrfbin left a comment

Choose a reason for hiding this comment

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

This is pretty close to what I was coming up with, and apart from a few naming nitpicks looks good to me ! Great job :)

helix-term/src/ui/picker.rs Outdated Show resolved Hide resolved
@diegodox
Copy link
Contributor Author

diegodox commented Nov 2, 2021

Apart from a few naming nitpicks looks good to me !

I don't like naming I did on last commit, too. They should be renamed.

sudormrfbin
sudormrfbin approved these changes Nov 3, 2021
View reviewed changes
Copy link
Member

@sudormrfbin sudormrfbin left a comment

Choose a reason for hiding this comment

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

LGTM ! Good work @diegodox 🎉

archseer
archseer approved these changes Nov 4, 2021
View reviewed changes
Copy link
Member

@archseer archseer left a comment

Choose a reason for hiding this comment

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

Great work @diegodox !

@archseer archseer merged commit 70d21a9 into helix-editor:master Nov 4, 2021
@archseer archseer mentioned this pull request Nov 4, 2021
Closed
@diegodox diegodox deleted the file_picker branch November 4, 2021 11:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@archseer archseer archseer approved these changes

@sudormrfbin sudormrfbin sudormrfbin approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@diegodox @archseer @sudormrfbin