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

Text formatting overhaul #1406

Merged
merged 6 commits into from
Feb 3, 2023
Merged

Conversation

autumnull
Copy link
Contributor

Description

If this is a code change, please include a summary of what you've coded, and link to the issue(s) it closes/implements.

If this is a documentation change, please briefly describe what you've changed and why.

This pulled request implements all of the changes suggested here as well as a few other so-called "improvements"

Summary of changes:

  • Added minifying step to plaintext to HTML conversion
  • Links in formatted text now don't have their scheme removed (this is kind of incidental, the Linkify extension in goldmark doesn't have an option to remove the scheme)
  • Added Formatter.FromEmojiOnly, which makes use of the existing goldmark parser to find emojis in a text (e.g. a display name) without processing mentions or tags.
  • Converted Formatter.FromPlain to use a modified goldmark parser.
  • Moved all of the logic for "what mentions/emojis/tags are in this text" into the goldmark parser, specifically into the rendering step, so that the context of these is taken into account and only included if the thing is actually active in the text.
  • ...as a result of this: moved a lot of tests around, and deleted some that weren't relevant anymore.
  • Unified the set of characters which can delimit mentions and hashtags into one set (any whitespace or punctuation)
  • Normalize unicode text in hashtags before validating, rendering or adding them to the database.

closes #1271
closes #789

Checklist

Please put an x inside each checkbox to indicate that you've read and followed it: [ ] -> [x]

If this is a documentation change, only the first checkbox must be filled (you can delete the others if you want).

  • I/we have read the GoToSocial contribution guidelines.
  • I/we have discussed the proposed changes already, either in an issue on the repository, or in the Matrix chat.
  • I/we have performed a self-review of added code.
  • I/we have written code that is legible and maintainable by others.
  • I/we have commented the added code, particularly in hard-to-understand areas.
  • I/we have made any necessary changes to documentation.
  • I/we have added tests that cover new code.
  • I/we have run tests and they pass locally with the changes.
  • I/we have run go fmt ./... and golangci-lint run.

Copy link
Contributor

@tsmethurst tsmethurst left a comment

Choose a reason for hiding this comment

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

This is looking really good to me so far, thanks for all your hard work!

I left some comments where I didn't fully understand stuff, but I should also add a disclaimer that:

a) my brain is tired today and
b) text / unicode stuff is far from my specialty in any case

Another question, and again perhaps it's already explained in the code and i just missed it because brain tired: does a remote lookup still happen when you mention an account that hasn't been dereferenced yet? Eg., if I mention @someone@somewhere.com will my instance try to webfinger + GET the AP representation of that account from somewhere.com? It did already, just wanna check that it still does.

internal/text/plain_test.go Outdated Show resolved Hide resolved
internal/text/plain_test.go Outdated Show resolved Hide resolved
internal/text/markdownextension.go Show resolved Hide resolved
internal/text/minify.go Show resolved Hide resolved
internal/text/goldmark_plaintext.go Show resolved Hide resolved
internal/text/replace.go Show resolved Hide resolved
internal/text/goldmark_extension.go Show resolved Hide resolved
internal/util/statustools.go Outdated Show resolved Hide resolved
internal/util/statustools.go Show resolved Hide resolved
internal/util/statustools.go Show resolved Hide resolved
@autumnull
Copy link
Contributor Author

Another question, and again perhaps it's already explained in the code and i just missed it because brain tired: does a remote lookup still happen when you mention an account that hasn't been dereferenced yet? Eg., if I mention @someone@somewhere.com will my instance try to webfinger + GET the AP representation of that account from somewhere.com? It did already, just wanna check that it still does.

I kept all of the logic for ingesting mentions and hashtags, it's just been relocated. Specifically I'm pretty sure that this happens in the parseMentionFunc that used to be called in 2 different places (1,2) but now is only called here. Basically all of the logic you might worry about going missing has been moved into the 2 functions there in internal/text/replace.go.

if err := dbService.conn.NewSelect().Model(tag).Where("LOWER(?) = LOWER(?)", bun.Ident("name"), t).Scan(ctx); err != nil {
if err == sql.ErrNoRows {
// tag doesn't exist yet so populate it
newID, err := id.NewRandomULID()
Copy link
Member

Choose a reason for hiding this comment

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

@tsmethurst do we want to be using NewULID() here or this NewRandomULID()? I know the latter uses a random timestamp.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think NewRandomULID is OK here because we'll probably not be paging through hashtags chronologically (unless you can think of a case where we'll do that?).

@tsmethurst tsmethurst merged commit 49beb17 into superseriousbusiness:main Feb 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants