Skip to content

docs: clarify disnake.Embed re-using disnake.File objects when editing messages#786

Merged
onerandomusername merged 8 commits into
DisnakeDev:masterfrom
Snipy7374:master
Oct 31, 2022
Merged

docs: clarify disnake.Embed re-using disnake.File objects when editing messages#786
onerandomusername merged 8 commits into
DisnakeDev:masterfrom
Snipy7374:master

Conversation

@Snipy7374
Copy link
Copy Markdown
Collaborator

@Snipy7374 Snipy7374 commented Oct 1, 2022

Summary

Checklist

  • If code changes were made, then they have been tested
    • I have updated the documentation to reflect the changes
    • I have formatted the code properly by running task lint
    • I have type-checked the code by running task pyright
  • This PR fixes an issue
  • This PR adds something new (e.g. new method or parameters)
  • This PR is a breaking change (e.g. methods or parameters removed/renamed)
  • This PR is not a code change (e.g. documentation, README, ...)

Comment thread disnake/message.py Outdated
@shiftinv shiftinv added s: needs review Issue/PR is awaiting reviews t: bugfix labels Oct 1, 2022
@shiftinv shiftinv added this to the disnake v2.7 milestone Oct 1, 2022
@shiftinv shiftinv linked an issue Oct 1, 2022 that may be closed by this pull request
3 tasks
Comment thread disnake/message.py Outdated
@Snipy7374 Snipy7374 requested review from shiftinv and removed request for onerandomusername October 3, 2022 16:34
Comment thread disnake/message.py
@onerandomusername onerandomusername self-requested a review October 5, 2022 01:23
@Snipy7374 Snipy7374 requested review from shiftinv and removed request for onerandomusername October 6, 2022 16:30
@shiftinv
Copy link
Copy Markdown
Member

shiftinv commented Oct 7, 2022

Hmm, something I completely missed - fp.closed isn't necessarily a good indicator of a file not being usable, that only works with disnake.File(<path>), but not with disnake.File(io.BytesIO(<data>)).

Taking this as an example:

a = await ctx.author.display_avatar.read()
e = disnake.Embed(title="embed")
e.set_image(file=disnake.File(io.BytesIO(a), filename="avatar.png"))

m = await ctx.send(embed=e)
await m.edit("hi", embed=e)

The .edit call will send a zero-length file as the embed image (since the BytesIO wasn't closed yet), again resulting in the original issue.
Can't think of a solution off the top of my head right now, the API handles embed attachments in a weird way.

@Snipy7374
Copy link
Copy Markdown
Collaborator Author

Hmm, something I completely missed - fp.closed isn't necessarily a good indicator of a file not being usable, that only works with disnake.File(<path>), but not with disnake.File(io.BytesIO(<data>)).

Taking this as an example:

a = await ctx.author.display_avatar.read()
e = disnake.Embed(title="embed")
e.set_image(file=disnake.File(io.BytesIO(a), filename="avatar.png"))

m = await ctx.send(embed=e)
await m.edit("hi", embed=e)

The .edit call will send a zero-length file as the embed image (since the BytesIO wasn't closed yet), again resulting in the original issue.
Can't think of a solution off the top of my head right now, the API handles embed attachments in a weird way.

couldn't we introduce a new attribute closed that will be edited when the IO or File Like object will be closed? I could only think about this implementation

@Snipy7374
Copy link
Copy Markdown
Collaborator Author

Snipy7374 commented Oct 15, 2022

Hi, at the moment this implementation works correctly however I would need some advice to make the fix more functional and to solve some problems with pyright :)

@Snipy7374
Copy link
Copy Markdown
Collaborator Author

thanks to @Enegg who helped me improve the code and delete some really shit that i wrote

@onerandomusername
Copy link
Copy Markdown
Member

I feel like this could be fixed simply: document the error at runtime if the embed has any files attached that are not open, and then have a method which can clean the closed files.

@Snipy7374
Copy link
Copy Markdown
Collaborator Author

I feel like this could be fixed simply: document the error at runtime if the embed has any files attached that are not open, and then have a method which can clean the closed files.

Right now this works (at least I haven't found any bug) so why we should change it

Copy link
Copy Markdown
Member

@onerandomusername onerandomusername left a comment

Choose a reason for hiding this comment

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

My biggest concern with this implement as it is is the amount of code and logic that is occuring in a finally of a try/except. If we could somehow minimize that, I would appreciate that.

That could be done by lazily clearing the files from an embed, but I'm also undecided on how this implementation works, it clears the files from an Embed object after they're sent.

However, most embed objects are only used once, so where this currently runs has a runtime impact on all embeds. I think I'd like to see it processed when an embed has a file when we attempt to send it, rather than after the embed is sent.

@Snipy7374 Snipy7374 requested review from onerandomusername and shiftinv and removed request for onerandomusername and shiftinv October 22, 2022 09:20
Copy link
Copy Markdown
Member

@shiftinv shiftinv left a comment

Choose a reason for hiding this comment

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

The docs of Embed.set_image and .set_thumbnail should mention that reusing embeds with files won't work in most cases (not sure on the wording).
looks good to me otherwise

@Snipy7374
Copy link
Copy Markdown
Collaborator Author

The docs of Embed.set_image and .set_thumbnail should mention that reusing embeds with files won't work in most cases (not sure on the wording). looks good to me otherwise

@shiftinv
What do you mean with reusing embeds with files won't work in most cases? Reusing embeds with files works if you don't modify images and if you modify them too

@shiftinv
Copy link
Copy Markdown
Member

What do you mean with reusing embeds with files won't work in most cases? Reusing embeds with files works if you don't modify images and if you modify them too

@Snipy7374 There are a couple cases where it doesn't work, e.g. sending the same embed object in two separate messages; afaict they can only be reused as long as you're editing the same message the embed was sent in originally, right?

@Snipy7374
Copy link
Copy Markdown
Collaborator Author

What do you mean with reusing embeds with files won't work in most cases? Reusing embeds with files works if you don't modify images and if you modify them too

@Snipy7374 There are a couple cases where it doesn't work, e.g. sending the same embed object in two separate messages; afaict they can only be reused as long as you're editing the same message the embed was sent in originally, right?

@shiftinv This is true, I had not thought about it. However while I was testing what you said I found a bug, using the same embed object, changing files and editing a message you get an I/O error message on closed file.

reproducible code:

e = disnake.Embed()
e.set_image(file=disnake.File("..."))
e.set_thumbnail(file=disnake.File("..."))
await inter.response.send_message(embed=e)
e.set_image(file=disnake.File(...))
e.set_thumbnail(file=disnake.File(...))
await inter.edit_original_message(embed=e)
await inter.send(embed=e)

This is because we have to clear _files after an edit attempt too.

@Snipy7374 Snipy7374 requested a review from shiftinv October 23, 2022 21:08
@Snipy7374 Snipy7374 deleted the master branch October 26, 2022 20:14
@Snipy7374 Snipy7374 restored the master branch October 26, 2022 20:22
@Snipy7374 Snipy7374 reopened this Oct 26, 2022
@Snipy7374 Snipy7374 closed this Oct 28, 2022
@Snipy7374 Snipy7374 reopened this Oct 28, 2022
@Snipy7374
Copy link
Copy Markdown
Collaborator Author

https://canary.discord.com/channels/808030843078836254/913779868985090089/1035407513908154378 at the end we decided to just make a docs change

@onerandomusername onerandomusername changed the title fix: disnake.Embed re-using disnake.File objects when editing messages docs: clarify disnake.Embed re-using disnake.File objects when editing messages Oct 30, 2022
@shiftinv shiftinv added t: documentation Improvements or additions to documentation/examples and removed t: bugfix labels Oct 31, 2022
Comment thread disnake/file.py Outdated
Comment thread changelog/786.doc.rst Outdated
Snipy7374 and others added 3 commits October 31, 2022 19:44
Co-authored-by: shiftinv <8530778+shiftinv@users.noreply.github.com>
Signed-off-by: Snipy7374 <davidecucci07@gmail.com>
Co-authored-by: shiftinv <8530778+shiftinv@users.noreply.github.com>
Signed-off-by: Snipy7374 <davidecucci07@gmail.com>
@Snipy7374 Snipy7374 requested review from shiftinv and removed request for onerandomusername October 31, 2022 18:47
Copy link
Copy Markdown
Member

@shiftinv shiftinv left a comment

Choose a reason for hiding this comment

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

On an unrelated note, seems like GitHub workflows are broken - I very much doubt these changes are related to that issue, just figured I'd mention it. I'll look into it in a bit.


edit: fixed in #838

Comment thread disnake/file.py Outdated
Snipy7374 and others added 2 commits October 31, 2022 21:02
Co-authored-by: shiftinv <8530778+shiftinv@users.noreply.github.com>
Signed-off-by: Snipy7374 <davidecucci07@gmail.com>
@Snipy7374 Snipy7374 requested a review from shiftinv October 31, 2022 20:13
@onerandomusername onerandomusername merged commit 43cc09e into DisnakeDev:master Oct 31, 2022
@onerandomusername onerandomusername removed the s: needs review Issue/PR is awaiting reviews label Oct 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

t: documentation Improvements or additions to documentation/examples

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

disnake.Embed methods supporting file param make the embed non-reusable

3 participants