Properly change global class paths on move or rename in the filesystem dock #85037
Conversation
Jordyfel
force-pushed
the
global-classes-on-move
branch
2 times, most recently
from
8221f98
to
21beeae
Compare
Jordyfel
force-pushed
the
global-classes-on-move
branch
from
21beeae
to
76cd31e
Compare
Jordyfel
force-pushed
the
global-classes-on-move
branch
from
76cd31e
to
ab83a01
Compare
Jordyfel
force-pushed
the
global-classes-on-move
branch
3 times, most recently
from
cbfb46b
to
3efdc10
Compare
Jordyfel
force-pushed
the
global-classes-on-move
branch
from
5a56597
to
447384f
Compare
| } | ||
|
|
||
| int index = -1; | ||
| EditorFileSystemDirectory *efd = EditorFileSystem::get_singleton()->find_file(pair.key, &index); |
There was a problem hiding this comment.
This is nearly 100% copied from EditorFileSystem._update_script_classes. I wonder if we can instead extract the necessary part of the method to a new method and call if from both locations?
The code in EditorFileSystem also handles the case when the file does not exist, which looks good to have in place as well.
There was a problem hiding this comment.
I did consider this, but the differences are enough that trying to extract it into a single function would be more annoying than useful. It's not like its a lot of code.
The "file" should be guaranteed to exist, because it is a EditorFileSystemDirectory fetched before a fs rescan is called, it uses data from before the move/rename operation. Can you think of a case where this would break?
There was a problem hiding this comment.
It makes the maintenance easier, as right now only EditorFileSystem._update_script_classes is responsible for updating script classes.
With your PR, there will be 2 places and if something needs to be fixed in the logic, one place may be forgotten.
Is there really a difference? I only see the order of ScriptServer::remove_global_class_by_path(path) and ClassDB::is_parent_class(type, "Script"), but that is also handled below at the language check. All other differences can be handled easily.
No, but for FS operations generally, it is always better to check and be safe as you can never be 100% sure.
Your code right now can fail or even crash if the index is not found and therefore -1.
|
I wonder why this happens in the first place?
|
|
Superseded by #90186 |
This fixes error spam about script files being looked for at the old location when moving lots of files together, as well as corruption of global class info which would require a project reload to fix.
MRP: test_proj.zip
Test by moving the resource together with it's script in the additional dir. Note the errors, and switch to the script tab. After the second move, the parser complains about problems with the global class.