Ensure more export errors are reported to users #85845
Conversation
| @@ -161,6 +161,7 @@ Error EditorExportPlatformPC::prepare_template(const Ref<EditorExportPreset> &p_ | |||
| } | |||
| if (err != OK) { | |||
| add_message(EXPORT_MESSAGE_ERROR, TTR("Prepare Template"), TTR("Failed to copy export template.")); | |||
| return err; | |||
There was a problem hiding this comment.
Obviously, this is moot right now, but if we were to extend this function with more checks, this would ensure we aren't forgetting to return here.
| if (sdk_path.is_empty()) { | ||
| add_message(EXPORT_MESSAGE_ERROR, TTR("Export"), TTR("Android SDK path must be configured in Editor Settings at 'export/android/android_sdk_path'.")); | ||
| return ERR_UNCONFIGURED; | ||
| } |
There was a problem hiding this comment.
I'm not sure we needed to use error macros in such places to begin with, but adding proper log messages will call ERR_PRINT for us anyway, so they can be removed.
| if (err != OK) { | ||
| CLEANUP_AND_RETURN(err); | ||
| } |
There was a problem hiding this comment.
Doesn't seem to be necessary, we never reassign err since the last check above.
| if (err != OK) { | ||
| // Message is supplied by the subroutine method. | ||
| return err; | ||
| } |
There was a problem hiding this comment.
Don't see a point in trying to export ZIP if we failed here already. Don't do it on Linux either.
Also fixes the timing issue when exporting all presets at the same time, where the error report would try to appear while the progress dialog was still visible.
YuriSizov
force-pushed
the
editor-export-hidden-errors
branch
from
80f6588
to
773b4d7
Compare
| // TODO: Improve error reporting by using `add_message` throughout all methods called via `_export_ios_plugins`. | ||
| // For now a generic top level message would be fine, but we're ought to use proper reporting here instead of | ||
| // just fail macros and non-descriptive error return values. | ||
| add_message(EXPORT_MESSAGE_ERROR, TTR("iOS Plugins"), vformat(TTR("Failed to export iOS plugins with code %d. Please check the output log."), err)); |
There was a problem hiding this comment.
I gave up, there is just too much inside of the call above that can return an error. And this whole method itself needs to be refactored. It's giant, does a lot of things and is pretty messy. If someone approaches this problem, hopefully this TODO will let them know what to improve.
There was a problem hiding this comment.
All the error conditions in the _export_ios_plugins seems to be forwarding error from _copy_asset (which have two error returns), all of them probably should not be ERR_ macros (since it's just adding unnecessary and non-informative messages) and should be changed to if (err != OK) return err;.
| // TODO: Improve error reporting by using `add_message` throughout all methods called via `_export_ios_plugins`. | ||
| // For now a generic top level message would be fine, but we're ought to use proper reporting here instead of | ||
| // just fail macros and non-descriptive error return values. | ||
| add_message(EXPORT_MESSAGE_ERROR, TTR("iOS Plugins"), vformat(TTR("Failed to export iOS plugins with code %d. Please check the output log."), err)); |
There was a problem hiding this comment.
All the error conditions in the _export_ios_plugins seems to be forwarding error from _copy_asset (which have two error returns), all of them probably should not be ERR_ macros (since it's just adding unnecessary and non-informative messages) and should be changed to if (err != OK) return err;.
|
Thanks for reviews! |
So apparently we don't have a good habit of adding export status messages when errors happen. This can lead to the export process failing without any concrete errors in the export log. This PR tries to address it to the best of my ability. In case some messages are still underreported, I've added a fallback error message to tell the users something.
It also fixes the timing issue when exporting all presets at the same time, where the error report would try to appear while the progress dialog was still visible. This leads to reports like #85814 focusing their attention on an irrelevant message.
Closes #85814, although I don't know what specifically blocks the users. But now, hopefully, they'll get a proper error message. It's probably cherry-pickable, but some edited code is messy so I would like a good review and some time for it to be tested before we consider that.