Skip to content
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

v1.3 #6

Merged
merged 49 commits into from
Nov 4, 2021
Merged

v1.3 #6

merged 49 commits into from
Nov 4, 2021

Conversation

MakeshVineeth
Copy link
Owner

  • Add Copy Contents operation.
  • Add Wikipedia logo as placeholder.
  • Code Improvements.
  • Fixed plugin is not respecting SearchTagOnly option.


namespace WikiPreview.Fluent.Plugin
{
public class CustomPreview : IResultPreviewControlBuilder
Copy link
Collaborator

Choose a reason for hiding this comment

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

Better to follow the interface name WikiResultPreviewControlBuilder


public bool CanBuildPreviewForResult(ISearchResult searchResult)
{
if (string.IsNullOrWhiteSpace(searchResult.SearchApp)) return false;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This condition is redundant since you do Equals below, if it was empty the Equals will return false

private ResultGenerator()
{
var assembly = Assembly.GetExecutingAssembly();
const string resourceName = "WikiPreview.Fluent.Plugin.Wikipedia-logo.png";
Copy link
Collaborator

Choose a reason for hiding this comment

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

No need to declare const if you only use it below

Copy link
Owner Author

Choose a reason for hiding this comment

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

For some reasons, IDE would keep on suggesting me to declare const. I'm directly giving the string literal now, let me know on this.


private readonly BitmapImageResult _bitmapLogo;

private ResultGenerator()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do you need to load the image every time this class is created? Make it a static property

Copy link
Owner Author

Choose a reason for hiding this comment

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

It was a Singleton before where only same instance will be provided. Now I've made it into static class as per your suggestions.

{
public sealed class ResultGenerator
{
private static readonly Lazy<ResultGenerator> LazySingleton =
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are you using lazy to not create the instance of the image below? Are there any cases you search with Wiki and not load it?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yes, it was mainly for loading the instance of the image and also store the image size preferences, also making it accessible across all classes. Earlier I thought lazy implementation can be more efficient. In the new code, I've achieved the same behavior that I wanted in a static way.

using var imageClient = new HttpClient();
imageClient.DefaultRequestHeaders.UserAgent.TryParseAdd(UserAgentString);
Stream stream = await imageClient.GetStreamAsync(imgUrl);
var bitmap = new Bitmap(stream);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are you creating Bitmap since the Avalonia one does not support it?

Copy link
Owner Author

@MakeshVineeth MakeshVineeth Oct 27, 2021

Choose a reason for hiding this comment

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

Yeah, AvaloniaBitmap is not working for some unknown reasons. In fact, it's breaking the plugin completely.

var wiki = await httpClient.GetFromJsonAsync<Wiki>(url, SerializerOptions);
if (wiki == null) return default;

Dictionary<string, PageView>.ValueCollection pages = wiki.Query.Pages.Values;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Check for null wiki?.Query?.Pages?.Values

</ItemGroup>
<ItemGroup>
<PackageReference Include="AsyncEnumerator" Version="4.0.2"/>
<PackageReference Include="Blast.API" Version="0.9.89.1-beta"/>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Update NuGets

<ItemGroup>
<PackageReference Include="AsyncEnumerator" Version="4.0.2"/>
<PackageReference Include="Blast.API" Version="0.9.89.1-beta"/>
<PackageReference Include="TextCopy" Version="1.5.1"/>
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is redundant

</ItemGroup>

<ItemGroup>
<None Remove="Wikipedia-logo.png"/>
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is redundant

@MakeshVineeth MakeshVineeth requested a review from adirh3 October 27, 2021 12:59
using var imageClient = new HttpClient();
imageClient.DefaultRequestHeaders.UserAgent.TryParseAdd(UserAgentString);

using HttpResponseMessage response =
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can use GetAsStream instead of using GetAsync and then ReadAsStreamAsync

@MakeshVineeth MakeshVineeth merged commit aee4954 into master Nov 4, 2021
@MakeshVineeth MakeshVineeth deleted the v1.3 branch November 4, 2021 08:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants