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

Fix clipboard warnings #11947

Merged

Conversation

kiminuo
Copy link
Collaborator

@kiminuo kiminuo commented Nov 16, 2023

No description provided.

@@ -11,8 +11,9 @@ public async Task<string> GetTextAsync()
{
if (ApplicationHelper.Clipboard is { } clipboard)
{
return await clipboard.GetTextAsync();
return await clipboard.GetTextAsync() ?? "";
Copy link
Collaborator Author

@kiminuo kiminuo Nov 16, 2023

Choose a reason for hiding this comment

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

Shouldn't be there ConfigureAwait(false)? Similarly elsewhere.

Copy link
Collaborator

@adamPetho adamPetho Nov 16, 2023

Choose a reason for hiding this comment

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

I think we don't use ConfigureAwait(false) in UiModels and ViewModels. Not sure why.

Copy link
Collaborator

@soosr soosr Nov 20, 2023

Choose a reason for hiding this comment

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

The usage of ConfigureAwait(false) doesn't depend on ViewModels or Models, it depends on what code is executed after the awaited call.

For example:

	public async Task MethodA()
	{
		await MethodB();
		// Here are some UI related code executed afterward.
	}

	public async Task MethodB()
	{
		await MethodC().ConfigureAwait(false);
		// NO UI related code executed afterward.
	}

After awaiting MethodB() you cannot add ConfigureAwait(false) because after it you need to stay on the UI thread as you deal with objects that must be changed on the UI thread.
While in MethodB() you can safely add it as after calling MethodC() you don't have to stay on UI thread as there is no UI related code executed.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't be there ConfigureAwait(false)? Similarly elsewhere.

Yes, it should be there. And TBH I don't think that it was added anywhere else in the UI, so improving the performance, it would be worth checking and fixing it if needed.

@kiminuo kiminuo requested a review from soosr November 16, 2023 09:33
Copy link
Collaborator

@adamPetho adamPetho left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -11,8 +11,9 @@ public async Task<string> GetTextAsync()
{
if (ApplicationHelper.Clipboard is { } clipboard)
{
return await clipboard.GetTextAsync();
return await clipboard.GetTextAsync() ?? "";
Copy link
Collaborator

@soosr soosr Nov 20, 2023

Choose a reason for hiding this comment

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

The usage of ConfigureAwait(false) doesn't depend on ViewModels or Models, it depends on what code is executed after the awaited call.

For example:

	public async Task MethodA()
	{
		await MethodB();
		// Here are some UI related code executed afterward.
	}

	public async Task MethodB()
	{
		await MethodC().ConfigureAwait(false);
		// NO UI related code executed afterward.
	}

After awaiting MethodB() you cannot add ConfigureAwait(false) because after it you need to stay on the UI thread as you deal with objects that must be changed on the UI thread.
While in MethodB() you can safely add it as after calling MethodC() you don't have to stay on UI thread as there is no UI related code executed.

@@ -11,8 +11,9 @@ public async Task<string> GetTextAsync()
{
if (ApplicationHelper.Clipboard is { } clipboard)
{
return await clipboard.GetTextAsync();
return await clipboard.GetTextAsync() ?? "";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't be there ConfigureAwait(false)? Similarly elsewhere.

Yes, it should be there. And TBH I don't think that it was added anywhere else in the UI, so improving the performance, it would be worth checking and fixing it if needed.

Copy link
Collaborator

@soosr soosr left a comment

Choose a reason for hiding this comment

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

ACK.

Fixing the missing ConfigureAwait(false) everywhere in UI should be done in a separate PR.

@kiminuo kiminuo merged commit 8a4e96d into WalletWasabi:master Nov 20, 2023
7 checks passed
@kiminuo kiminuo deleted the feature/2023-11-16-UiClipboard-warnings branch November 20, 2023 09:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants