[Android export] Added validation of the project name when using $genname in the 'Unique Name' field. #71646
Conversation
| //Check package name when used $genname in the "Unique Name" field | ||
| String basename = GLOBAL_GET("application/config/name"); | ||
| String name = get_valid_basename(); | ||
| if (valid && pn.find("$genname") >= 0 && name != basename) { | ||
| String p_name = p_preset->get("package/unique_name"); | ||
| String fn = get_package_name(p_name); //The final package name | ||
| err += vformat(TTR("Invalid package name. Will be replaced with \"%s\" when exporting."), fn); | ||
| err += "\n"; | ||
| } | ||
|
|
There was a problem hiding this comment.
There is a check above that already validates the package name, can this logic be added to it.
There was a problem hiding this comment.
@m4gr3d if I do the check above, then the variable valid may then change to the false. Then this warning will be displayed in red font, which will be wrong. If the export is not possible, then this warning is not necessary.
Here I need to delete line number 2394 and replace p_name with pn.
This is my first pull request and I don't now how to do it yet, because I messed up something with the branches. I will try to figure it out.
There was a problem hiding this comment.
I think the warning should be turned into an error, in which case it's fine to merge the added logic with is_package_name_valid.
The reason for being an error is that a warning that doesn't block the user from progressing can easily be overlooked leading to confusion when the package name doesn't match with the unique name.
I'd rather the export dialog to be explicit to the user that the selected name is invalid.
There was a problem hiding this comment.
As part of the error, we can suggest the user to manually overwrite the package name with a suitable replacement if they would like to keep the unique name.
Vilcrow
force-pushed
the
android-export-genname-check
branch
from
17071d7 to
f56555e
Compare
Vilcrow
changed the title
|
Fixed the commit history in this branch. It doesn't seem to have broken anything. |
| String basename = GLOBAL_GET("application/config/name"); | ||
| basename = basename.to_lower(); | ||
| if (basename != get_valid_basename()) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Looks good! Can you add a comment describing this logic. Otherwise it could be easy for a contributor to not realize that basename may not be equal to get_valid_basename() and mistakenly remove this method.
| return true; | ||
| } | ||
|
|
||
| bool EditorExportPlatformAndroid::is_project_name_valid(const String &p_package) const { |
There was a problem hiding this comment.
The p_package parameter doesn't seem to be needed.
| @@ -525,6 +530,22 @@ bool EditorExportPlatformAndroid::is_package_name_valid(const String &p_package, | |||
| return false; | |||
| } | |||
|
|
|||
| if (p_package.find("$genname") >= 0 && !is_project_name_valid(p_package)) { | |||
| if (r_error) { | |||
| *r_error = TTR("The end result may be unexpected. Try to explicitly specify the project name instead of the '$genname'."); | |||
There was a problem hiding this comment.
Can you update the error message to:
The project name does not meet the requirement for the package name format. Please explicitly specify the package name.
Vilcrow
force-pushed
the
android-export-genname-check
branch
from
f56555e to
9c7c1c5
Compare
|
@m4gr3d thanks. |
| //returns the project name without invalid characters | ||
| //or the "noname" string if all characters are invalid |
There was a problem hiding this comment.
Our comment style is full sentences (with capital letter and period), and space after //.
| //returns the project name without invalid characters | |
| //or the "noname" string if all characters are invalid | |
| // Returns the project name without invalid characters | |
| // or the "noname" string if all characters are invalid. |
Applies to the other comments below.
…name in the 'Unique Name' field.
Vilcrow
force-pushed
the
android-export-genname-check
branch
from
9c7c1c5 to
b8bc306
Compare
|
Thanks! And congrats for your first merged Godot contribution 🎉 |
Fixes #71645.