-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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
[PT Run] System plugin: Add IP and MAC #17023
Conversation
@check-spelling-bot Report🔴 Please reviewSee the files view or the action log for details. Unrecognized words (8)adresses To accept these unrecognized words as correct, run the following commands... in a clone of the git@github.com:htcfreek/PowerToys.git repository
If the flagged items do not appear to be textIf items relate to a ...
|
@niels9001 |
...ncher/Plugins/Microsoft.PowerToys.Run.Plugin.System/Components/SystemCommandResultContext.cs
Outdated
Show resolved
Hide resolved
...ncher/Plugins/Microsoft.PowerToys.Run.Plugin.System/Components/SystemCommandResultContext.cs
Outdated
Show resolved
Hide resolved
@check-spelling-bot Report🔴 Please reviewSee the files view or the action log for details. Unrecognized words (10)Dns To accept these unrecognized words as correct, run the following commands... in a clone of the git@github.com:htcfreek/PowerToys.git repository
If the flagged items do not appear to be textIf items relate to a ...
|
Doesn't it make sense to just use the Settings plugin icon as this is part of that plugin? My worry is that it might become challenging to find relevant icons for all extensions at some point. (e.g. a network card icon for IP address might be confusing to users) |
It's system plugin angd not settings plugin. And we don't have a useful generic icon in this plugin. For me the settings icon don't feels right. But we can use the "network adapter" icon for both. That would reduce the number of different icons. I plan to add some network commands like |
@check-spelling-bot Report🔴 Please reviewSee the files view or the action log for details. Unrecognized words (13)
To accept these unrecognized words as correct, run the following commands... in a clone of the git@github.com:htcfreek/PowerToys.git repository
If the flagged items do not appear to be textIf items relate to a ...
|
@check-spelling-bot Report🔴 Please reviewSee the files view or the action log for details. Unrecognized words (2)etwork Previously acknowledged words that are now absentGbps MbpsTo accept these unrecognized words as correct (and remove the previously acknowledged and now absent words), run the following commands... in a clone of the git@github.com:htcfreek/PowerToys.git repository
If the flagged items do not appear to be textIf items relate to a ...
|
@check-spelling-bot Report🔴 Please reviewSee the files view or the action log for details. Unrecognized words (1)etwork Previously acknowledged words that are now absentMbpsTo accept these unrecognized words as correct (and remove the previously acknowledged and now absent words), run the following commands... in a clone of the git@github.com:htcfreek/PowerToys.git repository
If the flagged items do not appear to be textIf items relate to a ...
|
@jsoref |
...cher/Plugins/Microsoft.PowerToys.Run.Plugin.System/Components/NetworkConnectionProperties.cs
Outdated
Show resolved
Hide resolved
...les/launcher/Plugins/Microsoft.PowerToys.Run.Plugin.System/Components/SystemPluginContext.cs
Outdated
Show resolved
Hide resolved
src/modules/launcher/Plugins/Microsoft.PowerToys.Run.Plugin.System/Components/ResultHelper.cs
Outdated
Show resolved
Hide resolved
@htcfreek: I'm sorry. At some point, I'm going to probably move the magic that handles Not sure about the other bit, can you clarify? |
@jsoref |
@check-spelling-bot Report🔴 Please reviewSee the files view or the action log for details. Unrecognized words (1)etwork Previously acknowledged words that are now absentMbpsTo accept these unrecognized words as correct (and remove the previously acknowledged and now absent words), run the following commands... in a clone of the git@github.com:htcfreek/PowerToys.git repository
If the flagged items do not appear to be textIf items relate to a ...
|
@niels9001 |
@htcfreek: So, the base dictionary appears to have:
(I have no idea what The dictionary's diff --git a/.github/actions/spell-check/expect.txt b/.github/actions/spell-check/expect.txt
index 0279f0ca3..81714abde 100644
--- a/.github/actions/spell-check/expect.txt
+++ b/.github/actions/spell-check/expect.txt
@@ -1185,3 +1185,2 @@ maxversiontested
Mbits
-Mbps
MBs
@@ -2391,2 +2390,2 @@ Zoneszonabletester
Zonev
-zzz
\ No newline at end of file
+zzz
diff --git a/.github/actions/spell-check/patterns.txt b/.github/actions/spell-check/patterns.txt
index 66274dccd..a9bc2dca1 100644
--- a/.github/actions/spell-check/patterns.txt
+++ b/.github/actions/spell-check/patterns.txt
@@ -51,2 +51,3 @@ TestCase\("[^"]+"
\\netstandard
+\\network
\\notifications
diff --git a/.github/actions/spell-check/reject.txt b/.github/actions/spell-check/reject.txt
new file mode 100644
index 000000000..9e4575656
--- /dev/null
+++ b/.github/actions/spell-check/reject.txt
@@ -0,0 +1 @@
+mbps You want the patterns change and expect changes, but obviously for this PR you wouldn't want to add the reject file, it's just for demonstration purposes. The notable thing about the dictionary is that it doesn't have |
I added |
Yeah, I know it's confusing. Dictionaries can evolve, but in general, any dictionary will be a snapshot in time. And in order to not surprise people, I haven't evolved the dictionary check-spelling uses. I do think I'll probably at some point make at least one revision to the dictionary. Including What would happen is that there'd be a newer version of the dictionary available, and people could opt-in to use it. And then much further into the future, I might change the default, so at an upgrade, people could get that newer dictionary, but opt to use the older one. I think my general advice is to not try to predict what the spell checker will suggest putting into But do note that the spell checker only thinks about words that match its sense of words, so
|
There's a space between the literal https://en.wikipedia.org/wiki/IPv4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It'd be something like this,
plus adding this to patterns.txt
(from https://github.com/check-spelling/check-spelling/wiki/Configuration-Examples:-patterns)
# version suffix <word>v#
[Vv]\d+(?:\b|(?=[a-zA-Z_]))
src/modules/launcher/Plugins/Microsoft.PowerToys.Run.Plugin.System.UnitTests/QueryTests.cs
Outdated
Show resolved
Hide resolved
...cher/Plugins/Microsoft.PowerToys.Run.Plugin.System/Components/NetworkConnectionProperties.cs
Outdated
Show resolved
Hide resolved
...cher/Plugins/Microsoft.PowerToys.Run.Plugin.System/Components/NetworkConnectionProperties.cs
Outdated
Show resolved
Hide resolved
...cher/Plugins/Microsoft.PowerToys.Run.Plugin.System/Components/NetworkConnectionProperties.cs
Outdated
Show resolved
Hide resolved
...cher/Plugins/Microsoft.PowerToys.Run.Plugin.System/Components/NetworkConnectionProperties.cs
Outdated
Show resolved
Hide resolved
src/modules/launcher/Plugins/Microsoft.PowerToys.Run.Plugin.System/Properties/Resources.resx
Outdated
Show resolved
Hide resolved
src/modules/launcher/Plugins/Microsoft.PowerToys.Run.Plugin.System/Properties/Resources.resx
Outdated
Show resolved
Hide resolved
src/modules/launcher/Plugins/Microsoft.PowerToys.Run.Plugin.System/Properties/Resources.resx
Outdated
Show resolved
Hide resolved
src/modules/launcher/Plugins/Microsoft.PowerToys.Run.Plugin.System/Properties/Resources.resx
Outdated
Show resolved
Hide resolved
src/modules/launcher/Plugins/Microsoft.PowerToys.Run.Plugin.System/Properties/Resources.resx
Outdated
Show resolved
Hide resolved
@jaimecbernardo |
@check-spelling-bot Report🔴 Please reviewSee the files view or the action log for details. Unrecognized words (1)ipv Previously acknowledged words that are now absentDavid IPv MortonTo accept these unrecognized words as correct (and remove the previously acknowledged and now absent words), run the following commands... in a clone of the git@github.com:htcfreek/PowerToys.git repository
If the flagged items do not appear to be textIf items relate to a ...
|
@jsoref @stefansjfw @niels9001 |
@htcfreek Here you go :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@htcfreek: looks good,
fwiw, you no longer need IPv
in expect
(and can also apparently remove settingsv
which probably also was matched by the pattern):
https://github.com/microsoft/PowerToys/runs/5612548869?check_suite_focus=true#step:4:70
(and you can remove the other two items as well, or wait for someone else to...)
src/modules/launcher/Plugins/Microsoft.PowerToys.Run.Plugin.System.UnitTests/ImageTests.cs
Show resolved
Hide resolved
src/modules/launcher/Plugins/Microsoft.PowerToys.Run.Plugin.System.UnitTests/QueryTests.cs
Show resolved
Hide resolved
I think the patterns are bad if they mark things as okay that don't have anything to do with And btw, |
Nah, the parsing is right My guess is that both David and Morton are in the base dictionary and thus don't need to be in expect. (I'd have to check.) |
Than you have updated base dict. Please let use move this dictionary cleanup into an other PR. |
No set date yet, but most likely it should be mid-end of this week. |
Currently I am waiting for a new approval from @stefansjfw and then this can go in. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Although, I'm not completely sure about "Reduce score" settings. But I guess we can adapt in the future if needed - ip and mac results are still on top of other plugins which could make some users unhappy. But, as I said, let's see..
What do you searched? I tried my best. But this is a fuzzyMatch scoring thing. What is you worry about the setting? |
That we'll end up with more "Reduce score" settings, and then scoring won't make sense :) |
What about |
I don't have strong feeling about this. It's more of a suggestion. @crutkas @jaimecbernardo WDYT? |
@jaimecbernardo |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just gave it a try. It feels OK to me. Thanks for the contribution.
It's ok to return these results in a good position from just "ip" or "mac". I like that the setting to reduce is on by default as well. Let's see how people feel about these. |
Summary of the Pull Request
What is this about:
This adds two new commands to show the IPs and MAC addresses of each network connection interface. To search for them on global queries the search term has to start with
ip
,mac
oraddress
.Note:
The new image has a not so good quality at the moment. I have asked @niels9001 for a better one. But I think the image should not block this PR from merging.The symbol is updated. (But not the screenshot.)How does someone test / validate:
Test with local build.
Quality Checklist
Contributor License Agreement (CLA)
A CLA must be signed. If not, go over here and sign the CLA.