Skip to content

Conversation

ktxyz
Copy link
Contributor

@ktxyz ktxyz commented Sep 4, 2025

Possibly closes #110258.

Adding direct info about which name caused the error makes sense. Not right after the colon as suggested in the issue since there might be different reasons for Autoload fail. Adding this inside invalid name error seems like a good place, since other cases already include more information:

String error;
if (!_autoload_name_is_valid(name, &error)) {
  EditorNode::get_singleton()->show_warning(TTR("Can't add Autoload:") + "\n" + error);
  return false;
}

if (!FileAccess::exists(p_path)) {
  EditorNode::get_singleton()->show_warning(TTR("Can't add Autoload:") + "\n" + vformat(TTR("%s is an invalid path. File does not exist."), p_path));
  return false;
}

if (!p_path.begins_with("res://")) {
  EditorNode::get_singleton()->show_warning(TTR("Can't add Autoload:") + "\n" + vformat(TTR("%s is an invalid path. Not in resource path (res://)."), p_path));
  return false;
}

Before:
obraz
After:
obraz

@ktxyz ktxyz requested a review from a team as a code owner September 4, 2025 18:50
@ktxyz ktxyz force-pushed the add-name-to-invalid-name-error-output branch from 02a696c to 554d9e0 Compare September 4, 2025 21:36
@ktxyz
Copy link
Contributor Author

ktxyz commented Sep 4, 2025

I moved the name as prefix to the actual message so it doesn't impact translation in any way, unlike having it as Invalid Name: %s.

@Calinou Calinou added bug topic:editor cherrypick:4.5 Considered for cherry-picking into a future 4.5.x release labels Sep 4, 2025
@Calinou Calinou added this to the 4.6 milestone Sep 4, 2025
@Calinou
Copy link
Member

Calinou commented Sep 4, 2025

I would expect it to look like this instead:

Can't add Autoload: invalid name.
GlobalTetrahedron: Must not collide with an existing global script class name.

Notice the lowercase "i" for "invalid name", and the fact it's on the same line as the "header" of the error dialog.

@ktxyz
Copy link
Contributor Author

ktxyz commented Sep 4, 2025

I would expect it to look like this instead:

Can't add Autoload: invalid name.
GlobalTetrahedron: Must not collide with an existing global script class name.

Notice the lowercase "i" for "invalid name", and the fact it's on the same line as the "header" of the error dialog.

The "header" part only really exists for invalid name and not other options

if (!_autoload_name_is_valid(name, &error)) {
  EditorNode::get_singleton()->show_warning(TTR("Can't add Autoload:") + "\n" + error);
  return false;
}

if (!FileAccess::exists(p_path)) {
  EditorNode::get_singleton()->show_warning(TTR("Can't add Autoload:") + "\n" + vformat(TTR("%s is an invalid path. File does not exist."), p_path));
  return false;
}

if (!p_path.begins_with("res://")) {
  EditorNode::get_singleton()->show_warning(TTR("Can't add Autoload:") + "\n" + vformat(TTR("%s is an invalid path. Not in resource path (res://)."), p_path));
  return false;
}

So this would have us

Can't add Autoload: invalid name.
%s: Must not collide with an existing global script class name.
---
Can't add Autoload:
%s is an invalid path. File does not exist.
---
Can't add Autoload:
%s is an invalid path. Not in resource path (res://).

How about

Can't add Autoload:
%s is an invalid name. Must not collide with an existing global script class name.
---
Can't add Autoload:
%s is an invalid path. File does not exist.
---
Can't add Autoload:
%s is an invalid path. Not in resource path (res://).

This has message formats the same for each case and only requires '%s is an invalid name.' translation.

@ktxyz ktxyz force-pushed the add-name-to-invalid-name-error-output branch from 554d9e0 to d3fadcb Compare September 4, 2025 23:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug cherrypick:4.5 Considered for cherry-picking into a future 4.5.x release topic:editor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Addon autoload class collision error is missing class_name in the error
3 participants