-
Notifications
You must be signed in to change notification settings - Fork 618
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
Use native upload when relaying from discord #1977
base: master
Are you sure you want to change the base?
Conversation
bridge/config/config.go
Outdated
@@ -166,6 +166,7 @@ type Protocol struct { | |||
URL string // mattermost, slack // DEPRECATED | |||
UseAPI bool // mattermost, slack | |||
UseLocalAvatar []string // discord | |||
UseNativeUpload bool // discord |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this behaviour should be non-configurable (i.e. just the default behaviour) for any bridges where it's possible. I can't think of a good use-cases for some people wanting external links, and others not wanting external links.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(We can always add a setting later on if someone really wants it. Otherwise I don't think it's worth supporting an extra code path here.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason why I had it configurable is because according to the documentation in the wiki a public link is preferable (referenced in the my issue #1965).
For most platforms this actually isn't possible (without using the media server)
Discord is unique in this regard.
So we'd be betraying that "standard" by making it non configurable.
Personally, I'm fine with making it the default discord behavior but it's up to the maintainers/community if that's the route we take.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my bad, i missed that part of the documentation. i will post a comment on your issue to help inform the spec for this pull request
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I updated the CR to not make it an option, should drop the commit for that change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I updated the CR to not make it an option, should drop the commit for that change?
@42wim what do you think?
b038158
to
e4e520e
Compare
1a56f60
to
3ffd96a
Compare
Code Climate has analyzed commit e68ddcf and detected 0 issues on this pull request. View more on Code Climate. |
@42wim I think I fixed the I removed the error return from the handle download because in reality there isn't really much we can do about it other than log it. |
@42wim could you please look into this PR? Seems like a little lines of code, yet a worthy update to a matterbridge :) |
Thanks! This is really cool. I've applied the patch and it seems to work... kind of. The image gets rendered correctly in Element Web and Element Android but not in Cinny. There it just looks like a regular downloadable attachment. I presume the issue is a missing {
"content": {
"body": "foo.jpeg",
"info": {
"h": 512,
"mimetype": "image/jpeg",
"size": 1337,
"w": 512,
"xyz.amorgan.blurhash": "..."
},
"msgtype": "m.image",
"url": "mxc://matrix.org/blablabla"
},
// ...
} The message from Matterbridge looks like this though: {
"content": {
"body": "foo.jpeg",
"info": {
"thumbnail_info": {}
},
"msgtype": "m.image",
"url": "mxc://matrix.org/blablabla"
},
// ...
} |
if err != nil { | ||
b.Log.Errorf("download %s failed %#v", attach.URL, err) | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested the pull request and had the issue that images were not being displayed on Element web. The image name still contained the parameters of the image url. I'd maybe add code to remove the parameters from an url before using it as filename.
urlClean, _, _ := strings.Cut(attach.URL, "?")
helper.HandleDownloadData(b.Log, rmsg, path.Base(urlClean), rmsg.Text, attach.URL, data, b.General)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can confirm that this works for me.
Add option for using a native upload when relaying from discord.
Resolves #1965