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

[PT Run][New Plugin] Unit Converter (#9800) #11406

Merged
merged 7 commits into from
Jun 8, 2021

Conversation

enricogior
Copy link
Contributor

@enricogior enricogior commented May 21, 2021

Summary of the Pull Request

What is this about:
See #9800

What is include in the PR:

How does someone test / validate:

Quality Checklist

  • Linked issue: [PowerToys Run] Unit conversion #9577
  • Communication: I've discussed this with core contributors in the issue.
  • Tests: Added/updated and all pass
  • Installer: Added/updated and all pass
  • Localization: All end user facing strings can be localized
  • Docs: Added/ updated
  • Binaries: Any new files are added to WXS / YML

Contributor License Agreement (CLA)

A CLA must be signed. If not, go over here and sign the CLA.

@enricogior enricogior added the Run-Plugin Things that relate with PowerToys Run's plugin interface label May 21, 2021
@enricogior enricogior force-pushed the user/ThiefZero/unitConversion branch from c063b86 to cc275fa Compare May 21, 2021 15:23
@enricogior
Copy link
Contributor Author

@ThiefZero
can you do a sanity check to verify this branch is OK? Thanks.

@microsoft microsoft deleted a comment from github-actions bot May 21, 2021
@htcfreek
Copy link
Collaborator

We should add a list of open problems/work items to this pr description. Only for saving it somehow and even if we plan to fix it in a separate PR/Issue.

@ThiefZero
Can you please add it. I think you have the best overview on it.

@microsoft microsoft deleted a comment from github-actions bot May 21, 2021
@enricogior enricogior marked this pull request as draft May 21, 2021 15:30
@htcfreek
Copy link
Collaborator

@crutkas, @jsoref
Why do we have the last two bot comments? For me it seems that the bot is triggered by comments sometimes.

@jsoref
Copy link
Contributor

jsoref commented May 21, 2021

@htcfreek, I had accidentally included an unfinished feature, but it should be gone as long as your code is current on both sides: 58d41d4

Otherwise, you get one comment for your Push and one comment for the Pull Request (if you push w/o creating a PR, you can fix things up before you create the PR).

@ThiefZero
Copy link
Contributor

@ThiefZero
can you do a sanity check to verify this branch is OK? Thanks.

Everything seems to be working here 👍.

@htcfreek

Future improvements:

  • A nicer way of rounding numbers (in addition to regular rounding, 1 mg in kg should display 0.000001 kg instead of 0 kg)
  • Use resource to translate from code unit names to something more human friendly on some units (e.g. degreeFahrenheit to degree Fahrenheit).

@enricogior
Copy link
Contributor Author

@jsoref
the strange thing is that the spell checker is still failing even if the branch has been rebased on current master.
Anyway this seems the only branch affected by this problem, probably not worthy to investigate.

@enricogior enricogior marked this pull request as ready for review May 24, 2021 09:08
@enricogior
Copy link
Contributor Author

Issue created to track the localization task #11429

@enricogior
Copy link
Contributor Author

enricogior commented May 24, 2021

New plugin checklist:

New plugin checklist

  • The plugin is a project under modules\launcher\Plugins
  • Microsoft plugin project name pattern: Microsoft.PowerToys.Run.Plugin.{PluginName}
  • Community plugin project name pattern: Community.PowerToys.Run.Plugin.{PluginName}
  • GlobalSuppressions.cs and StyleCop.json have to be included in the plugin project so it follows PowerToys code guidelines
  • The project file should import Version.props and specify <Version>$(Version).0</Version>
  • Make sure *.csproj specify only x64 platform target
  • The plugin has to contain a plugin.json file of the following format in its root folder
{
  "ID": string, // GUID string
  "ActionKeyword": string, // Direct activation phrase
  "IsGlobal": boolean,
  "Name": string, // Has to be unique, same as 'PluginName' in the project name pattern  
  "Author": string,
  "Version": "1.0.0", // For future compatibility
  "Language": "csharp", // So far we support only csharp 
  "Website": "https://aka.ms/powertoys",
  "ExecuteFileName": string, // Should be {Type}.PowerToys.Run.Plugin.{PluginName}.dll
  "IcoPathDark": string, // Path to dark theme icon. The path is relative to the root plugin folder 
  "IcoPathLight": string // Path to light theme icon. The path is relative to the root plugin folder 
}
  • Do not use plugin name or PowerToys as prefixes for entities inside of the plugin project
  • The plugin has to have Unit tests. Use MSTest framework
  • To enable localization add LocProject.json file to the plugin root folder. For details see localization.md
  • Plugin's output code and assets have to be included in the installer Product.wxs
  • Test the plugin with a local build. Build the installer, install, check that the plugin works as expected
  • All plugin's binaries have to be included in the signed build pipeline.user.windows.yml
  • The plugin target framework has to be .NET Core 3.1. All dependencies have to have .NET 5 version

@jsoref
Copy link
Contributor

jsoref commented May 24, 2021

@enricogior: eww, no, that's definitely worth investigating.

Things one can do (on a branch not tied to a PR, possibly even in a personal repository):

  1. Add debug: 1 to a with: section for the action itself.
  2. Change action the version to @prerelease.

I'll investigate later today. Do note that @microsoft deleted some comments e.g. #11406 (comment) – I'm not sure what they said.

I've made some changes recently to the logging output which should help a little, but this gives me the idea that I should ensure that the number of unrecognized words is reported in the logs.

@jsoref
Copy link
Contributor

jsoref commented May 24, 2021

@enricogior: check-spelling-sandbox@cc275fa#commitcomment-51227973

@check-spelling-bot Report

Unrecognized words, please review:

  • Hrenhei

...

To accept these unrecognized words as correct (and remove the previously acknowledged and now absent words), run the following commands

... in a clone of the [email protected]:check-spelling/PowerToys.git repository
on the user/ThiefZero/unitConversion branch:

update_files() {
perl -e '
my @expect_files=qw('".github/actions/spell-check/expect.txt"');
@ARGV=@expect_files;
my @stale=qw('"$patch_remove"');
my $re=join "|", @stale;
my $suffix=".".time();
my $previous="";
sub maybe_unlink { unlink($_[0]) if $_[0]; }
while (<>) {
if ($ARGV ne $old_argv) { maybe_unlink($previous); $previous="$ARGV$suffix"; rename($ARGV, $previous); open(ARGV_OUT, ">$ARGV"); select(ARGV_OUT); $old_argv = $ARGV; }
next if /^(?:$re)(?:(?:\r|\n)*$| .*)/; print;
}; maybe_unlink($previous);'
perl -e '
my $new_expect_file=".github/actions/spell-check/expect.txt";
use File::Path qw(make_path);
use File::Basename qw(dirname);
make_path (dirname($new_expect_file));
open FILE, q{<}, $new_expect_file; chomp(my @words = <FILE>); close FILE;
my @add=qw('"$patch_add"');
my %items; @items{@words} = @words x (1); @items{@add} = @add x (1);
@words = sort {lc($a)."-".$a cmp lc($b)."-".$b} keys %items;
open FILE, q{>}, $new_expect_file; for my $word (@words) { print FILE "$word\n" if $word =~ /\w/; };
close FILE;
system("git", "add", $new_expect_file);
'
}

comment_json=$(mktemp)
curl -L -s -S \
  --header "Content-Type: application/json" \
  "https://api.github.com/repos/check-spelling/PowerToys/comments/51227973" > "$comment_json"
comment_body=$(mktemp)
jq -r .body < "$comment_json" > $comment_body
rm $comment_json

patch_remove=$(perl -ne 'next unless s{^</summary>(.*)</details>$}{$1}; print' < "$comment_body")
  

patch_add=$(perl -e '$/=undef;
$_=<>;
s{<details>.*}{}s;
s{^#.*}{};
s{\n##.*}{};
s{(?:^|\n)\s*\*}{}g;
s{\s+}{ }g;
print' < "$comment_body")
  
update_files
rm $comment_body
git add -u
If you see a bunch of garbage

If it relates to a ...

well-formed pattern

See if there's a pattern that would match it.

If not, try writing one and adding it to the patterns.txt file.

Patterns are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your lines.

Note that patterns can't match multiline strings.

binary-ish string

Please add a file path to the excludes.txt file instead of just accepting the garbage.

File paths are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your files.

^ refers to the file's path from the root of the repository, so ^README\.md$ would exclude README.md (on whichever branch you're using).

@ThiefZero: Personally, I'd recommend adding a patterns.txt entry, something like:

# TestCase strings intentionally have non dictionary items
\[TestCase\(new string.*\]

@jsoref
Copy link
Contributor

jsoref commented May 24, 2021

Fwiw, the prerelease version handles this much better: https://github.com/check-spelling/PowerToys/runs/2657003343?check_suite_focus=true#step:4:23

Checking spelling...
(...Spell check files...)
Checking 1853 files
(...Spell check...)
Warning: src/modules/launcher/Plugins/Community.PowerToys.Run.Plugin.UnitConverter.UnitTest/InputInterpreterTests.cs: line 38, columns 59-65, Warning - `Hrenhei` is not a recognized word. (unrecognized-spelling)
(...Compare expect with new output...)
(...New output...)
(...Check for extra dictionaries...)
(...Unrecognized...)
Preparing a comment for push
https://api.github.com/repos/check-spelling/PowerToys/commits/ccc023c1113c36cf6ebc581fe81b98baf8533c03/comments
Please review
Error: Process completed with exit code 1.

check-spelling-sandbox@ccc023c#diff-07d72036bf4b7ef08b146ba6ac847004726a2442084984d4d8028c021fc6866aR38

I'm going to remove the url it's spitting out before Please review as that's not helpful (it was for debugging and I should have removed it before shipping 18).

I think I'm going to change Unrecognized to Unrecognized (x) where x is the number found just like prerelease's check-spelling-sandbox@ccc023c#commitcomment-51228789:

@check-spelling-bot Report

Please review:

Unrecognized words (1)
Hrenhei

@github-actions
Copy link

github-actions bot commented May 24, 2021

@check-spelling-bot Report

Unrecognized words, please review:

  • Hrenhei
Previously acknowledged words that are now absent apos autoupdate bc bh Chris's classmethod cls CMDARG cn cx cz deque df dw EB ev fd fody fx getmembers gh hc hh hk hu ip ismethod jp Kf lambson laute loadingbar messagebox multithreading nonwin NX overlaywindow pb Pn pv pw px QI rpc ru rv rx sz tadele td tt tw Tz ul unitconvert uv vh vk vm vw Vx wostringstream xa XY yy Zc zh zipfolder zm
To accept these unrecognized words as correct (and remove the previously acknowledged and now absent words), run the following commands

... in a clone of the [email protected]:microsoft/PowerToys.git repository
on the user/ThiefZero/unitConversion branch:

update_files() {
perl -e '
my @expect_files=qw('".github/actions/spell-check/expect.txt"');
@ARGV=@expect_files;
my @stale=qw('"$patch_remove"');
my $re=join "|", @stale;
my $suffix=".".time();
my $previous="";
sub maybe_unlink { unlink($_[0]) if $_[0]; }
while (<>) {
if ($ARGV ne $old_argv) { maybe_unlink($previous); $previous="$ARGV$suffix"; rename($ARGV, $previous); open(ARGV_OUT, ">$ARGV"); select(ARGV_OUT); $old_argv = $ARGV; }
next if /^(?:$re)(?:(?:\r|\n)*$| .*)/; print;
}; maybe_unlink($previous);'
perl -e '
my $new_expect_file=".github/actions/spell-check/expect.txt";
use File::Path qw(make_path);
use File::Basename qw(dirname);
make_path (dirname($new_expect_file));
open FILE, q{<}, $new_expect_file; chomp(my @words = <FILE>); close FILE;
my @add=qw('"$patch_add"');
my %items; @items{@words} = @words x (1); @items{@add} = @add x (1);
@words = sort {lc($a)."-".$a cmp lc($b)."-".$b} keys %items;
open FILE, q{>}, $new_expect_file; for my $word (@words) { print FILE "$word\n" if $word =~ /\w/; };
close FILE;
system("git", "add", $new_expect_file);
'
}

comment_json=$(mktemp)
curl -L -s -S \
  --header "Content-Type: application/json" \
  "https://api.github.com/repos/microsoft/PowerToys/issues/comments/847388337" > "$comment_json"
comment_body=$(mktemp)
jq -r .body < "$comment_json" > $comment_body
rm $comment_json

patch_remove=$(perl -ne 'next unless s{^</summary>(.*)</details>$}{$1}; print' < "$comment_body")
  

patch_add=$(perl -e '$/=undef;
$_=<>;
s{<details>.*}{}s;
s{^#.*}{};
s{\n##.*}{};
s{(?:^|\n)\s*\*}{}g;
s{\s+}{ }g;
print' < "$comment_body")
  
update_files
rm $comment_body
git add -u
If you see a bunch of garbage

If it relates to a ...

well-formed pattern

See if there's a pattern that would match it.

If not, try writing one and adding it to the patterns.txt file.

Patterns are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your lines.

Note that patterns can't match multiline strings.

binary-ish string

Please add a file path to the excludes.txt file instead of just accepting the garbage.

File paths are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your files.

^ refers to the file's path from the root of the repository, so ^README\.md$ would exclude README.md (on whichever branch you're using).

@jsoref
Copy link
Contributor

jsoref commented May 24, 2021

@enricogior add a .github/actions/spell-check/patterns.txt entry like:

# TestCase strings intentionally have non dictionary items
\[TestCase\(new string.*\]

per #11406 (comment) and the spell-checker should stop complaining.

@mykhailopylyp
Copy link
Contributor

We might want to consider adding telemetry so we know how often it is used. Probably it should be added to the PowerToys Run infrastructure code.

@htcfreek
Copy link
Collaborator

htcfreek commented May 25, 2021

@ThiefZero
Can your plugin convert 12 hour fornat to 24 hour format? If not I can open a feature request.

@htcfreek
Copy link
Collaborator

Does anyone have an idea how we can show the available units best?

Mayba a list on docs.microsoft and a hint "(See available units in our docs.)" in the description.

@ThiefZero
Copy link
Contributor

@ThiefZero
Can your plugin convert 12 hour fornat to 24 hour format? If not I can open a feature request.

It currently does not. I took a quick look around in UnitsNet and couldn't find it there either.

Does anyone have an idea how we can show the available units best?

Mayba a list on docs.microsoft and a hint "(See available units in our docs.)" in the description.

Do you mean in the Settings's plugin list? I see that Windows Search has an additional options segment.
image
It would probably be possible to create a custom list there that would also allow users to enable/disable certain units. Not sure it would look the prettiest, as the list showing only the default enabled would be quite large already (13 entries).

@mykhailopylyp
Copy link
Contributor

@ThiefZero

At this stage, am I supposed to apply the requested changes?

If you want to make changes to this branch create a pr into this branch.

@mykhailopylyp mykhailopylyp force-pushed the user/ThiefZero/unitConversion branch from abd45f7 to 774a2fa Compare June 3, 2021 11:27
@mykhailopylyp
Copy link
Contributor

@ThiefZero
To merge this one we need to implement localization. Are you going to do it or should we?
Take a look at https://github.com/microsoft/PowerToys/blob/master/doc/devdocs/localization.md to know how to do it. Also, you can take a look at the existing plugins.

@ThiefZero
Copy link
Contributor

@ThiefZero
To merge this one we need to implement localization. Are you going to do it or should we?
Take a look at https://github.com/microsoft/PowerToys/blob/master/doc/devdocs/localization.md to know how to do it. Also, you can take a look at the existing plugins.

I'll give it a go 😎

@ThiefZero
Copy link
Contributor

#11617

I read through the doc and added LocProject.json. Other stuff that the doc described I think are already in there (<EmbeddedResource Include="Properties\Resources.*.resx" />) and an entry in pipeline.user.windows.yml.
About that pipeline file, in my original PR, I was missing a reference to UnitsNet.dll in Product.wxs. Looking at the pipeline file, I noticed that the Calculator plugin also has Wox .dlls listed there. Should there be entries for

modules\launcher\Plugins\Community.PowerToys.Run.Plugin.UnitConverter\Wox.Infrastructure.dll
modules\launcher\Plugins\Community.PowerToys.Run.Plugin.UnitConverter\Wox.Plugin.dll
and possibly
modules\launcher\Plugins\Community.PowerToys.Run.Plugin.UnitConverter\UnitsNet.dll

added here as well?

@htcfreek
Copy link
Collaborator

htcfreek commented Jun 5, 2021

@ThiefZero
Do you translate only the results or the input too?

@ThiefZero
Copy link
Contributor

@ThiefZero
Do you translate only the results or the input too?

image
This is what's supported right now. So basically neither result/input.

@mykhailopylyp
Copy link
Contributor

@crutkas
Input queries are not localized. That is a user has to use English abbreviations to get a result. It doesn't seem trivial to make such localization. Is it something that blocks this PR?

@mykhailopylyp
Copy link
Contributor

@ThiefZero
Good catch! Only modules\launcher\Plugins\Community.PowerToys.Run.Plugin.UnitConverter\Wox.Infrastructure.dll and
modules\launcher\Plugins\Community.PowerToys.Run.Plugin.UnitConverter\Wox.Plugin.dll should be added to the pipeline.

Copy link
Contributor

@mykhailopylyp mykhailopylyp left a comment

Choose a reason for hiding this comment

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

LGTM! Tested works as expected.
@ThiefZero great job!

@mykhailopylyp mykhailopylyp merged commit 1dabd76 into master Jun 8, 2021
@mykhailopylyp mykhailopylyp deleted the user/ThiefZero/unitConversion branch June 8, 2021 15:53
@ThiefZero
Copy link
Contributor

ThiefZero commented Jun 8, 2021

LGTM! Tested works as expected.
@ThiefZero great job!

@mykhailopylyp

Thanks, and thank you for the help refactoring/cleaning things up. 😄

I've got a question though, I was looking forward to see my contribution stats and I checked the Insights page for them, but there's no entry at all under my name (#lines/commits etc.). Did I do something wrong? I figured it should be in there now that it's (finally) been merged to master 🤔

It's my first open source code contribution so let me know if I messed up somewhere

@mykhailopylyp
Copy link
Contributor

@ThiefZero
Thank you for raising this question. Probably, it happened because this PR was created by @enricogior. We will try to figure it out and fix it.

@mykhailopylyp mykhailopylyp restored the user/ThiefZero/unitConversion branch June 9, 2021 09:34
jaimecbernardo added a commit that referenced this pull request Jun 9, 2021
This reverts commit 1dabd76,
to give proper attribution to the author.
jaimecbernardo added a commit that referenced this pull request Jun 9, 2021
This reverts commit 1dabd76,
to give proper attribution to the author.
@jaimecbernardo
Copy link
Collaborator

Hi @ThiefZero ,
Sorry for that confusion. We've reverted and then re-added the plugin code in order to give you proper attribution.
Thanks a lot for the contribution!

@ThiefZero
Copy link
Contributor

ThiefZero commented Jun 9, 2021

@mykhailopylyp
@jaimecbernardo

Thank you for the effort and quick response. I can see myself in the list now, much love! 🥳

@crutkas crutkas deleted the user/ThiefZero/unitConversion branch June 15, 2021 18:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Run-Plugin Things that relate with PowerToys Run's plugin interface
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants