Better error display in debugger panel #71943
Conversation
How about this? I haven't tested it though. diff --git a/modules/mono/glue/runtime_interop.cpp b/modules/mono/glue/runtime_interop.cpp
index f0ea0313ea..48cd3ebaee 100644
--- a/modules/mono/glue/runtime_interop.cpp
+++ b/modules/mono/glue/runtime_interop.cpp
@@ -31,6 +31,7 @@
#include "runtime_interop.h"
#include "core/config/engine.h"
+#include "core/config/project_settings.h"
#include "core/debugger/engine_debugger.h"
#include "core/debugger/script_debugger.h"
#include "core/io/marshalls.h"
@@ -45,6 +46,7 @@
#include "modules/mono/managed_callable.h"
#include "modules/mono/mono_gd/gd_mono_cache.h"
#include "modules/mono/signal_awaiter_utils.h"
+#include "modules/mono/utils/path_utils.h"
#ifdef __cplusplus
extern "C" {
@@ -84,7 +86,8 @@ void godotsharp_stack_info_vector_destroy(
void godotsharp_internal_script_debugger_send_error(const String *p_func,
const String *p_file, int32_t p_line, const String *p_err, const String *p_descr,
bool p_warning, const Vector<ScriptLanguage::StackInfo> *p_stack_info_vector) {
- EngineDebugger::get_script_debugger()->send_error(*p_func, *p_file, p_line, *p_err, *p_descr,
+ const String file = "res://" + path::relative_to(*p_file, ProjectSettings::get_singleton()->get_project_data_path());
+ EngineDebugger::get_script_debugger()->send_error(*p_func, file, p_line, *p_err, *p_descr,
true, p_warning ? ERR_HANDLER_WARNING : ERR_HANDLER_ERROR, *p_stack_info_vector);
}
|
Yep, it works to get rid of the C++ Source one. The C++ Error, though, is here because we re-use a field that's supposed to be populated only for C++ errors, if I understand correctly. Arguably, it only repeats the information we have on the line above. IDK, it feels nice to have, but it feels weird to have it mislabelled. |
paulloz
force-pushed
the
debugger/better-errors-printing
branch
from
5310d2c to
30c9eb8
Compare
| if (!text.ends_with(")")) | ||
| text += "()"; |
There was a problem hiding this comment.
We mandate braces even around single line if statements in C++ (not in C# I think).
paulloz
force-pushed
the
debugger/better-errors-printing
branch
from
2628c82 to
866700a
Compare
|
Squashed. And sorry, I made additional changes in |
| if (!oe.error_descr.is_empty()) { | ||
| // Add item for C++ error condition. | ||
| TreeItem *cpp_cond = error_tree->create_item(error); | ||
| cpp_cond->set_text(0, "<" + TTR("C++ Error") + ">"); | ||
| cpp_cond->set_text(0, "<" + source_language_name + " " + TTR("Error") + ">"); |
There was a problem hiding this comment.
This breaks localization, in French it would become "<C++ Erreur>" when it should be "<Erreur C++>" or similar.
You need to use the %s substitution to let translators move it around - and likely add a translator comment to clarify the meaning.
Like:
# TRANSLATORS: %s is the name of a language, e.g. C++.
cpp_cond->set_text(0, "<" + vformat(TTR("%s Error"), source_language_name) + ">");Same below.
| cpp_source->set_text(1, source_txt); | ||
| cpp_source->set_text_alignment(0, HORIZONTAL_ALIGNMENT_LEFT); | ||
| tooltip += (source_is_project_file ? TTR("Source:") : TTR("C++ Source:")) + " " + source_txt + "\n"; | ||
| tooltip += source_language_name + " " + TTR("Source") + " " + source_txt + "\n"; |
There was a problem hiding this comment.
You removed the colon here but not for "C++ Error", that seems inconsistent.
| if (!oe.error_descr.is_empty()) { | ||
| // Add item for C++ error condition. | ||
| TreeItem *cpp_cond = error_tree->create_item(error); | ||
| cpp_cond->set_text(0, "<" + TTR("C++ Error") + ">"); | ||
| cpp_cond->set_text(0, "<" + source_language_name + " " + TTR("Error") + ">"); |
There was a problem hiding this comment.
This might break the "Open C++ Source on GitHub" right click option. See #66977.
You should also see if this option is shown for C# and what happens when it's selected.
There was a problem hiding this comment.
Still works fine for C++ sources (nothing basically didn't change for them). For C# we used to open a 404 and now shows the toast error. It's very weird to open the right click menu on C# files anyway because you usually have an external editor set, and even the right click opens the file in the external editor. Might be a good thing to fix in a subsequent PR.
Well, that might have been better for another PR as the rest was ready but the new changes aren't ;) |
I shouldn't reply to PR late, now I feel bad wasting your time. I'll fix everything tomorrow. |
|
Don't worry, I was being facetious :) The changes seem fine overall, we can take the time needed to polish them. |
- Use the right stack frame info as title of the error. - Use the actual C# exception type as error for exceptions raised from C#. - Show the right language instead of always **C++ Error**.
paulloz
force-pushed
the
debugger/better-errors-printing
branch
from
866700a to
c93eec4
Compare
|
Thanks! |
|
Is this fix in beta17 ? |
|
@Flavelius Can you show one of those errors unrolled? It's still possible to have this kind of message if the error originates from the generated code and not "your" code itself. Not sure about how we could improve that. |
|
Some are, some are not: |
For starters, I found it very weird that we were using the least deep stack frame as the main error message in red (and only info when the stack is closed), so I used the right (IMHO) one instead.
I also got rid of the odd doubled parenthesis we had in C# errors, so I got rid of them. I imagine it's here for other languages that don't have them already baked in the error.
Last, for C# exceptions, I added the actual exception type where we simply had "Exception" hard coded before.
I'd like to change the C++ Error and C++ Source labels when we're not dealing with C++ errors, but I didn't find a way to do it that didn't feel too hacky. I'm open to suggestions if someone wants to push me in the right direction.
Before:
After: