[PT-Run] Allow interchangeable use of / instead of \

[GhostVaibhav]

Summary of the Pull Request

As the title suggests, the PR adds the feature of using / instead of \ in the Registry plugin of PT Run.

PR Checklist

• Closes: [PowerToys Run] Registry Plugin: Allow interchangeable use of / instead of \ #33155
• Communication: I've discussed this with core contributors already. If work hasn't been agreed, this work might be rejected
• Tests: Added/updated and all pass
• Localization: All end user facing strings can be localized
• Dev docs: Added/updated
• New binaries: Added on the required places
  • JSON for signing for new binaries
  • WXS for installer for new binaries and localization folder
  • YML for CI pipeline for new test projects
  • YML for signed pipeline
• Documentation updated: If checked, please file a pull request on our docs repo and link it here: #xxx

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

• Manually tested by inputting registry paths with /
• Also added tests for the same

[GhostVaibhav]

Hi @htcfreek, I wrote some code. Can you review it, please? Also, I'm very new to testing, so could you point out some docs to get help regarding the same? Thanks.

[htcfreek]

Screenshot of both input formats would be great in the first place.

[GhostVaibhav]

Hi @htcfreek, I didn't quite get you. I have attached some screenshots regarding the new syntax -

Some screenshots of the old syntax -

[htcfreek]

Do we need the " in the result of screenshot 5?

[GhostVaibhav]

Do we need the " in the result of screenshot 5?

No, we don't. Will be removing in the next commit.

[GhostVaibhav]

Hi @htcfreek, I wrote some code. Can you review it, please? Also, I'm very new to testing, so could you point out some docs to get help regarding the same? Thanks.

Hi @htcfreek, any updates on this? Also, @jaimecbernardo, if you could comment on the testing part?

[jaimecbernardo]

Hi @GhostVaibhav,
Thanks for opening the PR.
Tests are in "src\modules\launcher\Plugins\Microsoft.PowerToys.Run.Plugin.Registry.UnitTest\Microsoft.PowerToys.Run.Plugin.Registry.UnitTests.csproj", which is also part of the PowerToys solution.
You can run tests from the Test > Test Explorer in Visual Studio.
Can use the other tests that are there as a basis. 😉
Hope this helps. Thank you!

[htcfreek]

Had no time to test yet. Hopefully on weekend.

[htcfreek]

There is a bug regarding the input parsing:

1. If I have a key "test" and a key "test/test" typing the "t" before the slash removes the key containing the slash from the result list and typing the whole key name clear the result list.
   

2. The result for key structuer with a key containing a slash is wrong. Looks like the splitting counter behaves wrong.


3. I can't show the contents of a key that's name contains a slash and all their subkeys:

[GhostVaibhav]

Hi @htcfreek and @jaimecbernardo, thank you both for your valuable input. However, I think I have hit a roadblock while figuring out, how to replace the / with the \. I am still trying to figure out the correct regex for that. It would be of great help if you have any inputs regarding the same. I have opened it as a question on StackOverflow (linked here). Thanks!

[GhostVaibhav]

Hi @htcfreek, I resolved all of the bugs that you pointed out while you were testing and added some tests for the same. However, the first one is not a bug from my code, but a bug in the underlying System.Registry plugin that is used to get the data for the context menu. You can test it in the latest stable version, and the result is the same.

[htcfreek]

However, the first one is not a bug from my code, but a bug in the underlying System.Registry plugin that is used to get the data for the context menu. You can test it in the latest stable version, and the result is the same.

Hi @GhostVaibhav .
We should open a issue for that in our repo to fix this in a second step. Or did you fixed it too?

[GhostVaibhav]

Hi @htcfreek, no, I didn't fix it. Also, this issue is completely separate from this PR's objectives. So, we can open a new issue and another PR for the same.

[GhostVaibhav]

Hi @htcfreek and @jaimecbernardo
The PR is complete, you can review it whenever free

[htcfreek]

Hi @htcfreek and @jaimecbernardo
The PR is complete, you can review it whenever free

@GhostVaibhav
I will not have the time before weekend.

[htcfreek]

Didn't tried the behavior on a loccal build. But had time to look at the test cases.

[htcfreek]

Missing a test with the / in a key name. Currently only value names containing a / are tested. And there are also no test cases for mixed inputs (/ and \) and having / in key and value name.

Beeing as creative as possible is great here.

[GhostVaibhav]

I've added some tests, have a look at them and let me know.

[htcfreek]

@GhostVaibhav
For my first point we already talked about creating the issue.
The point 3 is working good now and point 2 is working if you use the correct syntax.

But if you type the input without the " then the bug in point 2 with the wrong key splitting still happens.

Input: "HKEY_CURRENT_USER/test/"
- Should return: "HKEY_CURRENT_USER\test/test" (or nothing)
- Should not return: "HKEY_CURRENT_USER\test/test\new"

💡 Idea:
If we can't parse it without " then let's show an error result. The error result can tell the user that in this cas he needs " in the query. It should only be shown on action keyword search.

[GhostVaibhav]

Hi @htcfreek, are you referring to this?

[htcfreek]

Hi @htcfreek, are you referring to this?

@GhostVaibhav
Yes.

[GhostVaibhav]

Then, it's not a bug. We have a "test" key and a "test/test" key. So,

1. When writing HKEY_CURRENT_USER/SOFTWARE/test/, we refer to the first one. And
2. When writing HKEY_CURRENT_USER/SOFTWARE/"test/, we refer to the second one.

Also, we can't recommend the two results, because of the 2 totally different meanings.

[htcfreek]

Then, it's not a bug. We have a "test" key and a "test/test" key. So,

1. When writing HKEY_CURRENT_USER/SOFTWARE/test/, we refer to the first one. And
2. When writing HKEY_CURRENT_USER/SOFTWARE/"test/, we refer to the second one.

Also, we can't recommend the two results, because of the 2 totally different meanings.

1. No I only have a "test/test" key. The "test" does not exist anymore.
2. What I mean is: Without " (HKEY_CURRENT_USER/test/) the parsing is wrong and should show the "test/test" key and not the new key in the "test/test" key.

WITHOUT quotation mark

WITH quotation mark

[GhostVaibhav]

Hi @htcfreek, some things to consider.

1. The query HKCU/test/ is converted to HKCU\test\ by the query parser. The result of this query is as given below (as on the latest stable release) -

2. The query HKCU/test/ result on my modified code is as given below -

Both the above results are the same. So, that should be the expected behavior. If we want different results, we should probably open another issue regarding the same.

3. When you type HKCU/"test/, the second / is not converted to \, and therefore the query gets transformed to - HKCU\test/. So, it tries to auto-complete it.

Hope it clears some stuff out.

[htcfreek]

@GhostVaibhav
So this means we have a general logic error when key names contain a / in it's name.

Does this happen for HKCU\test/ too?

And shouldn't this get fixed by this PR? I am confused and I am sure that users going to open issues for that.

[GhostVaibhav]

Hi @htcfreek, some updates on this.

So, the key need not be fully matched to go inside it. On the latest stable release version, here's a screenshot explaining the same -

We can see in the above screenshot, that the query hkcu\software\te\ is not fully matching the key name. But, still, it matches.

Also, this shouldn't be included in this PR, because it is a separate broken functionality that needs to be fixed regardless. And, I guess we don't want to club features and fixes in the same PR. For your concern about users going to open issues for that. I will open the issues myself after this PR is closed and probably try fixing it as well.

[htcfreek]

Now that I know that the two open bug are bugs from stable branch an we open issues for it I am okay with it.

The new code looks good to me.