[Editor Help] Add support for embedded and remote images in the class reference pages.

[bruvzg]

Following up #69712 (comment)

• Add method to update RTL images without changing the text.
• Add support for [img]http(s)://image_url[/img] in the documentation, to load remote images (downloaded and cached).

Usage Example

<brief_description>
	Axis-Aligned Bounding Box.
	[img]https://godotengine.org/storage/app/uploads/public/638/8d9/a60/6388d9a60ac95864295672.jpg[/img]
</brief_description>
<description>
	[img]editor://Logo[/img]
	[AABB] consists of a position, a size, and several utility functions. It is typically used for fast overlap tests.
	It uses floating-point coordinates. The 2D counterpart to [AABB] is [Rect2].
	Negative values for [member size] are not supported and will not work for most methods. Use [method abs] to get an AABB with a positive size.
	[b]Note:[/b] Unlike [Rect2], [AABB] does not have a variant that uses integer coordinates.
	Loading (bad url): [img]http://192.168.3.210:34/f4f/dopefish.jpg[/img]
</description>

Screen.Recording.2022-12-08.at.10.10.08.mov

[YuriSizov]

• Add support for [img]editor://Logo[/img] in the documentation, to load embedded editor icon (get_theme_icon("Logo", "EditorIcons") equivalent).

If we decide to bundle some images with the built-in docs, I'm not sure we're going to use themes or the EditorIcons type specifically. Maybe this shouldn't be implemented until we have a better idea for how to use it.

[bruvzg]

I'm not sure we're going to use themes

Why not, if we will use bundled images it would only make sense for lightweight SVG diagrams which would benefit from using theme colors. But it's two lines of code, so removed it for now.

[Calinou]

Should [img] syntax allow specifying a placeholder wioth and height to use until the image is loaded? This would avoid reflows while images are loading, which can be distracting and have a performance impact.

[KoBeWi]

I tried embedding the Tween cheat sheet to test it (btw you could do it in your PR, so the feature is actually used) and the image is too big:

There's also no horizontal scroll.

[fire]

I recommend embedding svg images if possible. There may be a processing cost though.. Not sure

[bruvzg]

Should [img] syntax allow specifying a placeholder wioth and height to use until the image is loaded?

Added [img w,h] support.

What if the remote image was updated?

Updated to make request with If-Modified-Since: {cache_file_modification_date} header first instead of loading cached image directly.

[KoBeWi]

width field does not work. Using it makes the image positioned as if it had the specified width, but it has original size:

[img width=60]https://godotengine.org/storage/app/uploads/public/638/8d9/a60/6388d9a60ac95864295672.jpg[/img]

Not sure if related to this PR or some RTL bug.

[YuriSizov]

There is full intention to use images in the documentation, it's important. Feature doesn't need to be bundled with content it enables, because frankly it's not bruvzg's concern to add this content. This is a big side-track about nothing at this point.

the Godot Privacy Policy should be updated.

It needs to be updated as is, as it doesn't include export templates. And it doesn't mention that we use GitHub now (and who knows what they collect).

[Faless]

This is a big side-track about nothing at this point.

Well, that's your point of view, and dismissing my point like that is quite uncalled for.

I believe this is a very important topic, that deserves proper discussion.

Anyway, I explained my concerns, I rest my case.

[YuriSizov]

I'm not dismissing your concerns. In fact, I've addressed them and presented solutions. I'm simply pointing out that they aren't related to this PR which adds a needed feature. It doesn't define how we would use it, and it can't in any way update the privacy policy on the website. It's a separate task that can only be coordinated and performed after we have the feature ready.

[fire]

From what I see the request is that:

1. update the privacy policy on the website
2. specific option to disable remote fetching from the Help

Block this pr.

[Faless]

I'm simply pointing out that they aren't related to this PR which adds a needed feature

Disabling image loading by default, or prompting to ask for download, is totally part of this feature, and should be part of the PR.

It's a separate task that can only be coordinated and performed after we have the feature ready.

No, updating the Privacy Policy does not depend on this PR, and can (and should) be done before it.

[bruvzg]

I think we should have a global editor toggle that disables all online interactivity (or enables it, if we disable it by default to be more mindful of privacy). But that's out of scope of this PR.

Not sure if fully disabling by default is the best option, I would do Ask for new sources, Always load, Never load global setting (with Ask as a default value) and a second setting with a list of user approved sources.

And Show remote content, Always show remote content from X and Never load remote content buttons on the help page. If Never load is clicked, editor setting is updated and all images are replaced with flat links.

This way users will be aware of the options and nothing is loaded without consent.

[YuriSizov]

No, updating the Privacy Policy does not depend on this PR, and can (and should) be done before it.

No, it cannot be done before if we don't intend to merge this PR because you are blocking it. The privacy policy should reflect only what's actual.

Disabling image loading by default, or prompting to ask for download, is totally part of this feature, and should be part of the PR.

This PR implements the framework for better handling of images embedded into the docs. It doesn't have to also implement an entirely separate framework for managing user preferences about internet privacy. Because it doesn't add images itself. A follow-up can deal with that, implemented by anybody.

