Fix docker search output form when the description has CJK character by ChiuWang · Pull Request #35034 · moby/moby

ChiuWang · 2017-09-29T07:55:01Z

- What I did
fixes docker search output form when the description has CJK character
- How I did it
Every Chinese or Japanese character takes two position of a English charactor,
But technically many other languages or special symbol should be checked. But I didn't
find a search result case.
- How to verify it

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

GordonTheTurtle · 2017-09-29T07:55:03Z

Please sign your commits following these rules:
https://github.com/moby/moby/blob/master/CONTRIBUTING.md#sign-your-work
The easiest way to do this is to amend the last commit:

$ git clone -b "ellipsis" [email protected]:charrywanganthony/moby.git somewhere
$ cd somewhere
$ git commit --amend -s --no-edit
$ git push -f

Amending updates the existing PR. You DO NOT need to open a new one.

AkihiroSuda · 2017-09-29T11:20:14Z

SGTM, anyone can verify current behavior with other writing systems such as Arabic/Hebrew or Mongolian?

ChiuWang · 2017-09-29T13:35:38Z

As far as i know，any system with usual Chinese(Japanese) fonts will display the character by East_Asian_Width=Wide unicode.org. Because it's a verification for description so far, i set a general check range//UTF-8 Kana, CJK, Hangul (from 3000 to DFFF).
If it proves valuable, i could make it more accrurate and support more languages later. @AkihiroSuda

thaJeztah · 2017-09-29T13:54:24Z

ping @ripcurld0 you may know some images with other character sets ^^

thaJeztah · 2017-09-29T13:55:04Z

@charrywanganthony looks like you need to gofmt;

08:23:58 pkg/stringutils/stringutils.go:1::warning: file is not gofmted with -s (gofmt)
08:23:58 pkg/stringutils/stringutils_test.go:1::warning: file is not gofmted with -s (gofmt)
08:23:58 pkg/stringutils/stringutils.go:58:4:warning: should replace displayLen += 1 with displayLen++ (golint)
08:23:58 pkg/stringutils/stringutils.go:66:10:warning: should omit 2nd value from range; this loop is equivalent to `for i := range ...` (golint)
08:23:58 pkg/stringutils/stringutils.go:72:9:warning: should omit 2nd value from range; this loop is equivalent to `for i := range ...` (golint)
08:23:58 pkg/stringutils/stringutils.go:1::warning: file is not goimported (goimports)
08:23:58 pkg/stringutils/stringutils_test.go:1::warning: file is not goimported (goimports)

AkihiroSuda · 2017-09-30T13:19:05Z

https://en.m.wikipedia.org/wiki/Halfwidth_and_fullwidth_forms contains some full-width symbols, half-width Katakana and half-width Hangul Jamo

boaz0 · 2017-10-01T07:58:07Z

@thaJeztah the current code supports Hebrew characters pretty good. No need for code change. However, Hebrew text is printed in reverse.

$ docker search ripcurld00dx/boaz

ChiuWang · 2017-10-09T06:49:49Z

I update the commit @thaJeztah , and set a more precise range for @AkihiroSuda . :)

AkihiroSuda · 2017-10-09T07:04:32Z

Can we use some existing unicode library such as https://godoc.org/golang.org/x/text/width ?

ChiuWang · 2017-10-09T07:36:22Z

Thx, this lib looks right for us. I'll check it soon.

ChiuWang · 2017-10-10T06:44:29Z

I did the change but met a problem.

03:17:03 The result of vndr differs

03:17:03  D vendor/golang.org/x/text/width/gen.go
03:17:03  D vendor/golang.org/x/text/width/gen_common.go
03:17:03  D vendor/golang.org/x/text/width/gen_trieval.go
03:17:03  M vendor/golang.org/x/text/width/kind_string.go
03:17:03  M vendor/golang.org/x/text/width/tables.go
03:17:03  M vendor/golang.org/x/text/width/trieval.go

03:17:03 Please vendor your package with github.com/LK4D4/vndr.

