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

[Settings][Plugin manager] Activation commands: Fix conflict with calc plugin and add warning #19593

Merged
merged 19 commits into from
Aug 23, 2022

Conversation

htcfreek
Copy link
Collaborator

@htcfreek htcfreek commented Jul 23, 2022

Summary of the Pull Request

The old activation command for Time and Date plugin is problematic and brake some global queries for calculator plugin. This PR updates the default activation command.

Plugin old new
Time and Date ( )

Additionally this PR removes the existing NotAllowedKeyword warning and replaces it with a generic information bar above the plugin list. The bar contains a link to the "docs.msft" page where we describe the problem more detailed including a list of problematic keywords.

image

PR Checklist

Detailed Description of the Pull Request / Additional comments

Additionally this PR adds a replacement for the InfoBadge control that isn't available currently.

image
image

Validation Steps Performed

Test on local build and run unit tests successfully.

@github-actions

This comment has been minimized.

@htcfreek htcfreek marked this pull request as ready for review July 23, 2022 17:32
@github-actions

This comment has been minimized.

@htcfreek
Copy link
Collaborator Author

PR is ready.

@crutkas
Copy link
Member

crutkas commented Jul 23, 2022

I do not want the program plugin in this PR per the issue

@htcfreek

This comment was marked as outdated.

@htcfreek
Copy link
Collaborator Author

I do not want the program plugin in this PR per the issue

I have reverted the changes for program plugin (and did some small tweaks.)

@Jay-o-Way
Copy link
Collaborator

