Skip to content

Conversation

@jpobst
Copy link
Contributor

@jpobst jpobst commented Aug 28, 2020

In #689 we made generator localizable. One of the changes was this pattern:

- Report.Warning (0, Report.WarningReturnValue + 0, "Unknown return type {0} {1}.", java_type, context.ContextString);
+ Report.LogCodedWarning (0, Report.WarningUnknownReturnType, java_type, context.ContextString);

where:
Report.WarningUnexpectedFieldType = "Unknown return type '{0}' for member '{1}'."

However context.ContextString generates a hard to localize string like:

in method CreateFromXml in managed type Android.Content.Res.ColorStateList

resulting in warnings like this:

warning BG8800: Unknown return type 'System.Xml.XmlReader' for member 'in method CreateFromXml in managed type Android.Content.Res.ColorStateList'.

Instead, create a context.ContextTypeMember method that formats the member as just a type signature, which fits the localized message:

warning BG8800: Unknown return type 'System.Xml.XmlReader' for member 'Android.Content.Res.ColorStateList.CreateFromXml'.

@jpobst jpobst marked this pull request as ready for review August 28, 2020 17:08
string ContextMethodString => ContextMethod != null ? "in method " + ContextMethod.Name + " " : null;
string ContextTypeString => ContextType != null ? "in managed type " + ContextType.FullName : null;
public string ContextString => ContextFieldString + ContextMethodString + ContextTypeString;
public string ContextTypeMember => (ContextType != null ? $"{ContextType.FullName}." : string.Empty) + ContextMethod?.Name ?? ContextField?.Name;
Copy link
Contributor

Choose a reason for hiding this comment

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

When ContextMethod isn't null, could this also include the parameter list?

The example in the PR message, ColorStateList.CreateFromXml(), contains two overloads. It would presumably be helpful/useful if the warning message mentioned which overload was at fault:

warning BG8800: Unknown return type 'System.Xml.XmlReader' for member 'Android.Content.Res.ColorStateList.CreateFromXml(Android.Content.Res.Resources, System.Xml.XmlReader)'.

string ContextMethodString => ContextMethod != null ? "in method " + ContextMethod.Name + " " : null;
string ContextTypeString => ContextType != null ? "in managed type " + ContextType.FullName : null;
public string ContextString => ContextFieldString + ContextMethodString + ContextTypeString;
public string ContextTypeMember => (ContextType != null ? $"{ContextType.FullName}." : string.Empty) + ContextMethod?.Name ?? ContextField?.Name;
Copy link
Contributor

Choose a reason for hiding this comment

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

Considering that this method performs a string allocation, should it perhaps instead be a GetContextTypeMember() method?

@jonpryor
Copy link
Contributor

jonpryor commented Aug 28, 2020

Squash-and-merge message:

In d664e90e we made `generator` localizable.  One of the changes was
this pattern:

	- Report.Warning (0, Report.WarningReturnValue + 0, "Unknown return type {0} {1}.", java_type, context.ContextString);
	+ Report.LogCodedWarning (0, Report.WarningUnknownReturnType, java_type, context.ContextString);

in which `Report.WarningUnknownReturnType` is:

	"Unknown return type '{0}' for member '{1}'."

However `context.ContextString` generates a hard to localize string, e.g.

	in method CreateFromXml in managed type Android.Content.Res.ColorStateList

resulting in messages such as:

	warning BG8800: Unknown return type 'System.Xml.XmlReader' for member 'in method CreateFromXml in managed type Android.Content.Res.ColorStateList'.

This message doesn't "look right", nor is it itself localizable.

Instead, add a `context.GetContextTypeMember ()` method which formats the
member as a type signature, which doesn't need to be separately
localized and "reads correctly" in context:

	warning BG8800: Unknown return type 'System.Xml.XmlReader' for member 'Android.Content.Res.ColorStateList.CreateFromXml(Android.Content.Res.Resources, System.Xml.XmlReader)'.

Note that ^^ requires that ContextTypeMember actually include method parameter types. If that isn't done, the message should be updated accordingly.

var output = ContextType?.FullName ?? string.Empty;

if (ContextMethod != null) {
output += $"{ContextMethod.Name} ({string.Join (", ", ContextMethod?.Parameters.Select (p => p.RawType).ToArray ())})";
Copy link
Contributor

Choose a reason for hiding this comment

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

The p.RawType here confuses me.

For starers, I can't find it?

% git grep 'RawType' 
tests/Xamarin.Android.Tools.ApiXmlAdjuster-Tests/api-24.xml.in:        name="getRawType"

(Yet the PR builds, which makes this even more confusing to me.)

Secondly, do we want the "signature" to contain Java names or C# names? I'd think we'd want the Parameter.Type property?

https://github.com/xamarin/java.interop/blob/f6c12ba30048748572af57654b218c60bc9ff95f/tools/generator/Java.Interop.Tools.Generator.ObjectModel/Parameter.cs#L222-L226

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This point of the process is looking up Java types (type) and converting them to C# types (sym). So sym is still null, causing an NRE if we use Parameter.Type.

@jonpryor jonpryor merged commit e295b49 into master Aug 28, 2020
@jonpryor jonpryor deleted the localization-fix branch August 28, 2020 20:19
@jpobst jpobst added this to the 11.1 (16.9 / 8.9) milestone Sep 3, 2020
@github-actions github-actions bot locked and limited conversation to collaborators Apr 13, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants