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

Add brotli support #224

Merged
merged 1 commit into from
Nov 1, 2018
Merged

Add brotli support #224

merged 1 commit into from
Nov 1, 2018

Conversation

stokito
Copy link
Contributor

@stokito stokito commented Oct 27, 2018

Looks like my patch is working but if you can please review.
I almost made the same as bzip

@lukefromdc lukefromdc requested a review from a team October 27, 2018 18:08
@lukefromdc
Copy link
Member

Can you provide a test file for that format? I have never seen it before

@stokito
Copy link
Contributor Author

stokito commented Oct 27, 2018

This is an algorithm developed in Google for compressing HTTP related data. Used in Android app store and supported almost by all browsers
https://caniuse.com/#feat=brotli

https://github.com/google/brotli
https://en.wikipedia.org/wiki/Brotli
sudo apt install brotli

@lukefromdc
Copy link
Member

To test this in Engrampa we need to have a file compressed with it-or alternately if this provides the option to compress as well as decompress that would mostly test it but not prove that the compressed file was truly the same as those made by other programs.

@stokito
Copy link
Contributor Author

stokito commented Oct 27, 2018

sudo apt install brotli
brotli uptime.lsp

Result: https://stokito.com/uptime.lsp.br

Copy link
Member

@lukefromdc lukefromdc left a comment

Choose a reason for hiding this comment

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

I can confirm this works for both compressing and extracting brotli files, both the provided test file and one I compressed myself. Caja does not yet recognize the brotli packages as archives, treating them as unknown filetype, so an update will be needed there as well but that's a separate issue.

Memory management looks to be unchanged with no new variables. This looks good to go

@lukefromdc lukefromdc requested a review from a team October 27, 2018 19:23
@stokito
Copy link
Contributor Author

stokito commented Oct 27, 2018

Caja does not yet recognize the brotli packages as archives, treating them as unknown file type.

I guess that's because brotli not yet recognized byfile command
google/brotli#727

Thank you for review.

help/engrampa.pot Outdated Show resolved Hide resolved
@stokito
Copy link
Contributor Author

stokito commented Oct 30, 2018 via email

@raveit65
Copy link
Member

raveit65 commented Oct 30, 2018

Please don't touch pot files. This we doing for ourself in an extra commit.

@stokito
Copy link
Contributor Author

stokito commented Oct 30, 2018 via email

@raveit65
Copy link
Member

raveit65 commented Oct 30, 2018

Yes, please.
Are you familiar with using the git rebase -i <last-commit-before-your-PR> in you local repo?

There is a bug with creating pot files.

-"Project-Id-Version: MATE Desktop Environment\n"
+"Project-Id-Version: PACKAGE VERSION\n"

We always have to roll back this by hand.

@stokito
Copy link
Contributor Author

stokito commented Oct 30, 2018

ok, so I repushed the commit without pot files. Now can you marge it?

@sc0w sc0w self-requested a review November 1, 2018 00:25
Copy link
Member

@sc0w sc0w left a comment

Choose a reason for hiding this comment

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

I can open/compress files in that format with this PR.

@lukefromdc

Caja does not yet recognize the brotli packages as archives, treating them as unknown filetype, so an update will be needed there as well but that's a separate issue.

Yes, confirmed, I agree.

@lukefromdc lukefromdc merged commit f06c216 into mate-desktop:master Nov 1, 2018
@stokito stokito deleted the brotli branch August 28, 2019 06:32
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.

4 participants