When i use vndr i will get the information
unrecognized import path "golang.org/x/net" (https fetch: Get https://golang.org/x/net?go-get=1: Service Unavailable) because of Chinese net can't reach golang.org.

How can I change vendor.conf manually to avoid the error

AkihiroSuda · 2017-10-10T06:51:20Z

~~Please just put golang.org/x/text/width/gen.go <commit hash> to vendor.conf~~

Please just put golang.org/x/text <commit hash> to vendor.conf

EDIT: fix copy-paste error

AkihiroSuda · 2017-10-10T06:51:52Z

pkg/stringutils/stringutils.go

Please avoid using magic numbers

AkihiroSuda · 2017-10-10T08:06:46Z

pkg/stringutils/stringutils.go

These are already defined as width.EastAsianAmbiguous

Thx for your help, I correct this, but the vendor problem still exists. @_@

AkihiroSuda · 2017-10-10T09:33:59Z

vendor.conf

Please remove *.go lines and just update commit hash of golang.org/x/text

golang.org/x/text/width/gen.go

This was just my copy-paste error. I meant golang.org/x/text <commit hash>.
Sorry for causing confusion.

problem still exists. @-@ Do I need to update the whole text package?

Vendor check passed after I update text package. :)

AkihiroSuda · 2017-10-10T09:37:22Z

看起来不错，除了 vendor.conf之外 (LGTM except vendor.conf)

cc @yongtang @coolljt0725 请看一下 (PTAL) ?

thaJeztah · 2017-10-10T13:41:00Z

pkg/stringutils/stringutils_test.go

Is this expected? I don't think the whale emoji is a wide character?

The character do takes two positions of a narrow one. And the test will pass only in this way, after we adopt https://godoc.org/golang.org/x/text/width

Ah! Yes, you're right, I thought it only took a single width, but I see now.

Sorry for the noise 😊

Signed-off-by: Chao Wang <[email protected]>

thaJeztah · 2017-10-11T09:56:03Z

WindowsRS1 failure is a known issue, and not related to the changes in this PR; #32219

thaJeztah

My concern with this PR is that it changes the behavior of the Ellipsis(), but not Truncate(). Therefore, these functions act different, which is very inconsistent.

For example, the following result in "t🐳s":

stringutils.Truncate("t🐳s", 3)

But using ellipsis results in "t🐳":

stringutils.Ellipsis("t🐳s", 3)

