Skip to content

[LiveLogger ] Localize strings #8682

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Apr 25, 2023
Merged

[LiveLogger ] Localize strings #8682

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9cb6176
Localize LiveLogger strings.
rokonec Apr 20, 2023
58727f5
Minor syntax changes - by review comments.
rokonec Apr 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/MSBuild.UnitTests/MockTerminal.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,8 @@ public void EndUpdate()
public void Write(ReadOnlySpan<char> text) { AddOutput(text.ToString()); }
public void WriteColor(TerminalColor color, string text) => AddOutput(text);
public void WriteColorLine(TerminalColor color, string text) { AddOutput(text); AddOutput("\n"); }
public string RenderColor(TerminalColor color, string text) => text;

public void WriteLine(string text) { AddOutput(text); AddOutput("\n"); }
public void WriteLineFitToWidth(ReadOnlySpan<char> text)
{
Expand Down
5 changes: 5 additions & 0 deletions src/MSBuild/LiveLogger/ITerminal.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,11 @@ internal interface ITerminal : IDisposable
/// Writes a string to the output using the given color. Or buffers it if <see cref="BeginUpdate"/> was called.
/// </summary>
void WriteColorLine(TerminalColor color, string text);

/// <summary>
/// Return string representing text wrapped in VT100 color codes.
/// </summary>
string RenderColor(TerminalColor color, string text);
}

/// <summary>
Expand Down
95 changes: 56 additions & 39 deletions src/MSBuild/LiveLogger/LiveLogger.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,9 +42,20 @@ internal record NodeStatus(string Project, string? TargetFramework, string Targe
{
public override string ToString()
{
string duration = Stopwatch.Elapsed.TotalSeconds.ToString("F1");
Copy link
Contributor

@Forgind Forgind Apr 20, 2023

Choose a reason for hiding this comment

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

nit:
We're currently displaying hundredths of a second in the not-live logger, but it seems like we switched to tenths of a second for the live logger. Was that an intentional choice?

Copy link
Member Author

@rokonec rokonec Apr 21, 2023

Choose a reason for hiding this comment

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

It is consistent in new LiveLogger, so I believe it is intentional. IMO, from practical point, tenths are just enough.

Copy link
Member

@ladipro ladipro Apr 24, 2023

Choose a reason for hiding this comment

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

Is the double -> string formatting locale-aware if done in code like this? My suggestion is to omit the ToString call here and change the placeholder in Strings.resx to something like {0:F1}.

Copy link
Member Author

@rokonec rokonec Apr 24, 2023

Choose a reason for hiding this comment

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

Yes double.ToString does respect Thread.CurrentThread.CurrentCulture - I have also tested it.
Have though about if formatting rules belongs to resx. In this case I proffered it in application logic as I viewed presented decimal point precision as UX design responsibility which do not, IMO, belongs to translation team.
If we choose to change it in future, such change would not trigger need for translation.

Copy link
Member

@ladipro ladipro Apr 24, 2023

Choose a reason for hiding this comment

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

Thank you. I guess I could argue either way - for the app logic to own the formatting so it's easier to change in the future and for the resource files to contain it so it can be localized. FWIW, we already have a precedence where the format is specified in .resx and it's translated to the same format in all languages. So it probably doesn't matter.

msbuild/src/Build/Resources/Strings.resx

Line 1481 in c6e6cd4

<value>{0}{1,-3}{2}: {3:0.000}s {4:0.000}s {5} ({6}) </value>


return string.IsNullOrEmpty(TargetFramework)
? $"{Indentation}{Project} {Target} ({Stopwatch.Elapsed.TotalSeconds:F1}s)"
: $"{Indentation}{Project} [{TargetFramework}] {Target} ({Stopwatch.Elapsed.TotalSeconds:F1}s)";
? ResourceUtilities.FormatResourceStringIgnoreCodeAndKeyword("ProjectBuilding_NoTF",
Copy link
Contributor

@Forgind Forgind Apr 20, 2023

Choose a reason for hiding this comment

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

I was thinking about ways we could combine these into one ResourceUtilities.FormatResourceStringIgnoreCodeAndKeyword call...I'm tempted to propose rearranging the arguments to put TF at the end, so we can always pass it even if it's null or empty but not necessarily display it or even having a custom part in the middle of the string, but I'm not convinced either change is actually better.

Copy link
Member Author

@rokonec rokonec Apr 21, 2023

Choose a reason for hiding this comment

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

I though about that. I even have it coded as subpart but the code, IMO, was harder to read and translation was more confusing. It can be changed easily though, if we decide to do so.

Indentation,
Project,
Target,
duration)
: ResourceUtilities.FormatResourceStringIgnoreCodeAndKeyword("ProjectBuilding_WithTF",
Indentation,
Project,
TargetFramework,
Target,
duration);
}
}

Expand Down Expand Up @@ -217,14 +228,12 @@ private void BuildFinished(object sender, BuildFinishedEventArgs e)
Terminal.BeginUpdate();
try
{
double duration = (e.Timestamp - _buildStartTime).TotalSeconds;

Terminal.WriteLine("");
Terminal.Write("Build ");

PrintBuildResult(e.Succeeded, _buildHasErrors, _buildHasWarnings);

double duration = (e.Timestamp - _buildStartTime).TotalSeconds;
Terminal.WriteLine($" in {duration:F1}s");
Terminal.WriteLine(ResourceUtilities.FormatResourceStringIgnoreCodeAndKeyword("BuildFinished",
RenderBuildResult(e.Succeeded, _buildHasErrors, _buildHasWarnings),
duration.ToString("F1")));
}
finally
{
Expand Down Expand Up @@ -301,7 +310,8 @@ private void ProjectFinished(object sender, ProjectFinishedEventArgs e)
try
{
EraseNodes();
Terminal.WriteLine($"Restore complete ({duration:F1}s)");
Terminal.WriteLine(ResourceUtilities.FormatResourceStringIgnoreCodeAndKeyword("RestoreComplete",
duration.ToString("F1")));
DisplayNodes();
}
finally
Expand All @@ -322,30 +332,36 @@ private void ProjectFinished(object sender, ProjectFinishedEventArgs e)
{
EraseNodes();

double duration = project.Stopwatch.Elapsed.TotalSeconds;
string duration = project.Stopwatch.Elapsed.TotalSeconds.ToString("F1");
ReadOnlyMemory<char>? outputPath = project.OutputPath;

Terminal.Write(Indentation);

if (e.ProjectFile is not null)
{
ReadOnlySpan<char> projectFile = Path.GetFileNameWithoutExtension(e.ProjectFile.AsSpan());
Terminal.Write(projectFile);
Terminal.Write(" ");
}
if (!string.IsNullOrEmpty(project.TargetFramework))
{
Terminal.Write($"[{project.TargetFramework}] ");
}
string projectFile = e.ProjectFile is not null ?
Path.GetFileNameWithoutExtension(e.ProjectFile) :
string.Empty;

// Print 'failed', 'succeeded with warnings', or 'succeeded' depending on the build result and diagnostic messages
// Build result. One of 'failed', 'succeeded with warnings', or 'succeeded' depending on the build result and diagnostic messages
// reported during build.
bool haveErrors = project.BuildMessages?.Exists(m => m.Severity == MessageSeverity.Error) == true;
bool haveWarnings = project.BuildMessages?.Exists(m => m.Severity == MessageSeverity.Warning) == true;
PrintBuildResult(e.Succeeded, haveErrors, haveWarnings);
string buildResult = RenderBuildResult(e.Succeeded, haveErrors, haveWarnings);

_buildHasErrors |= haveErrors;
_buildHasWarnings |= haveWarnings;
if (string.IsNullOrEmpty(project.TargetFramework))
{
Terminal.Write(ResourceUtilities.FormatResourceStringIgnoreCodeAndKeyword("ProjectFinished_NoTF",
Indentation,
projectFile,
buildResult,
duration));
}
else
{
Terminal.Write(ResourceUtilities.FormatResourceStringIgnoreCodeAndKeyword("ProjectFinished_WithTF",
Indentation,
projectFile,
project.TargetFramework,
buildResult,
duration));
}

// Print the output path as a link if we have it.
if (outputPath is not null)
Expand All @@ -360,15 +376,13 @@ private void ProjectFinished(object sender, ProjectFinishedEventArgs e)
{
// Ignore any GetDirectoryName exceptions.
}
#if NETCOREAPP
Terminal.WriteLine($" ({duration:F1}s) → {AnsiCodes.LinkPrefix}{url}{AnsiCodes.LinkInfix}{outputPath}{AnsiCodes.LinkSuffix}");
#else
Terminal.WriteLine($" ({duration:F1}s) → {AnsiCodes.LinkPrefix}{url.ToString()}{AnsiCodes.LinkInfix}{outputPath.ToString()}{AnsiCodes.LinkSuffix}");
#endif

Terminal.WriteLine(ResourceUtilities.FormatResourceStringIgnoreCodeAndKeyword("ProjectFinished_OutputPath",
$"{AnsiCodes.LinkPrefix}{url.ToString()}{AnsiCodes.LinkInfix}{outputPath}{AnsiCodes.LinkSuffix}"));
}
else
{
Terminal.WriteLine($" ({duration:F1}s)");
Terminal.WriteLine(string.Empty);
}

// Print diagnostic output under the Project -> Output line.
Expand All @@ -386,6 +400,9 @@ private void ProjectFinished(object sender, ProjectFinishedEventArgs e)
}
}

_buildHasErrors |= haveErrors;
_buildHasWarnings |= haveWarnings;

DisplayNodes();
}
finally
Expand Down Expand Up @@ -708,26 +725,26 @@ public void Clear()
/// <param name="succeeded">True if the build completed with success.</param>
/// <param name="hasError">True if the build has logged at least one error.</param>
/// <param name="hasWarning">True if the build has logged at least one warning.</param>
private void PrintBuildResult(bool succeeded, bool hasError, bool hasWarning)
private string RenderBuildResult(bool succeeded, bool hasError, bool hasWarning)
{
if (!succeeded)
{
// If the build failed, we print one of three red strings.
string text = (hasError, hasWarning) switch
{
(true, _) => "failed with errors",
(false, true) => "failed with warnings",
_ => "failed",
(true, _) => ResourceUtilities.GetResourceString("BuildResult_FailedWithError"),
(false, true) => ResourceUtilities.GetResourceString("BuildResult_FailedWithWarn"),
_ => ResourceUtilities.GetResourceString("BuildResult_Failed"),
};
Terminal.WriteColor(TerminalColor.Red, text);
return Terminal.RenderColor(TerminalColor.Red, text);
}
else if (hasWarning)
{
Terminal.WriteColor(TerminalColor.Yellow, "succeeded with warnings");
return Terminal.RenderColor(TerminalColor.Yellow, ResourceUtilities.GetResourceString("BuildResult_SucceededWithWarn"));
}
else
{
Terminal.WriteColor(TerminalColor.Green, "succeeded");
return Terminal.RenderColor(TerminalColor.Green, ResourceUtilities.GetResourceString("BuildResult_Succeeded"));
}
}

Expand Down
8 changes: 7 additions & 1 deletion src/MSBuild/LiveLogger/Terminal.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,10 +132,16 @@ public void WriteColor(TerminalColor color, string text)
}
else
{
Write($"{AnsiCodes.CSI}{(int)color}{AnsiCodes.SetColor}{text}{AnsiCodes.SetDefaultColor}");
Write(RenderColor(color, text));
}
}

/// <inheritdoc/>
public string RenderColor(TerminalColor color, string text)
{
return $"{AnsiCodes.CSI}{(int)color}{AnsiCodes.SetColor}{text}{AnsiCodes.SetDefaultColor}";
}

/// <inheritdoc/>
public void WriteColorLine(TerminalColor color, string text)
{
Expand Down
96 changes: 96 additions & 0 deletions src/MSBuild/Resources/Strings.resx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1366,6 +1366,102 @@
<data name="UnsupportedSwitchForSolutionFiles" Visibility="Public">
<value>The '{0}' switch is not supported for solution files.</value>
</data>
<!-- **** LiveLogger strings begin **** -->
<data name="RestoreComplete" xml:space="preserve">
<value>Restore complete ({0}s)</value>
<comment>
Duration in seconds with 1 decimal point is: {0}"
</comment>
</data>
<data name="BuildFinished" xml:space="preserve">
<value>Build {0} in {1}s</value>
<comment>
Overall build summary
{0}: BuildResult_X bellow is
{1}: duration in seconds with 1 decimal point
</comment>
</data>
<data name="BuildResult_FailedWithError" xml:space="preserve">
<value>failed with errors</value>
<comment>
Part of Live Logger summary message: "Build {BuildResult_X} in {duration}s"
</comment>
</data>
<data name="BuildResult_FailedWithWarn" xml:space="preserve">
<value>failed with warnings</value>
<comment>
Part of Live Logger summary message: "Build {BuildResult_X} in {duration}s"
</comment>
</data>
<data name="BuildResult_Failed" xml:space="preserve">
<value>failed</value>
<comment>
Part of Live Logger summary message: "Build {BuildResult_X} in {duration}s"
</comment>
</data>
<data name="BuildResult_Succeeded" xml:space="preserve">
<value>succeeded</value>
<comment>
Part of Live Logger summary message: "Build {BuildResult_X} in {duration}s"
</comment>
</data>
<data name="BuildResult_SucceededWithWarn" xml:space="preserve">
<value>succeeded with warnings</value>
<comment>
Part of Live Logger summary message: "Build {BuildResult_X} in {duration}s"
</comment>
</data>
<data name="ProjectFinished_NoTF" xml:space="preserve">
<value>{0}{1} {2} ({3}s)</value>
Copy link
Contributor

@Forgind Forgind Apr 20, 2023

Choose a reason for hiding this comment

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

Suggested change
<value>{0}{1} {2} ({3}s)</value>
<value>{0}Building {1} {2} ({3}s)</value>

? (If so, then also below)

Copy link
Member Author

@rokonec rokonec Apr 21, 2023

Choose a reason for hiding this comment

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

Intent of this PR was to make Live Logger localizable while keeping it exactly like it was.
If changes in strings are needed I'd rather do it in separate PR.

<comment>
Project finished summary.
{0}: indentation - few spaces to visually indent row
{1}: project name
{2}: BuildResult_{X}
{3}: duration in seconds with 1 decimal point
</comment>
</data>
<data name="ProjectFinished_WithTF" xml:space="preserve">
<value>{0}{1} [2] {3} ({4}s)</value>
<comment>
Project finished summary including target framework information.
{0}: indentation - few spaces to visually indent row
{1}: project name
{2}: target framework
{3}: BuildResult_{X}
{4}: duration in seconds with 1 decimal point
</comment>
</data>
<data name="ProjectFinished_OutputPath" xml:space="preserve">
<value> → {0}</value>
<comment>
Info about project output - when known. Printed after ProjectFinished_NoTF or ProjectFinished_WithTF.
{0}: VT100 coded hyperlink to project output directory
</comment>
</data>
<data name="ProjectBuilding_NoTF" xml:space="preserve">
<value>{0}{1} {2} ({3}s)</value>
<comment>
Project finished summary.
{0}: indentation - few spaces to visually indent row
{1}: project name
{2}: target
{3}: duration in seconds with 1 decimal point
</comment>
</data>
<data name="ProjectBuilding_WithTF" xml:space="preserve">
<value>{0}{1} [2] {3} ({4}s)</value>
<comment>
Project finished summary including target framework information.
{0}: indentation - few spaces to visually indent row
{1}: project name
{2}: target framework
{3}: target
{4}: duration in seconds with 1 decimal point
</comment>
</data>
Comment on lines +1414 to +1462
Copy link
Member

@ladipro ladipro Apr 24, 2023

Choose a reason for hiding this comment

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

Do you think there's a chance non-English versions of these strings will look differently other than the seconds unit abbreviation? I wonder if we can have only one localized string {0}s and use it for all these.

Copy link
Member Author

@rokonec rokonec Apr 24, 2023

Choose a reason for hiding this comment

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

Do you think there's a chance non-English versions of these strings will look differently

I see it not likely.

However, I am not expert in localization. It could be that theoretically some languages might not use spaces, other languages might despise brackets, some languages prefer different order of data presenting so they can prefer target before of project like target of project.

I'd rather play safe here...

Copy link
Member

@ladipro ladipro Apr 24, 2023

Choose a reason for hiding this comment

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

The changes you're describing don't look like something that would be done as part of translation. Maybe there are RTL considerations, not sure. Not blocking but I'd like to ask our loc champion. @richaverma1 can you please provide guidance?

Copy link
Member Author

@rokonec rokonec Apr 24, 2023

Choose a reason for hiding this comment

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

I have checked in past and none supported languages in MSBuild is, AFAIK, RTL.

<!-- **** LiveLogger strings end **** -->

<!--
The command line message bucket is: MSB1001 - MSB1999
Expand Down
Loading