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

non-URI characters should be excluded from the link parser #4769

Closed
4 tasks done
brian6932 opened this issue Aug 16, 2023 · 3 comments · Fixed by #5509
Closed
4 tasks done

non-URI characters should be excluded from the link parser #4769

brian6932 opened this issue Aug 16, 2023 · 3 comments · Fixed by #5509
Labels
issue-report An issue reported by a user.

Comments

@brian6932
Copy link
Contributor

brian6932 commented Aug 16, 2023

Checklist

  • I'm reporting a problem with Chatterino
  • I've verified that I'm running the most recent nightly build or stable release
  • I've looked for my problem on the wiki
  • I've searched the issues and pull requests for similar looking reports

Describe your issue

Currently when you use a valid tld, the link parser doesn't check whether the domain characters are valid or not.

There are really 2 approaches you could go with this, you can either URI encode the characters within the white-space, imo that's unnatural and doesn't feel right for a chat program. The better thing to do is just exclude them like webchat does.

Screenshots

c2:
image
image
webchat:
image

OS and Chatterino Version

Chatterino Nightly 2.4.4 (commit 381d5c4) built on 2023-08-14 with Qt 5.15.2, MSVC 193532217 Running on Windows 10 Version 2009, kernel: 10.0.22621

@brian6932 brian6932 added the issue-report An issue reported by a user. label Aug 16, 2023
@brian6932 brian6932 changed the title non-URI characters should not be included in the link parser non-URI characters should be excluded from the link parser Aug 16, 2023
@cbclemmer
Copy link
Contributor

Added a PR to help with this #4778

@Felanbird
Copy link
Collaborator

I would prefer if the approach to fix this tackled the most user-facing problems, compared to patching tiny situations where a non-existent link is shown. Though this would likely require more of a refactor as it would result in two message elements.

Similar to the above mentioned () problem, links that trail with . are a more common user-facing problem. When this happens it's often due to a user writing a bot command, and ending the sentence with a ., which browser chat will ignore but Chatterino does not. In this case unless the link host handles this, the link will fail to load.

examples that automatically handle this problem:
youtube

examples where the link fails to load
github
twitch
runelite's plugin-hub
dashduck's website | working version https://drscovy.com/raids

@Nerixyz
Copy link
Contributor

Nerixyz commented Aug 22, 2023

I think parsing a link like this should work:

prefix                   link                          suffix
╭┴╮╭───────────────────────┴───────────────────────────╮╭┴─╮
 (  https://  foo.bar.baz.app  /my/path?query=alien#abc  ).
   ╰───┬────╯╰──────┬────────╯╰───────────┬────────────╯ 
    protocol      host                  rest

This could be stored in a result like this1:

struct Link {
    QStringView prefix;
    QStringView link;
    QStringView suffix;

    QStringView protocol;
    QStringView host;
    QStringView rest;

    QString source;
};

If prefix isn't empty, a text element is added for that. Then the link is added and finally the suffix. This way, the link would be properly highlighted.

The possible prefixes should be (. The possible suffixes should be ), ., ,, :, ). ),, ):.

Footnotes

  1. 🤓 On Qt 5.12 the string-views would be refs, because string-view support there is less than minimal.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
issue-report An issue reported by a user.
Projects
None yet
4 participants