Fix send media group #223
Fix send media group #223
Conversation
- add support for sending TelegramFile.ByByteArray files for all GroupableMedia types, including NPE fix for nameless files - add support for sending TelegramFile.ByFile file for all GroupableMedia types (previously they were not actually uploaded for InputMediaAudio and InputMediaDocument subtypes)
There was a problem hiding this comment.
Getting very late to this one, sorry @naftalmm. Thank you so much for the contribution, I love it! I left a couple of small comments.
| fun from(vararg media: GroupableMedia): MediaGroup = MediaGroup(media) | ||
| fun from(vararg media: GroupableMedia): MediaGroup = MediaGroup(generateUniqueNamesForNamelessByteArrays(media)) | ||
|
|
||
| private fun generateUniqueNamesForNamelessByteArrays(media: Array<out GroupableMedia>): Array<GroupableMedia> { |
There was a problem hiding this comment.
I'm wondering if we should push the decision about the name for the ByteArray files to consumers of the lib. instead (i.e. making filename not optional in TelegramFile.ByByteArray). It feels like they are in a better position to make the right call here, and not forcing it in the API may lead to unexpected results (e.g. they may not know that if they don't put a name in their ByByteArray objects, a random nonamex or whatever will be used instead). And if they don't care about the name, they can use the default of their choice (or we could even provide a default if we think that will be a common use case). What do you think?
There was a problem hiding this comment.
Making filename not optional will be a breaking change. So, if we want to make it non-nullable, we still need to provide some default (but now non-null) value (strictly speaking, it will be breaking change anyway, but only for someone who explicitly passed null for this parameter, repeating current default).
Not sure why I thought that Telegram API requires filenames uniqueness across MediaGroup. Revised their docs, but haven't found it. So, yes, we could provide some non-null default in TelegramFile.ByByteArray constructor (and not worry about filename collision if there will be two TelegramFiles with the same default value in MediaGroup) - it will greatly simlify the code and indeed will make interaction with API more straightforward.
While revising docs for sendMediaGroup found another restriction, we're missing check for:
Documents and audio files can be only grouped in an album with messages of the same type.
Will implement it the scope of this PR, because this very thing happens in the tests I've added. 🙈🙈🙈
| is InputMediaPhoto -> MediaTypeConstants.IMAGE | ||
| is InputMediaVideo -> MediaTypeConstants.VIDEO | ||
| is InputMediaAudio -> MediaTypeConstants.AUDIO | ||
| else -> null |
There was a problem hiding this comment.
Do you think it would be better to be exhaustive here and add all the possible branches? That way, if a new GroupableMedia is added, this code won't compile and whoever is doing the changes will be forced to consider how the new type should be dealt with here.
There was a problem hiding this comment.
Good point. Fixed.
| mutableListOf<MultipartBody.Part>().apply { | ||
| when (val media = groupableMedia.media) { | ||
| is TelegramFile.ByFile -> add(media.file.toMultipartBodyPart(mediaType = mediaType)) | ||
| is TelegramFile.ByByteArray -> add(media.fileBytes.toMultipartBodyPart(partName = media.filename!!, mediaType = mediaType)) | ||
| } | ||
| if (groupableMedia is InputMediaVideo && groupableMedia.thumb != null) { | ||
| add(groupableMedia.thumb.file.toMultipartBodyPart(mediaType = MediaTypeConstants.IMAGE)) | ||
| } | ||
| }.toList() |
There was a problem hiding this comment.
Not important, but just out of curiosity sharing the buildList function (https://kotlinlang.org/api/latest/jvm/stdlib/kotlin.collections/build-list.html) that can be very handy for cases like this.
There was a problem hiding this comment.
Indeed, buildList perfectly suits here, but first we need to bump Kotlin version at least to 1.6. Otherwise, we'll have to mess with @ExperimentalStdlibApi annotations
add medias of MediaGroup compatibility type check
This fixes #221 and other issues related to sendMediaGroup method.
Also had to change the hierarchy of
InputMediaandGroupableMediaclasses/interfaces:GroupableMediaextendsInputMedia(this better emphasize their relationship, since all data classes, which were extendingInputMediaat the same time implementedGroupableMediainterface, and allows to access properties ofInputMediaworking withGroupableMedia)