I think we can do this as we currently only get issues because of ( character.

Actually, I found one: #9473

@htcfreek
Copy link
Collaborator Author

I think we can do this as we currently only get issues because of ( character.

Actually, I found one: #9473

Sure. But let's have this in a second issue. The dot warning can be added easily and because I can't work on this PR the next weeks this should not block merging. If we enable the warning for dot then we have the warning per default on for program plugin. I don't think this is a good idea.

Copy link
Collaborator

@davidegiacometti davidegiacometti left a comment

Choose a reason for hiding this comment

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

I would prefer a simpler solution with a generic warning that alert user that some activation commands may limits some plugins.
At the moment ~, /, \ affects Program plugin.

This will be kinda hard to be mantained in future with new plugins.

I am fine with change default activation command for TimeDate.

@htcfreek
Copy link
Collaborator Author

I would prefer a simpler solution with a generic warning that alert user that some activation commands may limits some plugins.
At the moment ~, /, \ affects Program plugin.

This will be kinda hard to be mantained in future with new plugins.

I am fine with change default activation command for TimeDate.

/ and ~ are covered by NotAllowed check. And being more precise is more helpful for our users.

@davidegiacometti
Copy link
Collaborator

Not allowed means

I would prefer a simpler solution with a generic warning that alert user that some activation commands may limits some plugins.
At the moment ~, /, \ affects Program plugin.
This will be kinda hard to be mantained in future with new plugins.
I am fine with change default activation command for TimeDate.

/ and ~ are covered by NotAllowed check. And being more precise is more helpful for our users.

Does it means that I can't set these? I think we shouldn't block anything.

@htcfreek
Copy link
Collaborator Author

htcfreek commented Jul 24, 2022

Not allowed means

I would prefer a simpler solution with a generic warning that alert user that some activation commands may limits some plugins.
At the moment ~, /, \ affects Program plugin.
This will be kinda hard to be mantained in future with new plugins.
I am fine with change default activation command for TimeDate.

/ and ~ are covered by NotAllowed check. And being more precise is more helpful for our users.

Does it means that I can't set these? I think we shouldn't block anything.

Same thoughts. It means only that the codeparts are named NotAllowedKeyword. But the error message is different and says it is used in another plugin to. (We try to update the message in #19592.)

I think we should combine both to a warning message that says the following: This activation command can break other plugins on global queries (e.g. Calculator, Folder, Program). (@davidegiacometti, if you agree here please request this a change. I can't work on this the next two weeks.)

@stefansjfw
Copy link
Collaborator

I would prefer a simpler solution with a generic warning that alert user that some activation commands may limits some plugins. At the moment ~, /, \ affects Program plugin.

This will be kinda hard to be mantained in future with new plugins.

I am fine with change default activation command for TimeDate.

I kinda agree with this. We shouldn't add warnings per plugin as it'll make a mess in the future

@davidegiacometti
Copy link
Collaborator

A solution could be having each plug-in to provide a list of keywords that may conflict but it sounds a bit overkill.

A generic warning and/or user docs should be fine.

And of course don't ship new plug-ins with those keywords 😃

@jaimecbernardo
Copy link
Collaborator

Restarted it ;)

@davidegiacometti
Copy link
Collaborator

Done a few tests. It's working but looks a bit overkill to me.
Note that warning is still displayed with Folder and Caldulator plugins disabled and I don't like a solution that depends on plugins.

image

I would go just for these but I am up to the core team (#19593 (comment)):

  • Add one generic warning. User docs only should be fine too.
  • Don't ship new plug-ins with those keywords.

@htcfreek
Copy link
Collaborator Author

htcfreek commented Aug 10, 2022

Done a few tests. It's working but looks a bit overkill to me.
Note that warning is still displayed with Folder and Caldulator plugins disabled and I don't like a solution that depends on plugins.

image

I would go just for these but I am up to the core team (#19593 (comment)):

  • Add one generic warning. User docs only should be fine too.
  • Don't ship new plug-ins with those keywords.

We could add a centralized warning above the plugin list saying "Some direct activation commands may break global queries for other plugins." with a link to docs.msft. (We can still keep the badges on the plugin header or move them to the aktionkey settings row.)

Second alternative would be to have a generic (always visible) information bar above the plugin list linking to docs.msft and saying "There are some phrases that can break global queries of some plugins if you use them as activation command.".

@jaimecbernardo
What do you think? It was your idea with the warnig.

@crutkas, what is your preference on this as core team leader?

@htcfreek
Copy link
Collaborator Author

@jaimecbernardo
I am waiting for your answer on the question in my last comment. Can you please answer that I can continue work here.

@jaimecbernardo
Copy link
Collaborator

Hi @htcfreek,
Thanks for the ping and sorry for the delay.
I prefer the

Second alternative would be to have a generic (always visible) information bar above the plugin list linking to docs.msft and saying "There are some phrases that can break global queries of some plugins if you use them as activation command.".

I imagine people still trying it, though, but if it's only a warning above the list when they do it they won't read it.
Thanks for the thoughtful suggestions.

@htcfreek
Copy link
Collaborator Author

Hi @htcfreek,
Thanks for the ping and sorry for the delay.
I prefer the

Second alternative would be to have a generic (always visible) information bar above the plugin list linking to docs.msft and saying "There are some phrases that can break global queries of some plugins if you use them as activation command.".

I imagine people still trying it, though, but if it's only a warning above the list when they do it they won't read it.
Thanks for the thoughtful suggestions.

Should we use warning or information style? Can I add a section header to a aka link or de we need a new one?

@jaimecbernardo
Copy link
Collaborator

Hi @htcfreek,
Thanks for the ping and sorry for the delay.
I prefer the

Second alternative would be to have a generic (always visible) information bar above the plugin list linking to docs.msft and saying "There are some phrases that can break global queries of some plugins if you use them as activation command.".

I imagine people still trying it, though, but if it's only a warning above the list when they do it they won't read it.
Thanks for the thoughtful suggestions.

Should we use warning or information style? Can I add a section header to a aka link or de we need a new one?

I'm guessing informational one. Didn't understand the header question.

@htcfreek
Copy link
Collaborator Author

Should we use warning or information style? Can I add a section header to a aka link or de we need a new one?

I'm guessing informational one.

okay

Didn't understand the header question.

The question was if a link like https://aka.ms/PowerToysOverview_PowerToysRun#direct-activation-commands is working. I tested it and yes it is working.

@crutkas
Copy link
Member

crutkas commented Aug 19, 2022

Can someone help boil down what the POR here is? I looked at the issue and seemed like the solution proposed was to change the keyword.

I look at the screenshot and the issue is it doesn’t explain in the error how to help / what exactly is conflicting.

I do like maybe an error box at the top that could consolidate the conflicts.

@crutkas
Copy link
Member

crutkas commented Aug 19, 2022

I will say out of the box is shipping with conflicts feels odd :)

@htcfreek
Copy link
Collaborator Author

htcfreek commented Aug 19, 2022

Can someone help boil down what the POR here is? I looked at the issue and seemed like the solution proposed was to change the keyword.
I look at the screenshot and the issue is it doesn’t explain in the error how to help / what exactly is conflicting.

I do like maybe an error box at the top that could consolidate the conflicts.

The initial solution was to change the keyword. And additional @jaimecbernardo asked for an error message and I implemented it.

Because the current implemention might be to anoying we decide to have a generic always visible infobox hinting about the "possible conflict thing" and containing a "learn more" link to docs.microsoft. (New infobox is not done yet.) The generic infobox like we have on explorer addon page has the advantage to have less code and new conflicts can simply been listed on the docs page.

An automatic visible one (based on input) is more complex and in theory we then have to link keywords with plugins to check if the conflict plugin is enabled or not. This feels a bit to much here as @davidegiacometti mentioned.

I hope you are up to date now and everything is clear now 😉

@htcfreek
Copy link
Collaborator Author

I will say out of the box is shipping with conflicts feels odd :)

Agree. There are two possible solutions:

  • Changing program plugin keyword too. What you like to have in another pr.
  • Don't list the dot as conflict.

What do you prefer?

@crutkas
Copy link
Member

crutkas commented Aug 19, 2022

@htcfreek, I don't want to change the program plugin activation word as this time. I think list it as a conflict.

what i currently don't want to affect primary usage item for edge cases.

IsOpen="{x:Bind Mode=OneWay, Path=ViewModel.EnablePowerLauncher}"
IsTabStop="{x:Bind Mode=OneWay, Path=ViewModel.EnablePowerLauncher}">
<muxc:InfoBar.ActionButton>
<HyperlinkButton NavigateUri="https://aka.ms/PowerToysOverview_PowerToysRun#direct-activation-commands"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@crutkas
New aka-Link to not have coded section header reference?

@htcfreek htcfreek marked this pull request as ready for review August 21, 2022 09:56
@htcfreek
Copy link
Collaborator Author

@davidegiacometti, @jaimecbernardo, @crutkas, @stefansjfw
Changes are done. Please review. The docs update PR is MicrosoftDocs/windows-dev-docs#3957.

@htcfreek
Copy link
Collaborator Author

@jaimecbernardo
Can this get in for this month's release?

@jaimecbernardo
Copy link
Collaborator

Sorry for the delay and thanks for pinging 😄
Looked into just now.

Copy link
Collaborator

@jaimecbernardo jaimecbernardo left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you for the contribution!
I think it's better now 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Idea-Enhancement New feature or request on an existing product Product-Settings The standalone PowerToys Settings application Run-Plugin Manager Issues with the PowerToys Run plugin manager 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