[bruvzg]

It doesn't have to also implement an entirely separate framework for managing user preferences about internet privacy. Because it doesn't add images itself. A follow-up can deal with that, implemented by anybody.

Optional loading probably should be implemented in this PR, it's not adding any images, but enables third-party plugins/scripts with documentation to use this feature.

[YuriSizov]

Not sure if fully disabling by default is the best option, I would do Ask for new sources, Always load, Never load global setting (with Ask as a default value) and a second setting with a list of user approved sources.

The way I view it, privacy is a bigger problem than just help. Asset library is way worse and more concerning, because it deals with 3rd party content. So I think the option should be to disable all internet activity by default and display appropriate GUI elements that there is web content that can be loaded should the user choose to, explaining the implications.

So I wouldn't bother too much with granular settings for the help specifically, since the asset library would be even more complicated with images that don't have to pass through a strict validation that applies to engine contributions.

[YuriSizov]

Optional loading probably should be implemented in this PR, it's not adding any images, but enables third-party plugins/scripts with documentation to use this feature.

As mentioned before, plugins and scripts can do much worse things than load images from the internet. Same as testing user created games, for instance. This is always a danger and something we must also address in a more global, general way.

[Faless]

That's not how privacy policy works. Privacy policies MUST be updated before features are implemented, to give time to the users to be informed (which I admit, in the case of Godot is hard to estimate how long it is, but definitely not AFTER the fact).

…

On Mon, Aug 7, 2023, 20:44 Yuri Sizov ***@***.***> wrote: No, updating the Privacy Policy does not depend on this PR, and can (and should) be done before it. No, it cannot be done *before* if we don't intend to merge this PR because you are blocking it. The privacy policy should reflect only what's actual. Disabling image loading by default, or prompting to ask for download, is totally part of this feature, and should be part of the PR. This PR implements the framework for better handling of images embedded into the docs. It doesn't have to also implement an entirely separate framework for managing user preferences about internet privacy. Because it doesn't add images itself. A follow-up can deal with that, implemented by anybody. — Reply to this email directly, view it on GitHub <#69751 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAM4C3SNHHW32OZZAO7RPKTXUEZQFANCNFSM6AAAAAASX2UYSY> . You are receiving this because you commented.Message ID: ***@***.***>

[YuriSizov]

That's not how privacy policy works. Privacy policies MUST be updated before features are implemented, to give time to the users to be informed (which I admit, in the case of Godot is hard to estimate how long it is, but definitely not AFTER the fact).

But we can't update it if you block the PR and therefore the feature would never be allowed, Fabio. It's a catch 22. We first must agree on this PR, we can't update the policy before that.

[Faless]

You can absolutely update it, I don't understand why you keep saying you can't. The fact that you say that you can collect info in one scenario doesn't mean you must do that. Also, the asset library and the help setting should definitely be separated. They are 2 different things, with a different threat model. And forcing people to an all or nothing makes no sense. Email apps asks you to download images, they don't tie that to also disabling extensions support. They are two completely different things.

…

On Mon, Aug 7, 2023, 20:51 Yuri Sizov ***@***.***> wrote: That's not how privacy policy works. Privacy policies MUST be updated before features are implemented, to give time to the users to be informed (which I admit, in the case of Godot is hard to estimate how long it is, but definitely not AFTER the fact). But we can't update it if you block the PR and therefore the feature would never be allowed, Fabio. It's a catch 22. We first must agree on this PR, we can't update the policy before that. — Reply to this email directly, view it on GitHub <#69751 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAM4C3UJDX7HV2OJUKGURFDXUE2JRANCNFSM6AAAAAASX2UYSY> . You are receiving this because you commented.Message ID: ***@***.***>

Irrelevant.

[clayjohn]

Its probably time for everyone to take a step back from this discussion. It is clear there are some non-technical issues that require discussion. This isn't the kind of change that can be made without a proper discussion including relevant stakeholders.

But to center the discussion (which should take place in a different place):

1. We need to discuss whether this feature is worth adding and maintaining (i.e. considering disadvantaging users without constant internet, considering the maintenance burden of keeping images up to date etc.)
2. We need to discuss whether we want to allow the editor (outside of the asset library) to access the internet at all.
3. If we are happy with accessing the internet from the editor help, we should agree on how the images are displayed (this is more of a technical issue) (i.e. push button to download, automatically download etc.)
4. Then finally we can discuss how this impacts our privacy policy and make the necessary changes to ensure that we are in compliance with privacy laws at all times

[Faless]

I can suggest, given this PR also adds other features too, notably:

• Add method to update RTL images without changing the text.
• Add support for local [img] tags (embedded images)

That we could strip the http(s) download part from this PR, while leaving the above features.

And move the download part (EditorHelpImageDownloader) to a separate PR/discussion.

[Zireael07]

Seconding @Faless - I totally missed the "remote images" part and was looking forward to seeing local images in help.

[bruvzg]

Extracted RTL only part to #80410

I'll update this PR with the main client like remote content prompts a bit later.

[fire]

Was Add support for local [img] tags (embedded images) extracted to a pr?