Skip to content

Conversation

@taooceros
Copy link
Member

This pull request changes the Get & Download method implementation in Flow.Launcher.Infrastructure.Http by moving the old HttpWebRequest API to new HttpClient.

This has fixed my issue of connection to Github while checking for new Update.

@taooceros
Copy link
Member Author

Seems that Fody provides easy an injection for IPropertyChanged, which is included in Flow........but anyway, since I have written it, just leave it there.....

@taooceros taooceros requested a review from jjw24 December 21, 2020 16:41
@taooceros taooceros marked this pull request as ready for review December 21, 2020 16:41
2. Manually replace "#" with "%23" to solve the similar issue in Explorer plugin
3. Add GetAsync method with Uri as argument
4. Remove unused encoding argument
5. Change exception type for WebSearch Plguin
6. Update Comment

public static async Task Download([NotNull] string url, [NotNull] string filePath)
{
using var response = await client.GetAsync(url);
Copy link
Member

Choose a reason for hiding this comment

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

client.GetAsync crashes the entire app for me when i try to install plugin. Cant see any reason why though, no exception is thrown

Copy link
Member Author

Choose a reason for hiding this comment

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

Let me take a check.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh that's because before the Http.Download is not async, so no await is added there.

@jjw24
Copy link
Member

jjw24 commented Dec 29, 2020

getting this error also when i check for update:

image

@taooceros
Copy link
Member Author

taooceros commented Dec 29, 2020

getting this error also when i check for update:

image

Fix it, and the previous one. Forget to commit seperately, so both is on last commit. @jjw24
This is due to that I forget to update the JsonProperty in Newtonsoft.Json to JsonPropertyName in System.Text.Json

@taooceros taooceros requested a review from jjw24 December 29, 2020 10:37
@taooceros
Copy link
Member Author

taooceros commented Dec 29, 2020

Hold on, update also call the method Http.Download so need to change that as well.

@taooceros
Copy link
Member Author

taooceros commented Dec 29, 2020

image
emmm a question. Why do we need the mainthread to exercute this method? @jjw24

@taooceros
Copy link
Member Author

Done, please review. @jjw24
and it seems that we don't need to invoke Install method into main thread......

try
{
const string api = "http://suggestion.baidu.com/su?json=1&wd=";
result = await Http.Get(api + Uri.EscapeUriString(query), "GB2312");
Copy link
Member

Choose a reason for hiding this comment

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

removal of "GB2312" is intentional?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I have checked, the ReadAsString doesn't need the encoding to create the result.

@jjw24
Copy link
Member

jjw24 commented Dec 29, 2020

good work

@jjw24
Copy link
Member

jjw24 commented Dec 29, 2020

for reference on why use ConfigureAwait(false)

https://medium.com/bynder-tech/c-why-you-should-use-configureawait-false-in-your-library-code-d7837dce3d7f

@jjw24 jjw24 merged commit 58d281d into Flow-Launcher:dev Dec 29, 2020
@taooceros taooceros deleted the UpdateHttpMaster branch January 9, 2021 03:00
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