Skip to content
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

Merged
merged 4 commits into from
Feb 7, 2023

Conversation

paulloz
Copy link
Member

@paulloz paulloz commented Jan 25, 2023

I'm continuing my quest about improving the C# experience.

  • Rename the current .NET related editor settings from mono to dotnet, as it become more and more confusing for people to see the word mono in the interface.
    image
  • Add a button on the right of the MSBuild panel to directly open the logs folder, which is currently quite hard to pinpoint (since it uses an MD5 hash in its path).
    image
  • Add a few settings, so the user is able to tune how MSBuild logs behave.
    image
    • Verbosity Level is tied to the -v flag.
    • No Console Logging switches -noconlog on or off, basically either print to the console and the logfile or to the logfile alone.
    • Create Binary Log switches -bl on or off, binary logs being a very useful tool to debug MSBuild issues.
  • Finally, as I suggested in Wrong Chinese encoding for MSBuild errors in the editor #71326, try to tie the language used by MSBuild with the language used by Godot. Won't solve that issue entirely, but is still a step in the right direction IMHO.
    en fr ja
    image image image

Comment on lines 535 to 551
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));
}
Copy link
Member

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?

Copy link
Member Author

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"?

Copy link
Member

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);
Copy link
Member

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.

Copy link
Member Author

@paulloz paulloz Jan 25, 2023

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?

Copy link
Member

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

Copy link
Contributor

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.

Copy link
Member Author

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?

Copy link
Contributor

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.

Copy link
Member

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 to settingB 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.

@neikeq
Copy link
Contributor

neikeq commented Jan 26, 2023

  • No Console Logging switches -noconlog on or off, basically either print to the console and the logfile or to the logfile alone.

What is the use case for this?

  • Create Binary Log switches -bl on or off, binary logs being a very useful tool to debug MSBuild issues.

Does this override our custom MSBuild logger? We rely on that one to show the list of errors and warnings after building.

@paulloz
Copy link
Member Author

paulloz commented Jan 26, 2023

What is the use case for this?

Being able to produce useful build logs (on disk) without flooding the console.

Does this override our custom MSBuild logger? We rely on that one to show the list of errors and warnings after building.

This doesn't override anything, the custom logger hasn't been touched.

@paulloz paulloz force-pushed the csharp/better-logs-management branch from 0ed87fa to 5137c94 Compare February 1, 2023 16:16
Copy link
Member

@raulsntos raulsntos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@YuriSizov YuriSizov requested a review from neikeq February 7, 2023 15:21
@paulloz paulloz force-pushed the csharp/better-logs-management branch from 5137c94 to c70c82b Compare February 7, 2023 15:58
@paulloz
Copy link
Member Author

paulloz commented Feb 7, 2023

Rewrote the commit history as discussed. Shouldn't change anything behaviourwise.

@akien-mga akien-mga merged commit 8be4fee into godotengine:master Feb 7, 2023
@akien-mga
Copy link
Member

Thanks!

@paulloz paulloz deleted the csharp/better-logs-management branch February 7, 2023 19:53
@DmitriySalnikov
Copy link
Contributor

is it related? #64276

@paulloz
Copy link
Member Author

paulloz commented Feb 8, 2023

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants