Skip to content

Fix link for report's 'go to message' button #709

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

Merged
merged 4 commits into from
Dec 6, 2022

Conversation

Taz03
Copy link
Member

@Taz03 Taz03 commented Dec 2, 2022

The current weird link doesn't seem to work on linux systems.

This is a simple link and used more frequently and works on linux systems too

@Taz03 Taz03 requested review from a team as code owners December 2, 2022 22:11
@marko-radosavljevic marko-radosavljevic added bug Something isn't working priority: major labels Dec 2, 2022
@marko-radosavljevic
Copy link
Contributor

marko-radosavljevic commented Dec 3, 2022

A bit of a background on this PR.

Link buttons with discord:\\ URI scheme/protocol do not work on Linux (confirmed on multiple systems). Noticed on report's 'go to message' button, which is critical for my ability to effectively moderate.

Not sure if those ever worked.


This PR looks fine, but it does intentionally sidestep our own utility for handling URIs, which should be noted. For example, all link buttons generated with it have the same issue.

Which raises the question, what's the appropriate way to deal with this?

Discord URI scheme doesn't seem to be documented at all? How can other systems even support it? How can we even rely on it? How can I learn about it? Am I missing something? 😩

DiscordClientAction isn't that well documented either, and probably outdated:

A few notes;

  • and Android are NOT supported
  • It opens the LAST installed Discord version (Discord, Canary, PTB)

It works on my android, so I guess that's not true anymore.
In my confusion, and in pursuit of answers, I stumbled upon the original PR #391 that states:

A feature should NOT rely on this class, Discord can change something anytime, and iOS + Android aren't supported. They're there to enhance the functionality without making it worse.

So I guess we should offer regular links whenever possible. For example, regular message link in addition to report's 'go to message', and mention (so you can right-click) in addition to modmail's 'author profile'.

@Taz03 Taz03 self-assigned this Dec 3, 2022
@Taz03 Taz03 requested a review from Zabuzard December 4, 2022 13:52
@sonarqubecloud
Copy link

sonarqubecloud bot commented Dec 5, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@marko-radosavljevic marko-radosavljevic changed the title fixed the link for go to message button Fix link for report's 'go to message' button Dec 6, 2022
Copy link
Contributor

@marko-radosavljevic marko-radosavljevic left a comment

Choose a reason for hiding this comment

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

Looks good. ☺️ ❤️

@Taz03 Taz03 merged commit 91137ba into develop Dec 6, 2022
@Taz03 Taz03 deleted the bugfix/go-to-message-button branch December 6, 2022 13:48
@Zabuzard Zabuzard mentioned this pull request Dec 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working priority: major
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants