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

[Run] String-type settings support for PowerToys Run plugins #25326

Closed
wants to merge 1 commit into from

Conversation

Simizfo
Copy link

@Simizfo Simizfo commented Apr 8, 2023

Summary of the Pull Request

Hi there! This is Simone. I've posted a few weeks ago a Tweet where I've shown ChatGPT integration into PowerToys Run, implemented as a custom plugin. Clint Rutkas reached out to me to ask if I wanted to make this plugin a reality and I agreed.

We've exchanged a few mails and had a brief call two weeks ago and we discovered a few points that need development before the plugin can become a reality for everyone:

  • There is a need for String settings on PT Run plugins. Currently PT Run plugins can only have Boolean-types settings, and my plugin needs the user to enter a personal OpenAI API key to work. (This is what this PR is about)
  • A custom result layout can be made to break out of the limits of the single text-line result that Run can display.

PR Checklist

  • Closes: None, apparently (should I open an issue for this?)
  • Communication: I've discussed this with core contributors already. Clint, as mentioned before!
  • Tests: None yet
  • Localization: None yet
  • Dev docs: None yet

Detailed Description of the Pull Request / Additional comments

I've had to take some time and as I'm now ready to work on this, here's what I've planned for this PR:

  • The string option class extends the original one to avoid breaking other plugins

  • A new custom logic will be implemented in the view:

    • if the Value type is string, a new string setting control will be created, otherwise, the classic boolean checkbox will appear

I'm open to suggestions here, of course. Work has just started. Thanks!

Validation Steps Performed

None yet. PR is in development state

…ent in PT Run Plugins - First commit to open a Draft PR
@Simizfo
Copy link
Author

Simizfo commented Apr 8, 2023

I'm totally new here, wanted to ask if I should open other Draft PRs for the other things I'm going to work on, even if the work is not started yet?

Thanks!

@htcfreek
Copy link
Collaborator

htcfreek commented Apr 8, 2023

Pinging @Picazsoo because he is working on the same.

@Simizfo
Copy link
Author

Simizfo commented Apr 11, 2023

"the same" is referred to the ChatGPT plugin or the string settings for Run plugins?

@htcfreek
Copy link
Collaborator

"the same" is referred to the ChatGPT plugin or the string settings for Run plugins?

The code for supporting string options.

@Simizfo
Copy link
Author

Simizfo commented Apr 11, 2023

Oh, that's cool! That leaves me more time to work on the plugin itself and the other requirement for it. Well, waiting for their input, thanks!

@htcfreek
Copy link
Collaborator

htcfreek commented Apr 12, 2023

@Simizfo , @Picazoo
I thought about adding drop down setting feature too. Something likke this:

String[] valueList=red,green,blue
Int selectedValue=1

And I thought on having a enum to define the settings type like this:

0=bool
1=string
2=bool+string
3=DropDown
4=bool+DropDown

Not sure if this are good ideas.

@Simizfo
Copy link
Author

Simizfo commented Apr 12, 2023

Same here. Would be the perfect thing to let the user choose the chat model (ChatGPT/Bing if an API ends up being made or 3.5-turbo/gpt4 choice for ChatGPT)

@Picazsoo
Copy link
Contributor

Picazsoo commented Apr 13, 2023

Hi @Simizfo, @htcfreek , thanks for the heads-up. As I mentioned elsewhere I have a small PoC for the String value persisting. However my scope was a bit different. From what I understand here, @Simizfo wants to simply replace the checkbox with the text input field. My attempt was to have both - check the input field to enable some feature (and also enable the text input tied to that feature) and vice versa - so one would basically persist both the checkbox value and the string value. I ran into major issues with the styling/arrangement of the elements via XAML (proper reflow of the items on really narrow screen, etc). And honestly I did not even properly subclass the original additional settings class. It was really just a PoC ready for some further refinement. Working on my previous super-small addition to PowerToys was my first exposure to C# - I spend my daily live in Java & Javascript.

I agree with the @htcfreek's comment - we should support the combination with checkbox & also other control input types such as drop-downs with predefined values, etc. Possibly also numerical inputs with spinners and min-max restrictions, but then the simple extension to strings starts to bloat quite a bit (One can even imagine requiring multiple input controls). I think the enum idea makes sense - I don't have proper CS degree (and C# best practices knowledge) though, so I cannot judge whether it is a good and extensible pattern to follow.

Basically @Simizfo if you have made some major progress and lack of this is slowing you down, then take this up. I have a lot of unpredictable things going on in my life and cannot really tell how much time it would take me to implement - might be a couple of days or weeks. And I understand that this improvement will be greatly useful to many, so it should be done sooner that latter (-:

I will be potentially first one to use this as soon as you have it implemented, since I also have a need for it elsewhere.

@htcfreek
Copy link
Collaborator

@Simizfo
What about the required ui/xaml changes?

@htcfreek
Copy link
Collaborator

[Off-topic]
The most flexible solution for combined settings might be to have a property that hides the separator line.

https://github.com/microsoft/PowerToys/blob/main/src/settings-ui/Settings.UI/Views/PowerLauncherPage.xaml#L444-L447

@htcfreek htcfreek mentioned this pull request Sep 5, 2023
11 tasks
@htcfreek
Copy link
Collaborator

htcfreek commented Sep 13, 2023

@Simizfo , @Picazsoo
The existing issue is #14267.

I plan to start my own implementation attempt if I have the time this weekend.

@Simizfo
Copy link
Author

Simizfo commented Sep 17, 2023

Well, since I needed this for my ChatGPT + Powertoys Run plugin and that was scrapped, I have moved on from this too.

I have zero work done here compared to the other 2 parts of my plugin (the plugin itself and the ability to shape the result in PT Run in custom ways). Sorry about that!

@htcfreek
Copy link
Collaborator

Well, since I needed this for my ChatGPT + Powertoys Run plugin and that was scrapped, I have moved on from this too.

so we can close this here?

@htcfreek htcfreek closed this Sep 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants