Warn user about possibly unexpected behaviour with certain filenames in embeds?

[elenakrittik]

Summary

Warn user when they use try to attach files with non-ascii filenames to embeds.

What is the feature request for?

The core library

The Problem

It seems like Discord escapes spaces and non-ascii characters in attachment filenames, which leads to images in embeds that contain such characters appear outside of the embed when rendered in the client.

The Ideal Solution

User should be warnings.warned when they try to attach a file with "invalid" filename to an embed. .. warning::s in related methods should be helpful too, since (unfortunately) not all users setup logging for their bot.

The Current Solution

This is not an issue on our end, so N/A

Additional Context

Probably the best place to implement warning is Embed._handle_resource().

Credits to @Enegg for originally identifying the issue.

[Enegg]

This implicit name change is not specific to embeds, it's just that it's only there where it has clear side effects. I believe any File attachments undergo this change.

The best idea I could come up with is to create a class level bool attribute on the File class (._validate_filename?), False by default, which would be force set to True instance-level in Embed.set_image(file=...) (similarly to how Embed._default_colour works). Then, a warning would be issued from .send-like methods.

This would allow users to globally enable validation of filenames, as well as per File instance basis, should they wish to be notified about files which names won't be true to their origin.

[Enegg]

A relevant part from the official docs on uploading files

[elenakrittik]

Thanks for your thoughts. Since Discord docs say "must" (whilst currently disnake doesn't care about filenames at all), i think we should implement _validate_filenames as a compatibility attribute to not break existing codebases (along with a global parameter in Client.__init__ to control that attribute), and at some unspecified point later (including "never") remove it. What do you think?

[Enegg]

I think it is not neccesary to make it a definite error, since discord itself does not produce one, instead silently normalizing the filename. Hence, showing a warning should suffice, keeping everything else working as-is.

[elenakrittik]

That's particularly why i added that "including never" part. Since Discord says that's not right, we're not gonna make it look right too, but since Discord doesn't issue a hard error, we won't either. We can keep that attribute defaulting to False for as long as we want, and those users who do want to opt-in for more runtime checks can easily toggle that attribute to True.

[Enegg]

I agree with everything but putting the control parameter in Client.__init__.

[elenakrittik]

Where else then? Or should we tell the user to disnake.File.validate_filenames = True? (assuming the actual implementation would use a class var).

[Enegg]

I imagine File constructor could take an extra keyword-only parameter for the instance-basis warning, thus the note about enabling global validation would go there as well. Can also mention in Embed.set_image that it will issue a warning on incorrect filenames, and then reference the note in File's docstring.

[elenakrittik]

If we implement it this way then globally disabling/enabling validation will be up to the library (which controls the default value for that keyword argument), while it should really be up to the user.

Requiring the user to put validate_filenames=True each time they construct a File doesn't look good to me.

[Enegg]

No no. As I've already described in my first comment, the keyword argument is for per instance basis warning. If you wish for it to always apply, you set the class var/use a classmethod for that. Perhaps, the constructor level argument could even raise actual exception.

[elenakrittik]

Alright, then good.

[shiftinv]

my 2c on this issue:
This is definitely something users should be made aware of, but I'm not fully on board with the proposed implementation yet.

Overall it only concerns embeds (or more generally, attachment://). The automatic removal/replacement of invalid characters isn't a concern otherwise - hence I would just leave the File implementation as-is, and only look at the embed code.

A .. note:: on the File object regarding the character escapes (and transformations, e.g. ä gets turned into a as well) and a .. danger:: on embed methods which take a File would make sense.

In addition, I'd say raising an exception in Embed._handle_resource seems sensible, since the allowed character set is officially documented. I don't think there's a point in just warning the user here, since it will just not work the way it's supposed to anyway.