Pt run history plugin 19349

[jefflord]

Summary of the Pull Request

Create a system to access and use history in PT Run (for now just the history of selected items)

PR Checklist

• Closes: PowerToys Run, history #19349
• 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

Testing to see how well this can work as a plugin.

Validation Steps Performed

[htcfreek]

@jefflord
Can you please request a review from me.

[jefflord]

Can you please request a review from me.

Is there a formal way to do this? Or just "hey @htcfreek please review this!"

[htcfreek]

Can you please request a review from me.

Is there a formal way to do this? Or just "hey @htcfreek please review this!"

There is a menu on the github website on the right side of the pr description.

[jefflord]

I am sorry, I still don't get all the github stuff. I am used to DevOps...

There are no reviewers in the list to request a review from:

I think the owners of the repo have to assign reviews first? Please tell me what I am missing. Thanks.

[htcfreek]

I am sorry, I still don't get all the github stuff. I am used to DevOps...

There are no reviewers in the list to request a review from:

I think the owners of the repo have to assign reviews first? Please tell me what I am missing. Thanks.

I think you don't have permission.

[jefflord]

I think you don't have permission.

Right, this is the mystery. I can just "at" someone that I know is working the PT Run area to see if they can help? @jaimecbernardo, can you help out?

[jaimecbernardo]

Sorry. Adding some reviewers in here

[htcfreek]

@jefflord
Something is wrong. The output dir (...\plugins\history) for "Debug x64" is empty on my machine. I can't build the plugin. Building the plugin directly compiles the Powertys.Run binaries into the plugin folder.

[jefflord]

@htcfreek

This is the first plugin I have created and so maybe someone else could help trouble shoot a build issue.

Though, I can say if the output dir is empty it must not be building, right?

Try to get more information by changing the build settings in VS:

Can you try doing a full clean and full rebuild on the entire solution?

I will review the "new-plugin-checklist.md" and up you when there is new code to test, I might have missed something.

To the other reviewers: @stefansjfw and @jaimecbernardo,
I saw the "new-plugin-checklist.md" a bit late, crap... Should I rename the plugin to "Community.PowerToys.Run.Plugin.History" (it's current Microsoft.PowerToys.Run.Plugin.History)

[htcfreek]

Though, I can say if the output dir is empty it must not be building, right?

Not sure. Based on log the project is build and the dll is generated. Maybe something with the build output path?!

Can you try doing a full clean and full rebuild on the entire solution?

Good idea. Will try later or tomorrow. Maybe it's because I select Launcher for debugging. (But in general this should work too.)

To the other reviewers: @stefansjfw and @jaimecbernardo,
I saw the "new-plugin-checklist.md" a bit late, crap... Should I rename the plugin to "Community.PowerToys.Run.Plugin.History" (it's current Microsoft.PowerToys.Run.Plugin.History)

For me this feels like a core feature. So Microsoft should be okay.

[htcfreek]

First of all I was able to build it in a full and clean debug build of the whole solution. And the functionality of the plugin is great. This will be a big improvement.

There are a few things we should fix/improve:

1. See the code review.
2. The history entries for shell plugin needs a restart of PT Run to be available.
3. We need a more clear delete button image. The current one conflicts with WW plugin. The new image should contain the history symbol and a delete symbol. (Btw. WW plugin uses delete icon for kill process and remove icon for close window.)
   
4. The tool tip of the delete button shouldn't contain the title as this can be to long. Please keep it short and simple.

Regarding the build process:

I saw that you referenced the PowerLauncher project as include in your plugin project. Maybe this is the cause of all the problems:

• Plugin won't build/is ignored, if I build PowerLauncher project for direct debugging.
• On full solution build we have a second PowerLauncher.exe and other project files in the plugin folder were it shouldn't be.
  @stefansjfw or @jaimecbernardo can you help here?

Additional idea:

A nice thing could be if we show the history on opening PT Run if ShowLastQuery option is off and/or the search input is empty. This can be an optional setting.

[jefflord]

@htcfreek wow, fantastic review. I will be digging in soon!

Thank you!

[htcfreek]

@jefflord
I didn't test all plugins for occurrence in history. Only a few one. I think this is something you can do by yourself.

[jefflord]

@htcfreek

This is a bummer, I cannot build anymore. I did update VS to 17.3.

As an example issue, getting just an updated branch main, I cannot build "ApplicationUpdate".

Error (active)	E0135	namespace "nonstd" has no member "make_unexpected"
ApplicationUpdate	...\src\common\updating\updating.cpp	75	

Error	C2039	'make_unexpected': is not a member of 'nonstd'	
ApplicationUpdate	...\src\common\updating\updating.cpp	75	

Error	C3861	'make_unexpected': identifier not found	
ApplicationUpdate	...\src\common\updating\updating.cpp	75	

Any clue on this?

UPDATE: Reverted back to VS 17.2.7 and the build is OK again. hmmmmmm

[jefflord]

@htcfreek thanks again for the awesome review.

I made all the code changes suggested. With regard to the other comments:

The history entries for shell plugin needs a restart of PT Run to be available.

Fixed and should work better for other plugins.

We need a more clear delete button image

How's this:

I think we can only pick from these? https://docs.microsoft.com/en-us/windows/apps/design/style/segoe-ui-symbol-font

The tool tip of the delete button shouldn't contain the title as this can be to long. Please keep it short and simple.

True, done

I saw that you referenced the PowerLauncher...

Yes, because I need access to PluginManager.AllPlugins and PluginManager.API. I really hated doing this. I know this seems like a bad call since it's almost circular reference, and I see that it could be weird for the build process regarding build order. But what can be done?

1. Move PluginManager out of PowerLauncher then add a reference to that in PowerLauncher and and History...
2. Get real sneaky and use reflection to access what we need in PluginManager?
3. ???

What I will/did change this reference to NOT copy local, so we don't get a copy of the PowerLauncher assembly in the History output folder.

[jefflord]

@htcfreek

I think these take care of all the rest, some you have seen.

doc/devdocs/modules/launcher/plugins/history.md
#19917
https://github.com/jefflord/PowerToys/tree/patch-1

.pipelines/ESRPSigning_core.json
#19919
https://github.com/jefflord/PowerToys/tree/patch-2

docs/hub/powertoys/run.md
MicrosoftDocs/windows-dev-docs#3998
https://github.com/jefflord/windows-uwp/tree/patch-1

This new-plugin process is "a bit" involved, but I guess it does not happen a lot and so that's OK.

Please let me know what I missed (I know you will).

[htcfreek]

@jefflord
You can request the final review from @jaimecbernardo and @stefansjfw to get this finally approved. I think it's ready. 👍🏻🚀

[jefflord]

@htcfreek Soooo much thanks, wish me luck!

[jefflord]

@jaimecbernardo and @stefansjfw, please review at your convenience.

Thanks

[htcfreek]

@htcfreek Soooo much thanks, wish me luck!

You don't need luck because you did a very great job. 😉

[jefflord]

@stefansjfw, the extra PRs are closed. I think everything we need is in this single PR now. Thanks.

[stefansjfw]

I saw there is a discussion about this already. just want to note this to see if there is a way to go around it

[stefansjfw]

This is now included by default in all csprojects. You should rebase to the latest main.

[stefansjfw]

This is now included by default in all csprojects

[stefansjfw]

I did a brief first review. Let's rebase on top of master and remove packages that are now included by default

[stefansjfw]

Overall, I like this plugin. It works pretty well. Nice work! I'm puzzled about this dependency, but I understand that there is no simpler way to do this. Talking about future work now... Maybe extracting Plugin manager would be the way to go. Not sure atm.

[jefflord]

@stefansjfw thanks, I think it's a neat plugin that poses a weird dependency question, but I think it's worth the effort.

I did a brief first review. Let's rebase on top of master and remove packages that are now included by default

Well, I don't know for sure what that means. I assume the goal is to get a version of the PR based on a newer version of main. Bit of git newb here. Help?

I can update my main (sync my fork), then rebase this branch onto my main, and address conflicts. But then I have only updated my main. If this is what you mean, how to I go on from there with the updated main branch in my repo?

I can also merge an updated main into my branch at jefflord:PTRun-History-Plugin-19349.

Thanks in advance

[stefansjfw]

@stefansjfw thanks, I think it's a neat plugin that poses a weird dependency question, but I think it's worth the effort.

I did a brief first review. Let's rebase on top of master and remove packages that are now included by default

Well, I don't know for sure what that means. I assume the goal is to get a version of the PR based on a newer version of main. Bit of git newb here. Help?

I can update my main (sync my fork), then rebase this branch onto my main, and address conflicts. But then I have only updated my main. If this is what you mean, how to I go on from there with the updated main branch in my repo?

I can also merge an updated main into my branch at jefflord:PTRun-History-Plugin-19349.

Thanks in advance

git remote -vv should show you all remotes you've added for PowerToys project locally. If there is no microsoft/PowerToys remote there you can add it with
git remote add msft https://github.com/microsoft/PowerToys.git

Then, just to make sure you have the latest code do:
git pull --all

And then git merge msft/main should merge the latest msft/main locally with your PTRun-History-Plugin-19349 branch. Some minor conflicts will be there but should be easy to resolve.

[jefflord]

@stefansjfw OK, I was confused by the "rebase " suggestion here (#19569 (comment)).

I just need to "merge an updated main into my branch at jefflord:PTRun-History-Plugin-19349."

I usually do this by updating my/main with with msft/main via github "sync fork", but I like this option of directly adding msft as a remote to my local repo.

Also, I am SOOO glad you asked for this to be done because there was a merge conflict and it was in a place of code I wrote for another PR (#19215) that was recently (7 days ago) already merged into main!

[jefflord]

@jaimecbernardo FYI: I didn't mean to remove any reviewer(s) from this. This happened inadvertently.

[htcfreek]

@jaimecbernardo
Wat is needed to get this in for this month's release?

[stefansjfw]

@jaimecbernardo Wat is needed to get this in for this month's release?

FYI, I'm fine with the code in general. The only thing to be discussed further here is the already mentioned dependency. I guess it can go in for now as it is and later we can figure out whether there is a better way to do this.

[jefflord]

@jaimecbernardo Wat is needed to get this in for this month's release?

FYI, I'm fine with the code in general. The only thing to be discussed further here is the already mentioned dependency. I guess it can go in for now as it is and later we can figure out whether there is a better way to do this.

Sweet, I agree, ship now and make changes as needed. There might be bug/enhancements that cause us to refactor sooner than later but we won't know until we ship it.

[jaimecbernardo]

Sorry for the delay and thanks for the ping.
This is awesome! It seems to work so seamlessly!
Thanks a lot for this contribution! 😃

[jaimecbernardo]

LGTM! Thank you for the contribution! Regarding the dependency cycle, a new issue can be created after this PR is merged to refactor what's needed to a lib dependency.

[jefflord]

Fantastic thanks a bunch to @jaimecbernardo and a special thanks to @htcfreek!

[jefflord]

@stefansjfw, @jaimecbernardo, @htcfreek

Shower thoughts:

Re: The "issue" where this plugin takes a dependency on PowerLauncher. I think all four of us were a little put off by this at first.

But it just dawned on me. In a "true" plugin model, it's absolutely normal for the plugin to take a dependency on the plugin-host. And it's actually more rare for the plugin-host to take a dependency on the plugins.

But maybe for PT Run, plugins are just the name for what's really "modules", since external plugins are not supported anyway, right?

TLDR:
In any case, in light of this further consideration I don't feel the need to do anything about this plugins dependency situation unless changing it actually provides some real benefit discovered in the future.