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

Add CoreWidgetProvider & SSH Wallet widget #320

Merged
merged 35 commits into from
May 10, 2023

Conversation

stefansjfw
Copy link
Contributor

@stefansjfw stefansjfw commented May 2, 2023

Summary of the pull request

SSHWalletWidget

References and relevant issues

Detailed description of the pull request / Additional comments

Validation steps performed

PR checklist

  • Closes #xxx
  • Tests added/passed
  • Documentation updated

using Microsoft.Windows.ApplicationModel.Resources;

namespace CoreWidgetProvider.Helpers;
public static class Resources
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't DevHome already have a system for accessing resources? @AmelBawa-msft

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ping @AmelBawa-msft. Is another Resources ok here?

Copy link
Contributor

Choose a reason for hiding this comment

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

We do have class StringResource, ideally we need a single implementation, unless the current one does not work for your use case. I do want to add an option for specifying CultureInfo format, IIRC by default we use CurrentCulture, but we should allow more flexibility.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see. Would be nice to reuse that. However, it should probably be moved to DevHome.Common from DevHome/Services first so I'd prefer doing this in a follow-up PR

// What is faster, String.Replace, RegEx, or StringBuilder.Replace? It is String.Replace().
// https://learn.microsoft.com/en-us/archive/blogs/debuggingtoolbox/comparing-regex-replace-string-replace-and-stringbuilder-replace-which-has-better-performance
var resourceString = GetResource(identifier, log);
str = str.Replace($"%{identifier}%", resourceString);
Copy link
Contributor

Choose a reason for hiding this comment

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

string in C# are immutable. Looks like str is replaced for every identifier.

Copy link
Contributor Author

Choose a reason for hiding this comment

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


[JsonSourceGenerationOptions(WriteIndented = true)]
[JsonSerializable(typeof(DataPayload))]
internal partial class SourceGenerationContext : JsonSerializerContext
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT: 1 class per file.

Copy link
Contributor

Choose a reason for hiding this comment

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

No icons? Do we have icons for the core widget?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not yet. Any ideas for the icon from MDL icons? If not, where to ask for help for icons?

dhoehna
dhoehna previously requested changes May 2, 2023
Copy link
Contributor

@dhoehna dhoehna left a comment

Choose a reason for hiding this comment

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

Please remove duplicate code.

Please address the side-effects.

Please remove ARM support.

@dhoehna dhoehna requested review from manodasanW and florelis May 2, 2023 17:35
@krschau
Copy link
Collaborator

krschau commented May 9, 2023

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@stefansjfw stefansjfw force-pushed the user/stefansjfw/ssh-wallet-widget branch from 65ef9c3 to 7c651f6 Compare May 10, 2023 12:38
@microsoft microsoft deleted a comment from azure-pipelines bot May 10, 2023
@stefansjfw stefansjfw force-pushed the user/stefansjfw/ssh-wallet-widget branch from 7c651f6 to 2539b17 Compare May 10, 2023 12:45
@microsoft microsoft deleted a comment from azure-pipelines bot May 10, 2023
@stefansjfw stefansjfw force-pushed the user/stefansjfw/ssh-wallet-widget branch from 2539b17 to 599d970 Compare May 10, 2023 12:47
@microsoft microsoft deleted a comment from azure-pipelines bot May 10, 2023
@stefansjfw stefansjfw force-pushed the user/stefansjfw/ssh-wallet-widget branch 2 times, most recently from 600d533 to 607755d Compare May 10, 2023 13:39
@microsoft microsoft deleted a comment from azure-pipelines bot May 10, 2023
@microsoft microsoft deleted a comment from azure-pipelines bot May 10, 2023
@stefansjfw stefansjfw force-pushed the user/stefansjfw/ssh-wallet-widget branch from 607755d to d5c9f81 Compare May 10, 2023 14:13
@microsoft microsoft deleted a comment from azure-pipelines bot May 10, 2023
@microsoft microsoft deleted a comment from azure-pipelines bot May 10, 2023
@microsoft microsoft deleted a comment from azure-pipelines bot May 10, 2023
@microsoft microsoft deleted a comment from azure-pipelines bot May 10, 2023
@stefansjfw
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@microsoft microsoft deleted a comment from azure-pipelines bot May 10, 2023
@crutkas crutkas dismissed dhoehna’s stale review May 10, 2023 16:31

Feedback was addressed, Asked cory and he said override was ok as Kristen is reviewing actively

@crutkas crutkas merged commit dbeecf1 into main May 10, 2023
@stefansjfw stefansjfw deleted the user/stefansjfw/ssh-wallet-widget branch May 10, 2023 19:32
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.

9 participants