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

Filename width fix #5

Closed
wants to merge 6 commits into from
Closed

Filename width fix #5

wants to merge 6 commits into from

Conversation

NHOrus
Copy link
Contributor

@NHOrus NHOrus commented Feb 3, 2017

Some people have unicode filenames. They obviously misalign with naive width calculations.

@NHOrus
Copy link
Contributor Author

NHOrus commented Feb 3, 2017

And I am done for today.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.5%) to 70.69% when pulling ef87533 on NHOrus:namewidth into 2abe4bb on gongo:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.5%) to 70.69% when pulling ef87533 on NHOrus:namewidth into 2abe4bb on gongo:master.

@@ -83,8 +84,9 @@ func maximumNameLength(filenames []string) int {
max := 0
for _, name := range filenames {
base := filepath.Base(name)
if len(base) > max {
max = len(base)
width := runewidth.StringWidth(base)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As this is a bit more costly than len(string), calling only once per loop and instead using temporary variable.

@gongo
Copy link
Owner

gongo commented Feb 3, 2017

In my environment:

image

@NHOrus
Copy link
Contributor Author

NHOrus commented Feb 4, 2017

screenshot at 2017-02-04 03-22-01
Without filename width patch and with it.
Need to think more about it.

@NHOrus
Copy link
Contributor Author

NHOrus commented Feb 4, 2017

screenshot at 2017-02-04 03-27-01
And with utf8. RuneCountInString() instead of go-runewidth.

@NHOrus
Copy link
Contributor Author

NHOrus commented Feb 4, 2017

Sorry. I don't know.

@gongo
Copy link
Owner

gongo commented Feb 4, 2017

I will try to check that out too 💪

@gongo
Copy link
Owner

gongo commented Feb 6, 2017

Hi @NHOrus .
I came up with a good way. I might develop it on another branch. 🙏

@NHOrus
Copy link
Contributor Author

NHOrus commented Feb 6, 2017

I think if you succeed it would be great to put in it's own library. Because, well, not a question of width of symbol, but width of a glyph in font.

Also, willing to help with that.

@gongo
Copy link
Owner

gongo commented Feb 6, 2017

Because, well, not a question of width of symbol, but width of a glyph in font.

Sure.

But, even if it can not be solved in all environments , it may be able to solve to some extent (for example: limited to monospaced fonts) 💪

@emersion
Copy link

emersion commented Feb 6, 2017

Maybe this can help? https://godoc.org/golang.org/x/text/width

@gongo gongo mentioned this pull request Feb 7, 2017
@gongo
Copy link
Owner

gongo commented Feb 7, 2017

@NHOrus

I fixed it in GH-7, so close this PR. Thanks !!

@gongo gongo closed this Feb 7, 2017
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