File forwarding #50
File forwarding #50
Conversation
… Mobile) Anything related to Embeds is untested
| { | ||
| await Send.SendErrorWithDeleteReaction(context, "This server has reached its limit of 50 messages for the month. To lift this limit, ask an admin or moderator to upgrade your server to Voltaire Pro. (This can be done via the `!volt pro` command.)"); | ||
| return; | ||
| } | ||
|
|
||
| var prefix = PrefixHelper.ComputePrefix(context, dbGuild); | ||
| var channel = candidateChannels.OrderBy(x => x.Name.Length).First(); | ||
| var messageFunction = Send.SendMessageToChannel(channel, replyable, context, dbGuild.UseEmbed); | ||
| await messageFunction(prefix, message); | ||
| if (context.Message.Attachments.Any()) |
There was a problem hiding this comment.
Would it be possible to do this check and the iteration inside of Send.SendMessageToChannel so we don't have this conditional logic everywhere? If we do go this route, I think the first thing to do here would be to determine if the supplied context has attachments.
There was a problem hiding this comment.
You're right that this would be preferable, and it was in fact how I did it initially. It worked fine if a message had a single attachment, but when there were multiple, I noticed that the whole Func<string, string, Task<IUserMessage>> function is supposed to return only a single IUserMessage Task, and I couldn't really figure out a way around that (as the messageFunction is awaited all the way back in SendToGuild).
Doing the logic outside of the Async area was the easiest way I could find to make sense of it.
There was a problem hiding this comment.
Right - could we, for example, return a function that returns that last IUserMessage? Or an array of them?
There was a problem hiding this comment.
as they are awaited inside of SendToGuild, we can't really just return the last one. It would have to be a list.
I'm not very familiar with Async flows so I'm not sure if it can be an ordinary list or if it has to be some kind of Async List.
My issue with using an Async List is that the attachments might have their order jumbled, which would be a problem if the message text was only attached to the first file.
If we do go that route, Task.WhenAll() should be enough.
There was a problem hiding this comment.
Yeah - the caller is what is doing the awaiting, but I think all of the callers are essentially just awaiting the returned message. We may have to alter the inner workings, but we should be able to have this thing await the returned function and have it receive the result of sending the last message, I think.
I'm happy to take a whack at this at this week some point 🙂.
There was a problem hiding this comment.
I'd appreciate that, I'm struggling a little to get my head around async, hence the current state of my changes.
There was a problem hiding this comment.
Oh, one more issue with just returning the last message;
sendReply and SendDirectMessage both add the Delete React to each message that gets sent. if you look at those, the Send.AddReactionToMessage is inside the loop, to let users delete each image individually.
| for (int attachmentIndex = 0; attachmentIndex < context.Message.Attachments.Count; attachmentIndex++) | ||
| { | ||
| var messageFunction = Send.SendMessageToChannel(channel, replyable, context, dbGuild.UseEmbed, attachmentIndex); | ||
| await messageFunction(prefix, message); |
There was a problem hiding this comment.
If a message is sent with the attchments, does the message end up being sent multiple times?
There was a problem hiding this comment.
This was the case until I added the ternary operations in the SendFileAsync lines.
My implementation of the attachmentIndex was a lazy fix to retrieve the index of the current attachment that was being iterated, to ensure that the attachments were being send in order.
Initially, if the message was supplied with each request, it would repost the text alongside each attachment, however I used the attachment index attachmentIndex == 0 ? message : "" to remove it.
| @@ -31,32 +35,56 @@ public static async Task PerformAsync(ShardedCommandContext context, string chan | |||
| } | |||
| } | |||
|
|
|||
| public static Func<string, string, Task<IUserMessage>> SendMessageToChannel(IMessageChannel channel, bool replyable, ShardedCommandContext context, bool forceEmbed = false) | |||
| public static Func<string, string, Task<IUserMessage>> SendMessageToChannel(IMessageChannel channel, bool replyable, ShardedCommandContext context, bool forceEmbed = false, int attachmentIndex = -1) | |||
| { | |||
There was a problem hiding this comment.
(to continue my previous comment on shifting the check into this function)
If we can do the logic here - I think the goal would be to abstract the check for the existence of files to the top of this function, and leave the branching below agnostic. You may need to add another layer of abstraction, but the branching below shouldn't need to occur in multiple places.
There was a problem hiding this comment.
As it is, there are 4 conditions that change the text that is passed along into the message. Adding the attachment check makes a total of 8 options.
Instead of doing the attachments check before this and branching before the text has been finalised, it might be better to use the 4 branches to generate the text and then pass along the resulting message text into another function, which can then determine what to do about attachments, with the text just being 1 input argument. This function would return a Task<IUserMessage> back to the Send.SendMessageToChannel
There was a problem hiding this comment.
Yeah - I'm fine with either option. I think it would need to do something like that, in fact. Some way to capture which message function should be used. This could be another function that is called which wraps SendMessageAsync, or a function which is always called by the returned function.
Allow Voltaire to forward files that it receives as attachments.
If it were just single attachments the flow would be quite simple, but Discord Mobile allows for multiple attachments in one message, which the Discord.NET API does not allow. The flow got quite messy as a result but it works.
While most of the code is still Asynchronous, the changes were made so that the attachments are still sent in the same order they arrived, inside the original message.
I haven't used any lambdas (yet), just simple old Java-like logic.