Looking at this package, I think it has become a collection of not-so-related functionality, and its scope is becoming more vague with this change. Let me try to explain that;
Ellipsis() is clearly targeted at displaying strings. Truncate() on the other hand is more ambigious; when used for displaying perhaps the string should be truncated at display width, not number of characters. However, because the purpose of the package is not well defined, it can be used in other contexts, and those can be problematic (e.g. limit a string's length to a maximum number of characters for storage).

I see this package has also been mentioned in #32989 (see @dnephin's gist), and based on the above, I think we should "dismantle" this package, and move code to places it will be used.

Having a look where things are used, and what we can do:

Function	Where it's used	What to do
`GenerateRandomAlphaOnlyString()`	Only used in test (integration-cli)	integration-cli: `RandomTmpDirPath()` already mentions that it should not be used. integration-cli/cli/build/fakestorage/storage.go uses it. Other uses may not even need a random string; so either move it to the integration-cli, or perhaps it can be removed altogether.
`GenerateRandomASCIIString()`	Looks like it's not used	Probably can be removed
`Ellipsis()`	Only used in the CLI (https://github.com/docker/cli)	Move to that repository, and rewrite to indicate it uses `displayWidth`, not `length`.
`Truncate()`	Looks like it's not used	Move to the, and define its purpose, or perhaps rename it (`TruncateDisplayWidth`, rename the package to `displayutils` and name it `displayutils.TrimWidth()` - bad examples, but just to get you thinking), or remove it altogether
`InSlice()`	used in a couple of tests, and also in the `seccomp` and `oci_linux` parts	It's a small function, and given that it's used in different contexts, I think we should copy it to those parts that use it, possibly renaming it for each purpose.
`ShellQuoteArguments()`	Looks like it's not used; also this isn't a "generic" string function - it's specifically for handling shell arguments.	Given that it isn't used, we can probably remove it.

thaJeztah · 2017-10-11T12:15:46Z

pkg/stringutils/stringutils.go

+		if maxlen <= 3 {
+			return string(r[:maxlen])
+		}
+		return string(r[:maxlen-3]) + "..."


If we're improving this function, perhaps this is the right moment to use the actual ellipsis ("…") character for this; not sure if that is one or two positions wide, but at least it allows showing more of the original string 👍

thaJeztah · 2017-10-11T12:19:06Z

pkg/stringutils/stringutils.go

+		//In a broad sense, wide characters include East Asian Wide, East Asian Fullwidth, and East Asian Ambiguous,
+		//see http://unicode.org/reports/tr11/
+		kind := width.LookupRune(r).Kind()
+		if kind == width.EastAsianAmbiguous || kind == width.EastAsianWide || kind == width.EastAsianFullwidth {


Perhaps this should be used to a utility function, something like:

// charWidth returns the number of horizontal positions a character occupies, // and is used to account for wide characters when displaying strings. // // In a broad sense, wide characters include East Asian Wide, East Asian Full-width, // and East Asian Ambiguous (see http://unicode.org/reports/tr11/). func charWidth(r rune) int { switch width.LookupRune(r).Kind() { case width.EastAsianAmbiguous, width.EastAsianWide, width.EastAsianFullwidth: return 2 default: return 1 } }

Then this loop could be something like;

var ( display []int displayWidth int ) rs := []rune(s) for _, r := range rs { cw := charWidth(r) displayWidth += cw display = append(display, cw) }

OK , I will improve the code after we move the function to CLI repository.

Thanks! Are you interested in working on that move?

It will be a great pleasure ：)

ChiuWang · 2017-10-12T06:09:37Z

Thanks for your careful work @thaJeztah
So based on the above, I will
PR[*]: dumplicate the package function by function.
then
PR: remove the stringutils package in moby
then
Close this PR
？

thaJeztah · 2017-10-12T07:35:16Z

Awesome! First step would be to open a PR in https://github.com/docker/cli to have these utilities there.

Once that is done, you can open a new PR in this repository to remove the things we no longer need (and make the other changes)

I suggest we close this PR for now; does that sound good to you?

ChiuWang · 2017-10-12T07:47:15Z

OK.

ChiuWang · 2017-10-12T08:16:55Z

But I'm a little confused, what doesopen a PR in https://github.com/docker/cli to have these utilities there exactly mean. @thaJeztah

thaJeztah · 2017-10-12T08:33:49Z

That repository contains the Docker CLI; the parts of stringutils that are only used there (such as Ellipsis() should be added to that repository; if you can open a pull request for that.

Happy to give more information/help if you need

ChiuWang · 2017-10-12T09:01:32Z

Yeah, I know this. What i'm comfused is the stringutils(moby) is in moby/moby/pkg/ ,but stringutils(cli) is in docker/cli/vendor/github.com/docker/docker/pkg/, this "vendor-relation" between moby and docker is somewhat equivocal.(sorry I'm a newbee ^-^)

thaJeztah · 2017-10-12T09:27:49Z

Ah yes; what we use in these repositories is "vendoring"; basically, instead of downloading dependencies when compiling, we keep a copy of all dependencies in the repository.

Doing so, we know exactly which version of the dependency is used (the vendor.conf file contains the exact commit that was used), and also makes sure we can always build (even if the server hosting the dependency is down)

In this case, the Docker CLI uses some packages from this (https://github.com/moby/moby) repository - but still under the old name (https://github.com/docker/docker - that will change soon).

Here's some information about vendoring that I found https://blog.gopheracademy.com/advent-2015/vendor-folder/

ChiuWang · 2017-10-12T09:37:07Z

If so, IMHO why not just change the code in this repository and then use vendor tools to update the package for CLI. But you recommend First step would be to open a PR in CLI ... Once that is done....

thaJeztah · 2017-10-12T09:46:39Z

Well; the reason for moving it, is that usually, packages in a repository should be in that repository because the repository itself uses it (but they could be useful for other projects)

I this case, the code is not used at all in this repository, so it's better to move it to the repository (Docker CLI) where it's actually used

ChiuWang · 2017-10-12T10:05:42Z

Thx! sorry for bothering u the beginner level question.

thaJeztah · 2017-10-12T10:11:10Z

Not at all; they're good questions!

GordonTheTurtle added dco/no Automatically set by a bot when one of the commits lacks proper signature status/0-triage labels Sep 29, 2017

ChiuWang force-pushed the ellipsis branch from 036b128 to 61c6cea Compare September 29, 2017 08:14

GordonTheTurtle removed the dco/no Automatically set by a bot when one of the commits lacks proper signature label Sep 29, 2017

thaJeztah added status/2-code-review status/failing-ci Indicates that the PR in its current state fails the test suite and removed status/0-triage labels Sep 29, 2017

ChiuWang force-pushed the ellipsis branch from 61c6cea to 6accf8c Compare October 9, 2017 06:44

ChiuWang force-pushed the ellipsis branch from 6accf8c to 0dd3eeb Compare October 10, 2017 03:10

AkihiroSuda reviewed Oct 10, 2017

View reviewed changes

pkg/stringutils/stringutils.go Outdated

Copy link

Member

AkihiroSuda Oct 10, 2017

Choose a reason for hiding this comment

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

Please avoid using magic numbers

ChiuWang force-pushed the ellipsis branch 2 times, most recently from 6bad739 to 640bf2f Compare October 10, 2017 08:03

AkihiroSuda reviewed Oct 10, 2017

View reviewed changes

ChiuWang force-pushed the ellipsis branch 3 times, most recently from 2c48d5e to a16d694 Compare October 10, 2017 08:26

AkihiroSuda reviewed Oct 10, 2017

View reviewed changes

ChiuWang force-pushed the ellipsis branch from a16d694 to 1d13382 Compare October 10, 2017 10:03

thaJeztah reviewed Oct 10, 2017

View reviewed changes

ChiuWang force-pushed the ellipsis branch from 1d13382 to a20a46e Compare October 11, 2017 01:51

Fix docker search output form when the description has CJK character

b5693ab

Signed-off-by: Chao Wang <[email protected]>

ChiuWang force-pushed the ellipsis branch from a20a46e to b5693ab Compare October 11, 2017 02:27

AkihiroSuda added rebuild/powerpc and removed status/failing-ci Indicates that the PR in its current state fails the test suite labels Oct 11, 2017

GordonTheTurtle removed rebuild/powerpc labels Oct 11, 2017

thaJeztah requested changes Oct 11, 2017

View reviewed changes

ChiuWang closed this Oct 12, 2017

ChiuWang mentioned this pull request Oct 13, 2017

Refactor stringutils and fix docker search output form docker/cli#621

Merged

This was referenced Oct 27, 2017

Separate the GenerateRandomAlphaOnlyString function from stringutils #35313

Merged

Copy Inslice() to those parts that use it #35342

Merged

ChiuWang mentioned this pull request Nov 13, 2017

Split and remove pkg/stringutils #35473

Merged

Comments

Conversation

ChiuWang commented Sep 29, 2017

Uh oh!

GordonTheTurtle commented Sep 29, 2017

Uh oh!

AkihiroSuda commented Sep 29, 2017

Uh oh!

ChiuWang commented Sep 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thaJeztah commented Sep 29, 2017

Uh oh!

thaJeztah commented Sep 29, 2017

Uh oh!

AkihiroSuda commented Sep 30, 2017

Uh oh!

boaz0 commented Oct 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ChiuWang commented Oct 9, 2017

Uh oh!

AkihiroSuda commented Oct 9, 2017

Uh oh!

ChiuWang commented Oct 9, 2017

Uh oh!

ChiuWang commented Oct 10, 2017

Uh oh!

AkihiroSuda commented Oct 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AkihiroSuda Oct 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AkihiroSuda commented Oct 10, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Oct 11, 2017

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ChiuWang commented Oct 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thaJeztah commented Oct 12, 2017

Uh oh!

ChiuWang commented Oct 12, 2017

Uh oh!

ChiuWang commented Oct 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ChiuWang commented Sep 29, 2017 •

edited

Loading

boaz0 commented Oct 1, 2017 •

edited

Loading

AkihiroSuda commented Oct 10, 2017 •

edited

Loading

AkihiroSuda Oct 10, 2017 •

edited

Loading

ChiuWang commented Oct 12, 2017 •

edited

Loading

ChiuWang commented Oct 12, 2017 •

edited

Loading