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

Token-aware text wrapping of commit summaries #9931

Merged
merged 25 commits into from
Jun 5, 2020
Merged

Conversation

niik
Copy link
Member

@niik niik commented Jun 3, 2020

Closes #9185

Description

As described in #9185 our naive text wrapping efforts implemented in #2575 suffer from a major problem when commit summaries contains links that gets wrapped.

image

Since the wrapper operates on the raw text of the commit summary before the Tokenizer class (used by the RichText component) has had a chance to work it happily hard wraps in the middle of a url. When the RichText component then gets a chance to work on it it creates a link out of the link that remains.

This was a limitation that was called out in #2575

[...] a fairly simple fix we think will work alright for now. Note that this might cause links to break if we truncate them in the middle. The solution for this would be to do a pre-pass with the tokenizer and truncate using tokens instead but that's a bigger task that I'm not convinced that we need right now.

In reviewing #9259 it became clear that that assumption no longer held true and we needed to look into a robust way of doing wrapping that was Token aware.

In this PR I've made it possible to provide the RichText component with a list of tokens as well as a string for the component to tokenize. I've also updated the CommitSummary to tokenize both the commit summary and the body before providing them to the RichText component.

Finally I've implemented a token-aware wrapper function that hopefully covers most of the many edge-cases involved in doing this type of wrapping. I've set up https://github.com/niik/commit-summary-wrap-tests as a list of commits with weird behaviors that should be accounted for and intend to keep adding commits there as I encounter them.

Screenshots

Here's a few before and afters

Before
image

After

image

Here the wrapper takes into account the fact that the link has been resolved as an issue link and thereby shortened to just two characters.

Before

image

After

image

Before

image

This illustrates the main problem described in #9185.

After

image

While the link is hard-wrapped both parts are clickable and go to the same URL.

Before

image

Not aware of emojis, splits after 72 characters (counting ':one:` as five characters)

After

image

Emojis are counted as two regular characters so we're able to fit a lot of them in the commit summary.

Notes

This does not solve (but it doesn't make the situation any worse either) the problem of multibyte unicode characters (native emoji). They can still get wrapped in the middle. I don't intend to address that as part of this PR.

Release notes

Notes: [Fixed] Links are no longer broken when text wrapping needs to take place in commit summaries

tierninho
tierninho previously approved these changes Jun 3, 2020
Copy link
Contributor

@tierninho tierninho left a comment

Choose a reason for hiding this comment

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

All looking good when running through various test cases. ✨

Copy link
Contributor

@rafeca rafeca left a comment

Choose a reason for hiding this comment

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

Really nice improvement! ✨

I've added a comment, otherwise the new logic looks good to me!

app/src/lib/text-token-parser.ts Outdated Show resolved Hide resolved
app/src/lib/wrap-rich-text-commit-message.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@rafeca rafeca left a comment

Choose a reason for hiding this comment

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

Thanks for making the array readonly! It's much better! (also the logic in the wrap function).

@niik niik merged commit 582707d into development Jun 5, 2020
@niik
Copy link
Member Author

niik commented Jun 5, 2020

Thanks y'all!

@niik niik deleted the rich-text-hard-wrap branch June 5, 2020 16:17
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.

urls within commits get cut off and cannot get opened by the browser
3 participants