Add CoreWidgetProvider & SSH Wallet widget #320
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dhoehna
reviewed
May 2, 2023
dhoehna
reviewed
May 2, 2023
dhoehna
reviewed
May 2, 2023
| using Microsoft.Windows.ApplicationModel.Resources; | ||
|
|
||
| namespace CoreWidgetProvider.Helpers; | ||
| public static class Resources |
There was a problem hiding this comment.
Doesn't DevHome already have a system for accessing resources? @AmelBawa-msft
There was a problem hiding this comment.
Ping @AmelBawa-msft. Is another Resources ok here?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
dhoehna
reviewed
May 2, 2023
| // 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); |
There was a problem hiding this comment.
string in C# are immutable. Looks like str is replaced for every identifier.
There was a problem hiding this comment.
yes.
Resources.cs taken from https://github.com/microsoft/devhomegithubextension/blob/main/src/GithubPlugin/Helpers/Resources.cs
dhoehna
reviewed
May 2, 2023
dhoehna
reviewed
May 2, 2023
CoreWidgetProvider/CoreWidgetProvider/Widgets/SSHWalletWidget.cs
Outdated
Show resolved
Hide resolved
dhoehna
reviewed
May 2, 2023
CoreWidgetProvider/CoreWidgetProvider/Widgets/SSHWalletWidget.cs
Outdated
Show resolved
Hide resolved
dhoehna
reviewed
May 2, 2023
CoreWidgetProvider/CoreWidgetProvider/Widgets/SSHWalletWidget.cs
Outdated
Show resolved
Hide resolved
dhoehna
reviewed
May 2, 2023
CoreWidgetProvider/CoreWidgetProvider/Widgets/SSHWalletWidget.cs
Outdated
Show resolved
Hide resolved
dhoehna
reviewed
May 2, 2023
CoreWidgetProvider/CoreWidgetProvider/Widgets/SSHWalletWidget.cs
Outdated
Show resolved
Hide resolved
dhoehna
reviewed
May 2, 2023
CoreWidgetProvider/CoreWidgetProvider/Widgets/SSHWalletWidget.cs
Outdated
Show resolved
Hide resolved
dhoehna
reviewed
May 2, 2023
CoreWidgetProvider/CoreWidgetProvider/Widgets/SSHWalletWidget.cs
Outdated
Show resolved
Hide resolved
dhoehna
reviewed
May 2, 2023
CoreWidgetProvider/CoreWidgetProvider/Widgets/SSHWalletWidget.cs
Outdated
Show resolved
Hide resolved
dhoehna
reviewed
May 2, 2023
|
|
||
| [JsonSourceGenerationOptions(WriteIndented = true)] | ||
| [JsonSerializable(typeof(DataPayload))] | ||
| internal partial class SourceGenerationContext : JsonSerializerContext |
dhoehna
reviewed
May 2, 2023
dhoehna
reviewed
May 2, 2023
CoreWidgetProvider/CoreWidgetProvider/Widgets/WidgetProvider.cs
Outdated
Show resolved
Hide resolved
dhoehna
reviewed
May 2, 2023
CoreWidgetProvider/CoreWidgetProvider/Widgets/WidgetProvider.cs
Outdated
Show resolved
Hide resolved
dhoehna
reviewed
May 2, 2023
CoreWidgetProvider/CoreWidgetProvider/Widgets/WidgetProvider.cs
Outdated
Show resolved
Hide resolved
dhoehna
reviewed
May 2, 2023
CoreWidgetProvider/CoreWidgetProvider/Widgets/WidgetProvider.cs
Outdated
Show resolved
Hide resolved
dhoehna
reviewed
May 2, 2023
CoreWidgetProvider/CoreWidgetProvider/Widgets/WidgetProvider.cs
Outdated
Show resolved
Hide resolved
dhoehna
reviewed
May 2, 2023
CoreWidgetProvider/CoreWidgetProviderPackage/CoreWidgetProviderPackage.wapproj
Outdated
Show resolved
Hide resolved
dhoehna
reviewed
May 2, 2023
CoreWidgetProvider/CoreWidgetProviderPackage/CoreWidgetProviderPackage.wapproj
Outdated
Show resolved
Hide resolved
dhoehna
reviewed
May 2, 2023
There was a problem hiding this comment.
No icons? Do we have icons for the core widget?
There was a problem hiding this comment.
Not yet. Any ideas for the icon from MDL icons? If not, where to ask for help for icons?
dhoehna
reviewed
May 2, 2023
CoreWidgetProvider/CoreWidgetProviderPackage/Package.appxmanifest
Outdated
Show resolved
Hide resolved
dhoehna
reviewed
May 2, 2023
CoreWidgetProvider/CoreWidgetProviderPackage/Package.appxmanifest
Outdated
Show resolved
Hide resolved
dhoehna
reviewed
May 2, 2023
dhoehna
previously requested changes
May 2, 2023
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
stefansjfw
force-pushed
the
user/stefansjfw/ssh-wallet-widget
branch
from
65ef9c3 to
7c651f6
Compare
stefansjfw
force-pushed
the
user/stefansjfw/ssh-wallet-widget
branch
from
7c651f6 to
2539b17
Compare
stefansjfw
force-pushed
the
user/stefansjfw/ssh-wallet-widget
branch
from
2539b17 to
599d970
Compare
stefansjfw
force-pushed
the
user/stefansjfw/ssh-wallet-widget
branch
2 times, most recently
from
600d533 to
607755d
Compare
stefansjfw
force-pushed
the
user/stefansjfw/ssh-wallet-widget
branch
from
607755d to
d5c9f81
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
crutkas
dismissed
dhoehna’s stale review
Feedback was addressed, Asked cory and he said override was ok as Kristen is reviewing actively
EricJohnson327
approved these changes
May 10, 2023
EricJohnson327
approved these changes
May 10, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary of the pull request
References and relevant issues
Detailed description of the pull request / Additional comments
Validation steps performed
PR checklist