Documentation Image Loading + Caching W/ Relative and Absolute images #1773
Conversation
…as well. -DocumentationTextBlock image resolving now solves relative links, loads images, and caches them (Caching only on Release build). -More than just .md are included into the AppX from docs. -Fixed InitialContent.md using a white image on white background for example.
|
Important question, do we clear the Cache on version update? |
|
Any reason we don't just have a webview to the docs website? |
|
You mean instead of the MarkdownTextBlock? A WebView isn't native, takes up memory, doesn't adapt to theming, slow to load, steals input, might have issues with touch, etc. |
|
Yes, instead of the MarkdownTextBlock. The documentation on the docs site looks better than markdown does. |
|
It also works offline this way too. However, this seems to also package the documentation images with the app, effectively increasing the size of the package by quite a bit. I would rather the app retrieve the images remotely and cache them in run time. |
|
Hmm, ok. I have an idea, I will include all files and debug, and .md only in release. That way we can still view the Images in debug, if we are altering the documentation in a PR. |
…nly collect the .md files. -Pragma disabled SA1009 for Value Tuple, because following it will cause SA1015.
|
@nmetulev Done. |
| { | ||
| try | ||
| { | ||
| using (var client = new HttpClient()) |
There was a problem hiding this comment.
Keep a single static instance of HttpClient rather than creating a new one each time
| private async void SaveImageToCache(string localpath, Stream imageStream) | ||
| { | ||
| // Close stream when finished. | ||
| using (imageStream) |
There was a problem hiding this comment.
Move the using outside of the method. Methods usually will not dispose of objects passed to them
There was a problem hiding this comment.
It is inside the method, because we want to save the file asynchronously without interrupting the UI thread, so it takes a second copy of the image stream and saves it in this void method, this way it can display the image before the file is saved to improve performance.
| { | ||
| if (response.IsSuccessStatusCode) | ||
| { | ||
| var imageCopy = await CopyStream(response.Content); |
There was a problem hiding this comment.
Make a note that we cannot dispose of imageCopy because we need it for the image source
There was a problem hiding this comment.
It is not disposed of, that is why the stream is copied.
| @@ -147,22 +147,26 @@ public bool IsSupported | |||
| } | |||
| } | |||
|
|
|||
| public async Task<string> GetDocumentationAsync() | |||
| #pragma warning disable SA1009 // Doesn't like ValueTuples. | |||
| public async Task<(string contents, string Path)> GetDocumentationAsync() | |||
There was a problem hiding this comment.
Both return variables should be cased the same. Either capitalize both, or lowercase both
… it can also share the HttpClient with the rest of the Sample. -Fixed casing of return variable for GetDocumentationAsync. -Use dispose instead of using for SaveImageToCache.
|
@skendrot Changes have been made. |
| var deferral = e.GetDeferral(); | ||
| BitmapImage image = null; | ||
|
|
||
| var absolute = Uri.TryCreate(e.Url, UriKind.Absolute, out Uri Url); |
There was a problem hiding this comment.
lower case the Url variable
| var deferral = e.GetDeferral(); | ||
| BitmapImage image = null; | ||
|
|
||
| var absolute = Uri.TryCreate(e.Url, UriKind.Absolute, out Uri Url); |
There was a problem hiding this comment.
If absolute is not going to be used again, just put the TryCreate in the if
if(Uri.TryCreate(e.Url, UriKind.Absolute, out Uri url) == false)
| return imageStream; | ||
| } | ||
|
|
||
| private async void SaveImageToCache(string localpath, Stream imageStream) |
There was a problem hiding this comment.
async void methods should be avoided. I understand we want to get the image back to the user as fast as possible, but this only happens once. Every time after it should grab from the local file system.
Either
- Make this a Task returning method and await it
- Change
CopyToAsynctoCopyTo. I rarely notice a performance hit when coping from one stream to the next.
-Made Url lowercase. -Simplified relative check.
|
@skendrot Changes have been made again. |
|
ping @skendrot |
|
Looks like there are conflicts now |
|
Fixed conflicts. |
-GetDocumentationPathAsync now returns the Path of the documentation as well.
-DocumentationTextBlock image resolving now solves relative links, loads images, and caches them (Caching only on Release build).
-More than just .md are included into the AppX from docs.
-Fixed InitialContent.md using a white image on white background for example.
PR Type
What kind of change does this PR introduce?
What is the current behavior?
What is the new behavior?
PR Checklist
Please check if your PR fulfills the following requirements:
Does this PR introduce a breaking change?