Add local font checking and loading functionality for improved font m…#59
Add local font checking and loading functionality for improved font m…#59shps951023 merged 1 commit intomainfrom
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the application's font management by introducing the ability to leverage locally installed fonts. This change aims to provide a smoother and faster experience for users, particularly those requiring CJK fonts, by reducing network requests and improving rendering efficiency. The system now intelligently checks for local font availability before resorting to downloading, ensuring optimal font delivery. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds a great enhancement by attempting to use local fonts before downloading them, which can improve performance and user experience. The implementation is mostly solid, but I have a few suggestions to improve performance, maintainability, and robustness. In Converter.razor, I've suggested refactoring the font loading logic into a separate method to improve readability, and to add logging for exceptions that are currently being silently ignored. In download.js, I've proposed an optimization for the font-finding algorithm to make it more performant, especially for users with many fonts installed.
| try | ||
| { | ||
| var request = new HttpRequestMessage(HttpMethod.Get, "fonts/NotoSansSC-Regular.ttf"); | ||
| request.SetBrowserResponseStreamingEnabled(true); | ||
| using var response = await Http.SendAsync(request, HttpCompletionOption.ResponseHeadersRead); | ||
| response.EnsureSuccessStatusCode(); | ||
|
|
||
| var totalBytes = response.Content.Headers.ContentLength ?? -1; | ||
| using var contentStream = await response.Content.ReadAsStreamAsync(); | ||
| using var fontMs = new MemoryStream(); | ||
| var buffer = new byte[81920]; | ||
| long totalRead = 0; | ||
| int read; | ||
|
|
||
| while ((read = await contentStream.ReadAsync(buffer)) > 0) | ||
| // Try Local Font Access API first (Chrome 103+, requires user permission) | ||
| string? localFontFamily = null; | ||
| try { localFontFamily = await JS.InvokeAsync<string?>("tryLoadLocalFontMeta"); } catch { } | ||
|
|
||
| if (localFontFamily is not null) | ||
| { | ||
| fontMs.Write(buffer, 0, read); | ||
| totalRead += read; | ||
| if (totalBytes > 0) | ||
| var jsStreamRef = await JS.InvokeAsync<IJSStreamReference?>("getLocalFontStream"); | ||
| if (jsStreamRef is not null) | ||
| { | ||
| fontProgress = (int)(totalRead * 100 / totalBytes); | ||
| fontStatus = string.Format(L.T("FontDownloadProgress"), fontProgress, FormatSize(totalRead), FormatSize(totalBytes)); | ||
| await using var fontStream = await jsStreamRef.OpenReadStreamAsync(maxAllowedSize: 50 * 1024 * 1024); | ||
| using var fontMs = new MemoryStream(); | ||
| await fontStream.CopyToAsync(fontMs); | ||
| MiniSoftware.MiniPdf.RegisterFont(localFontFamily, fontMs.ToArray()); | ||
| fontLoadSuccess = true; | ||
| fontStatus = "✓ " + string.Format(L.T("LocalFontUsed"), localFontFamily); | ||
| } | ||
| else | ||
| { | ||
| fontStatus = $"{L.T("LoadingFont")} ({FormatSize(totalRead)})"; | ||
| localFontFamily = null; // stream unexpectedly null, fall through to download | ||
| } | ||
| StateHasChanged(); | ||
| } | ||
|
|
||
| MiniSoftware.MiniPdf.RegisterFont("NotoSansSC", fontMs.ToArray()); | ||
| fontLoadSuccess = true; | ||
| fontStatus = "✓ " + L.T("FontLoaded"); | ||
| if (localFontFamily is null) | ||
| { | ||
| // Fall back to downloading NotoSansSC | ||
| fontStatus = L.T("LoadingFont"); | ||
| StateHasChanged(); | ||
|
|
||
| var request = new HttpRequestMessage(HttpMethod.Get, "fonts/NotoSansSC-Regular.ttf"); | ||
| request.SetBrowserResponseStreamingEnabled(true); | ||
| using var response = await Http.SendAsync(request, HttpCompletionOption.ResponseHeadersRead); | ||
| response.EnsureSuccessStatusCode(); | ||
|
|
||
| var totalBytes = response.Content.Headers.ContentLength ?? -1; | ||
| using var contentStream = await response.Content.ReadAsStreamAsync(); | ||
| using var fontMs = new MemoryStream(); | ||
| var buffer = new byte[81920]; | ||
| long totalRead = 0; | ||
| int read; | ||
|
|
||
| while ((read = await contentStream.ReadAsync(buffer)) > 0) | ||
| { | ||
| fontMs.Write(buffer, 0, read); | ||
| totalRead += read; | ||
| if (totalBytes > 0) | ||
| { | ||
| fontProgress = (int)(totalRead * 100 / totalBytes); | ||
| fontStatus = string.Format(L.T("FontDownloadProgress"), fontProgress, FormatSize(totalRead), FormatSize(totalBytes)); | ||
| } | ||
| else | ||
| { | ||
| fontStatus = $"{L.T("LoadingFont")} ({FormatSize(totalRead)})"; | ||
| } | ||
| StateHasChanged(); | ||
| } | ||
|
|
||
| MiniSoftware.MiniPdf.RegisterFont("NotoSansSC", fontMs.ToArray()); | ||
| fontLoadSuccess = true; | ||
| fontStatus = "✓ " + L.T("FontLoaded"); | ||
| } | ||
| } |
There was a problem hiding this comment.
The font loading logic within this try-catch block has grown quite large and complex. To improve the readability and maintainability of the ConvertFile method, consider extracting this font loading logic into a new private async method, for example EnsureFontLoadedAsync(). The ConvertFile method would then just need to await this new method.
| while ((read = await contentStream.ReadAsync(buffer)) > 0) | ||
| // Try Local Font Access API first (Chrome 103+, requires user permission) | ||
| string? localFontFamily = null; | ||
| try { localFontFamily = await JS.InvokeAsync<string?>("tryLoadLocalFontMeta"); } catch { } |
There was a problem hiding this comment.
Swallowing exceptions with an empty catch block can hide bugs and make debugging difficult. If the JavaScript interop call fails for an unexpected reason, it will fail silently. It's better to log the exception to the developer console to aid in debugging, while still allowing the code to fall back to the font download behavior.
try
{
localFontFamily = await JS.InvokeAsync<string?>("tryLoadLocalFontMeta");
}
catch (Exception ex)
{
// This is expected to fail if the API is not supported or permission is denied.
// Log for debugging and fall back to downloading.
Console.WriteLine($"Could not query local fonts: {ex.Message}");
}
| let found = null; | ||
| for (const family of preferred) { | ||
| found = fonts.find(f => f.family === family && (f.style === 'Regular' || f.style === 'Normal')); | ||
| if (!found) found = fonts.find(f => f.family === family); | ||
| if (found) break; | ||
| } |
There was a problem hiding this comment.
The current implementation for finding a preferred font can be inefficient. It iterates over the entire fonts array up to twice for each font family in the preferred list. For users with many fonts, this could be slow. A more performant approach is to first group the available fonts by family name and then iterate through the preferred list. This reduces the algorithmic complexity and will perform better.
const familyMap = new Map();
for (const font of fonts) {
if (!familyMap.has(font.family)) {
familyMap.set(font.family, []);
}
familyMap.get(font.family).push(font);
}
let found = null;
for (const family of preferred) {
const familyFonts = familyMap.get(family);
if (familyFonts) {
found = familyFonts.find(f => f.style === 'Regular' || f.style === 'Normal') || familyFonts[0];
if (found) {
break;
}
}
}There was a problem hiding this comment.
Pull request overview
Adds client-side local font detection/loading to reduce the need to download bundled CJK fonts during conversions in the Blazor WebAssembly app.
Changes:
- Introduces Local Font Access API probing in
download.jsand exposes JS helpers to return a cached font stream. - Updates the converter page to prefer a locally available CJK font (when permitted) and fall back to downloading
NotoSansSC-Regular.ttf. - Adds new i18n strings for “checking local fonts” and “using local font” across supported languages.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| MiniPdf.Web/MiniPdf.Web.Client/wwwroot/js/download.js | Adds Local Font Access API helpers to locate a preferred local CJK font and provide a stream to .NET. |
| MiniPdf.Web/MiniPdf.Web.Client/Pages/Converter.razor | Uses JS helpers to load/register a local font first, otherwise downloads and registers the bundled Noto Sans SC font. |
| MiniPdf.Web/MiniPdf.Web.Client/I18n.cs | Adds new translation keys for local-font checking/usage status messages. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| await using var fontStream = await jsStreamRef.OpenReadStreamAsync(maxAllowedSize: 50 * 1024 * 1024); | ||
| using var fontMs = new MemoryStream(); | ||
| await fontStream.CopyToAsync(fontMs); | ||
| MiniSoftware.MiniPdf.RegisterFont(localFontFamily, fontMs.ToArray()); | ||
| fontLoadSuccess = true; |
…anagement