Replace SplitStringAtByteN with EllipsisStringWholeWord
#31822
Conversation
GiteaBot
added
the
lgtm/need 2
github-actions
bot
added
modifies/api
| steps[i] = &ActionTaskStep{ | ||
| Name: name, |
| runner := &actions_model.ActionRunner{ | ||
| UUID: gouuid.New().String(), | ||
| Name: name, | ||
| Name: base.EllipsisStringWholeWord(req.Msg.Name, 255), |
There was a problem hiding this comment.
Why are we changing an API response in the first place?
Let's leave it to the caller to handle that case.
| } | ||
| } | ||
| act.Content = fmt.Sprintf("%d|%s", issue.Index, truncatedContent) | ||
| act.Content = fmt.Sprintf("%d|%s", issue.Index, base.EllipsisStringWholeWord(comment.Content, 200)) |
|
Now I see the whole story. If this PR is to fix #31780, I agree with @delvh, it should let the frontend code to handle it. And some |
| assert.Equal(t, "foo bar…", EllipsisStringWholeWord("foo bar foo", 10)) | ||
| assert.Equal(t, "foo bar foo", EllipsisStringWholeWord("foo bar foo", 11)) | ||
|
|
||
| assert.Equal(t, "测试文本…", EllipsisStringWholeWord("测试文本一二三四", 4)) |
There was a problem hiding this comment.
The truncation should be done at "byte" level, but not "rune" level.
The problem of "rune" level is that CJK chars will be too much longer than expected.
assert.Equal(t, "foo…", EllipsisStringWholeWord("foo bar", 4))
assert.Equal(t, "测试文本…", EllipsisStringWholeWord("测试文本一二三四", 4))
See the difference. That's why the old function is "SplitStringAtByteN"
(History: #17928 (review) )
There was a problem hiding this comment.
Sure, it was intended to be at rune level because EllipsisString works on rune level too. Didn't thought about the database restriction here because I would expect a hard cut.
There was a problem hiding this comment.
Actually there is usually no "database restriction", because VARCHAR(10) could store 10 CJK runes or emoji runes (say, 30 - 40bytes) if its charset/collation is UTF-8 based.
I think the real problem is that CJK chars render much longer than ASCII chars with the same rune length on the UI ......... for example, assume one ASCII char's width is about W.
- 9 CJK runes: about W * 9 * 2 = W * 18
- 9 bytes CJK chars: W * 3 * 2 = W * 6
- 9 bytes ASCII chars: W * 9
So "9 CJK runes" exceeds the width limitation too much on the UI, while usually "9 bytes CJK chars" won't exceed the expected "9 bytes ASCII" width.
(I don't mean blocking for this change, just share the backgrounds, maybe it's worth to document the details no matter what method is finally chosen)
There was a problem hiding this comment.
-> Refactor "string truncate" #32984
|
-> Do not render truncated links in markdown #32980 |
Fixes #31780
The fix is in
services/feed/action.go, the other changed places areSplitStringAtByteNcall which discard the remainer.This does only fix new texts. Existing action feed entries are not altered.