Create a field when Ctrl-dropping a resource into the code editor

[JoNax97]

The code editor now creates a const field when dropping a resource into the code editor while CTRL/CMD is held, similar to what it does for nodes.

Closes godotengine/godot-proposals#7673

Additionally, it fixes some inconsistencies that I found while testing the different cases (see the snippet below)

# Dragging a single resource works just as it did before.
"res://a_folder/an_icon.svg"

# Dragging a single resource while holding CTRL now adds a constant.
const ICON := preload("res://a_folder/icon.png")

#Dragging a script resource while holding CTRL creates a constant with PascalCase naming.
const AScript = preload("res://a_script.gd")

# Dragging multiple resources and/or folders in 4.1 caused an "Expected newline after comment string" syntax error.
"res://a_folder/", "res://a_scene.tscn"

# Dragging multiple resources and/or folders to an empty line now creates valid syntax.
"res://a_folder/"
"res://a_script.gd"

# Dragging multiple resources and/or folders to a non-empty line drops everything into 1 line.
var list = [ "res://a_folder/icon.png", "res://a_scene.tscn" ]

# Dragging multiple resources and/or folders to a non-empty line while holding CTRL skips the variables and drops everything into 1 line.
var list_2 = [ preload("res://a_folder/icon.png"), preload("res://a_scene.tscn") ]

# Dragging folders while holding CTRL in 4.1 would try to preload them, causing a syntax error.
preload("res://a_folder/")

# Dragging multiple resources and/or folders while holding CTRL now only preloads valid resources.
"res://a_folder/"
const AScript = preload("res://a_script.gd")

# Dragging an internal resource in 4.1 was missing quotation marks, resulting in a syntax error.
res://a_scene.tscn::CanvasItemMaterial_rvlbh

# Dragging an internal resource now is correctly quoted.
"res://a_scene.tscn::CanvasItemMaterial_rvlbh"

# Dragging an internal resource while holding CTRL is not allowed.
# The following error toast is displayed instead:
"Preloading internal resources is not supported"

[mieldepoche]

nice work!
Some observations:

• Naming doesn't work as expected when filenames have a dot in them:

• Dragging multiple resources and/or folders now creates valid syntax

I think this should only happen when dropping on an empty line though, since that particular case (with the commas) is wanted so you can easily put several resource path in an array. And if you ctrl-drop them, they all get preloaded too.

previous behavior	your pr
simplescreenrecorder-2023-09-16_03.56.57.webm	simplescreenrecorder-2023-09-16_03.55.48.webm

[JoNax97]

Fixed the variable name error and restored the previous behavior (no variable, comma separated elements) for non-empty, non-whitespace lines only. This should address the use case of filling an array as described by @mieldepoche

[mieldepoche]

I tested the pr and everything works as expected now! (at least I didn't find any bug).

One thing I stumbled upon is that you can drag an internal resource before saving the scene (just after creating it), so it ends up with no valid path (either "" or "res://a_scene.tscn::" depending on where it is). Maybe there should be an error message when trying to drag an unsaved resource that says "can't reference an unsaved resource, please save your scene first" or something like that? (note: I assume that it's what's happening here)

simplescreenrecorder-2023-09-19_10.46.10.webm

[JoNax97]

I think it should be fixed now. Thanks @mieldepoche for your very detailed testing!

[JoNax97]

For the "unsaved internal resource" error, I tried both a modal warning and a toast.

Is there a guideline about which to use? Personally, I find the toast less intrusive.
Also, what do you think about the text?

[mieldepoche]

Is there a guideline about which to use

I don't know.
I too find the popup more intrusive and painful (because it requires you aiming on a small button with the mouse).
But the toast can be hidden so you can miss the message.

[Calinou]

Should a const be created instead of a var since preload() is used? See godotengine/godot-proposals#7841.

[JoNax97]

If that's desirable then sure, I can implement it.

A couple questions though:
Does it provide any concrete advantage?
Are we sure there's no use case for overwriting the variable later?
Should the same be done for dropped node paths?

[mieldepoche]

Does it provide any concrete advantage?

it prevents you from changing it by accident (at least it should), but I'm sure you know that :p
Other than that, idk.

Are we sure there's no use case for overwriting the variable later?

I think that this is a niche case. Most of the time, you'll preload your resource and that's it.

Should the same be done for dropped node paths?

You can't.

The question of casing arises though:

• I don't agree with Smartly create a new variable when Ctrl-dropping a resource in an empty script line godot-proposals#7673 (comment)_ (which was posted in the issue that this pr adress), the constant name should not be in all caps, although I guess it's about personal taste but I prefer to keep the constant case (all caps) for numerical constants, not Objects. (because I find MY_OBJECT.function() ugly). I mean converting between constant and snake case is a shortcut away but idk. Right now all of the demo projects use variables and therefore snake_case for their preloads for example. I've seen people use const snake_case = preload(...) on streams, etc.
• regardless of that, preloading script should use PascalCase for the constant name, as per the docs:

Also use PascalCase when loading a class into a constant or a variable:

const Weapon = preload("res://weapon.gd")

[JoNax97]

I feel preloading classes has become pretty much redundant with class_name. I'm not sure it warrants an special case.

I also dislike the uppercase naming. At any rate it should be controllable by a setting.

I agree with switching to const though, it encourages good practices

[mieldepoche]

I feel preloading classes has become pretty much redundant with class_name.

I never use it myself, but I think ideally it would be nice to comply with the style guide. I think people use it to load a type locally, to avoid global namespace pollution (at least that's what I remember being cited).
That being said, I'm not the one doing the work 😅 it didn't sound hard to implement in my head, but I have no idea.

[JoNax97]

I think it would be good to have some input from the editor team to see if the extra complexity is warranted.

Also, as I said in the proposal:

Now that I think about it, why would always assume that the correct action is to preload into a constant?

Isn't loading into a variable an equally valid option?
At any rate, it makes less assumptions about user intent.

Any opinions @Calinou @YuriSizov @KoBeWi ?

[KoBeWi]

I think preloading a script as PascalCase const is a nice feature. I actually thought about implementing it recently. Note that this is the only way to use it as an actual type, as var won't work.
As for other resources, it doesn't really matter. They can be var.

I think people use it to load a type locally, to avoid global namespace pollution (at least that's what I remember being cited).

This. I have an addon that has 7 preloaded "classes", because they don't need to be global.
Also a class_name script with preloaded scripts can be used as a namespace.

[JoNax97]

For this particular case, should we still respect type hints setting?
const NewScript: GDScript = preload("res://new_script.gd")

[JoNax97]

Updated OP to reflect the new cases.

[KoBeWi]

You need to rebase the PR. When doing force-push, always try to be up-to-date with master.

[mieldepoche]

I tested the last changes a bit and everything seem to work as expected! and I didn't find any bug.

nitpicking:

Shouldn't this be Test_ScriptTest? If so I guess it's a bug in godot's to_pascal_case (which I assume you're using). Idk if that's important.

one last thing about `const`

After testing a bit, I'm actually even more convinced it's better to const things by default, because it make the lines shorter/less heavy (since no explicit typing is required).
It's up to personal preference I guess, so I won't fight over it, but I had one last idea: maybe const could be behind a ctrl+shift modifier? optionally displaying an help message in the same way the 2D editor does it?
I don't realize if it's bloat at this point and the PR fully solves the issue I had already.

dropping a texture in the 2D canvas	how it would behave in the script editor

It could also be an editor setting.

[JoNax97]

Re: const vs var: I don't think this warrants the extra complexity of a setting.

If using const is preferred and has no obvious drawbacks, I will just make it so. It is the official style, after all.

I can keep the actual behavior behind the shift key.
If that's not desired, users can still type "var name =" manually and drop the preload part

[JoNax97]

If we were to keep the "var" option behind the shift key, would someone prefer to also use "load" instead of "preload"?

That was the only real use case where var is actually needed but I don't know if there's any demand for this.

[mieldepoche]

would someone prefer to also use "load" instead of "preload"

I think it's really an edge case, most of the time you want to preload the resource.
afaik preload will load the resource on script load, and load will do on resource instantiation from that script.

so like if I get things correctly:

# resource.gd
extends Resource

const preloaded_member = preload("...") # will be loaded when you `const MyResource = preload("resource.gd")`
var loaded_member = load("...") # will be loaded when you `var instance := MyResource.new()`

but postponing the loading to instantiation time is like an edge case I think, if ever done at all willingly.

To sum it up:

• I do not think that using load is not common enough to warrant an edge case in this feature, it can be changed on the rare cases where you need it,
• likewise for const vs var. I though of the additional Shift as a way to cover all cases, but honestly I think simple is better (unless people start complaining and proving me wrong). Manually changing const to var when needed (+ manually adding the resource type) should be enough imo.

[JoNax97]

Ok, I switched to const and decided to keep it simple for now. If feedback shows demand for optional var, load() or whatever, it can be done in a follow-up.

The PR is tested, rebased and ready to go now.

[JoNax97]

Applied reviews and simplified the changes significantly since there's no need to support type hints nor internal resources.

[YuriSizov]

Thanks!

[nubels]

Ok, I switched to const and decided to keep it simple for now. If feedback shows demand for optional var, load() or whatever, it can be done in a follow-up.

The PR is tested, rebased and ready to go now.

Awesome! Thank you very much for implementing this feature. :)