Discord: Download files, don't send as URLs#44
Conversation
|
Heck, freaking linter, actually I am not "gopher", I am C/C++ developer, and the rest of stuff I done, just done on the production server during debugging (I made many other fixes including this, because it just failed with several cases already), written via |
| url = attach.URL | ||
| name = attach.Filename | ||
|
|
||
| err := helper.HandleDownloadSize(b.Log, &rmsg, name, int64(attach.Size), b.General) |
There was a problem hiding this comment.
No expert at all here, but it might be that the linter doesn't like that err wasn't declared as a variable name for some reason?
At least the possible solutions here make it sound like this should fix it?
There was a problem hiding this comment.
I always find golang so confusing with := vs = it's like the exact opposite of ergonomic! Here variable err was already defined so you should be doing err = instead of err := (assignment to a new variable shadowing previous assignment).
There was a problem hiding this comment.
Looks like there is another same one at line 118.
|
|
||
| // no empty messages | ||
| if rmsg.Text == "" { | ||
| if rmsg.Text == "" && len(m.Attachments) == 0 { |
There was a problem hiding this comment.
Maybe we should check the length of rmsg.Extra['file'] instead of the original number of attachments, because we are checking for max allowed size and blacklisted files, so we may end up with 0 attachments to forward despite having some incoming.
There was a problem hiding this comment.
I didn't knew about that field, primarily because I didn't investigated entire project and I wanted to implement the most simple thing that just work, I'll try to place the thing, but I am not sure WHAT stuff that field contains? Array? Structures?
There was a problem hiding this comment.
Anyway, to properly finish that (I mean the full pile of changes) I will need to set up go compiler on my local system and debug thing locally, not on production server (that I usually alternate only when any troubles gets bringed up).
There was a problem hiding this comment.
Sorry for the extra trouble. It is much appreciated if you could look into this further while we try to fix some other general linter issues.
There was a problem hiding this comment.
So here this is the method called when a message is received from Discord. So you're generating a new config.Message instance (rmsg) that will be passed to other bridges (b.Remote <- rmsg).
That check is meant to prevent empty messages from reaching other bridges. So you should check if after the logic has been applied (eg. preventing too big files), some files have been added to that rmsg.Extra['file'] which contains info about attachments. If you check m.Attachments, you're checking if there were attachments in the original Discord message, not if those attachments have been successfully "imported" in the matterbridge message.
| } | ||
| first = false | ||
| } else { | ||
| caption = "" |
There was a problem hiding this comment.
So on discord only the first image can have a caption? Or am i missing something?
There was a problem hiding this comment.
Discord has one united caption for all the images sent in the pack. Probably I should merge them into list?
- like
- this
To make it display all the captions properly?
There was a problem hiding this comment.
I'm not sure what should be the policy here, when there's a united caption for a bunch of attachments. Should we apply it only on the first attachment? On all attachments? In all cases, this is beyond this specific bridge so i'd be happy to merge anything, but we should definitely document that kind of expected behavior in the guide to write new bridges (and align existing ones with expectations).
|
Hello, thanks for taking the time to write and test this! I have a few minor review comments, but i have one more fundamental question: why do I'm also curious if you would be interested in becoming a maintainer for the discord bridge alongside @poVoq. It's not exactly more responsibilities, but we'd ping you to review and test discord changes before they are merged. |
d438615 to
fd5b697
Compare
|
Hello @Wohlstand do you think you would have some time to take care of the changes requested here soon? If you have no time in the coming weeks, i can take care of it myself since this is a high-priority PR for @poVoq. Just say the word whatever you prefer. My review comment about |
|
Now that #59 is merged can we get this fixed up ready for merge as well? |
|
I didn't yet worked on the stuff, because I was busy on other thingies, and I would like try to set up a kind of local IDE to properly apply these tweaks and test the stuff locally. As I already said I developed these thingies just using mcedit in terminal and debugged locally on production server with a goal to just get it working as I need. |
And make that as an option
fd5b697 to
d608e6f
Compare
|
Rebased and updated |
|
I am testing this right now, but it doesn't seem to make a difference, I still get the Discord CDN links on IRC and XMPP despite having the Matterbridge mediaserver configured. Does this require some additional setting to work? And if so, please add a change to the documentation for it. Also: can you please include a changelog entry for this change? Thanks! |
I may guess something got been broken... The original code that I developed, worked flawlessly. |
|
And ye, I do have the file sharing proxy configured on the bridge with a web server frontend. So, files gets re-uploaded to the mediaserver. Actually, let me try some again... |
|
Now the question: DID you properly set the |
Ideed that was the issue. With But that is what I meant with documentation missing in your PR. |
I basically didn't care about docs, but now it's a time. I will try to add documentation note then. |
|
Thanks! |

My old tweak that allows re-uploading of files from Discord to destination server (Telegram, Matrix, or the MediaProxy if destination is not available).
Fixes #37