-
-
Notifications
You must be signed in to change notification settings - Fork 21.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
C#: MSBuild logs and panel enhancements #72061
C#: MSBuild logs and panel enhancements #72061
Conversation
modules/mono/editor/GodotTools/GodotTools/Build/MSBuildPanel.cs
Outdated
Show resolved
Hide resolved
public override void _DisablePlugin() | ||
{ | ||
base._DisablePlugin(); | ||
|
||
_editorSettings.SettingsChanged -= OnSettingsChanged; | ||
} | ||
|
||
private void OnSettingsChanged() | ||
{ | ||
// We want to force NoConsoleLogging to true when the VerbosityLevel is at Detailed or above. | ||
// At that point, there's so much info logged that it doesn't make sense to display it in | ||
// the tiny editor window, and it'd make the editor hang or crash anyway. | ||
var verbosityLevel = (VerbosityLevelId)(int)_editorSettings.GetSetting(Settings.VerbosityLevel); | ||
var hideConsoleLog = (bool)_editorSettings.GetSetting(Settings.NoConsoleLogging); | ||
if (verbosityLevel >= VerbosityLevelId.Detailed && !hideConsoleLog) | ||
_editorSettings.SetSetting(Settings.NoConsoleLogging, Variant.From(true)); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make more sense to handle this in BuildSystem?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean forcing -noconlog
regardless of Settings.NoConsoleLogging
if the verbosity is too high? I thought about it, but it felt better to show the user the checkbox getting automatically set to true. Makes it more "intentional"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's what I meant. I'm not sure what's better to be honest.
@@ -171,7 +179,7 @@ public void ShowErrorDialog(string message, string title = "Error") | |||
[UsedImplicitly] | |||
public Error OpenInExternalEditor(Script script, int line, int col) | |||
{ | |||
var editorId = (ExternalEditorId)(int)_editorSettings.GetSetting("mono/editor/external_editor"); | |||
var editorId = (ExternalEditorId)(int)_editorSettings.GetSetting(Settings.ExternalEditor); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Users coming from previous betas will lose their settings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do you think this should be handled?
Edit: To be clear, I agree it's not ideal. But on the other hand: it's a single setting, and it's probably better now than after the beta?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know if we have something in Godot to handle editor setting renames. If we don't, we may want to still look for the old name and, if found, set the new setting's value; at least for the betas/RCs.
cc @akien-mga
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless there's a built-in way to do this, I think it may be too much work to bother with it. Otherwise, you could check if the old setting is present, move its value to the new one, and remove the old one to avoid repeating this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you be more specific about what you mean by "not bother with it"? Are you implying we'd leave everything labelled "mono" or that we could afford to break settings compatibility between betas?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I meant not bother about migrating from the old setting. It may be annoying, but it's a one-time thing and there are other breaking changes coming as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Asked in RC:
@raulsntos: Do we have a way to migrate EditorSettings? For example, if an user has configured
settingA
in beta 5, and it was renamed tosettingB
in beta 10, they now have to reconfigure it.@akien-mga: Not currently, though I guess it could be handled in EditorSettings
_get
and_set
like other properties. But usually we don't try to preserve compat for editor settings.
So I think it's fine as is.
What is the use case for this?
Does this override our custom MSBuild logger? We rely on that one to show the list of errors and warnings after building. |
Being able to produce useful build logs (on disk) without flooding the console.
This doesn't override anything, the custom logger hasn't been touched. |
0ed87fa
to
5137c94
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
- toggle creation of binary logs - manage log verbosity - toggle logging in console
5137c94
to
c70c82b
Compare
Rewrote the commit history as discussed. Shouldn't change anything behaviourwise. |
Thanks! |
is it related? #64276 |
@DmitriySalnikov Do you mean "does it fix it?"? If so, not sure. All we're doing here is forcing the CLI language to the same locale as Godot. I didn't see any encoding issue with all the locales I had available to test, but didn't explicitly check ru. |
I'm continuing my quest about improving the C# experience.
mono
todotnet
, as it become more and more confusing for people to see the word mono in the interface.-v
flag.-noconlog
on or off, basically either print to the console and the logfile or to the logfile alone.-bl
on or off, binary logs being a very useful tool to debug MSBuild issues.