-
-
Notifications
You must be signed in to change notification settings - Fork 455
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
Redo font previewer #1249
Redo font previewer #1249
Conversation
Thank you for the contribution. I can't review it right now, but I'll get to it as soon as I can. |
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.
Some small changes required, but otherwise good.
} | ||
} | ||
|
||
private static string TryDecode(IUnityObjectBase asset, out byte[] data, out string? fileName, out string? mimeType) |
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.
Methods with this design pattern should always have a boolean return type.
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.
And this part is similar to the code:
AssetRipper/Source/AssetRipper.GUI.Web/Pages/Assets/AudioTab.cs
Lines 39 to 46 in 6771232
private static string TryDecode(IUnityObjectBase asset) | |
{ | |
if (asset is IAudioClip clip && AudioClipDecoder.TryDecode(clip, out byte[]? decodedAudioData, out string? extension, out _)) | |
{ | |
return $"data:audio/{extension};base64,{Convert.ToBase64String(decodedAudioData, Base64FormattingOptions.None)}"; | |
} | |
return ""; | |
} |
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.
That code doesn't have any out parameters, which makes it different. In this case, we want a Boolean return. The current return can be made into another out parameter.
public override string DisplayName => Localization.AssetTabFont; | ||
|
||
public override string HtmlName => "font"; | ||
|
||
public override bool Enabled => Data.Length > 0; | ||
public override bool Enabled => !string.IsNullOrEmpty(Source); |
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.
Why?
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 just want it keep the same structure as
public override bool Enabled => !string.IsNullOrEmpty(Source); |
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.
My intuition says that the previous version is more future-proof. I foresee Source being removed in some way when this becomes asynchronous.
#932