-
Notifications
You must be signed in to change notification settings - Fork 6.6k
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
[Advanced Paste] Add Semantic Kernel opt-in to allow chaining of paste actions #35902
base: main
Are you sure you want to change the base?
Conversation
…/advanced-paste-semantic-kernel
…/advanced-paste-semantic-kernel
…/advanced-paste-semantic-kernel
Two cents on this:
|
@htcfreek Regarding the expander for Paste with AI, I think it isn't necessary - I think we'd want to highlight the new features. It's also consistent without the expander - see Mouse Utilities for example. Having said that, you've brought up the important point that the "Custom Preview" toggle being present is tied to AI being enabled. But in this PR, I've made it also work for Image to Text. So we should untie it from AI! I'll do that.. Regarding the policy, is there any benefit that we would get by adding one given that we already one for online AI models? |
Two maybe benefits I am not sure about:
But I am not sure if these are things that require a policy. 🤔 |
Then we should move the toggle into tge behavior settings section. |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
In terms of GPO for the feature, I'm thinking the open AI already has controls to limit credit usage / model usage per token, in case they're using a work API key 🤔 A new GPO seems not necessary at this point 😄 |
@htcfreek I've moved the Custom Preview toggle into the Behavior section as you suggested. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code LGTM.
Testing looks pretty good too. Some errors could be more explicit, like when an image can't be converted through text, but I don't think that's blocking.
Still checking on some verifications, so I'm not approving yet, but will approve after verifications are done. Good work!
Summary of the Pull Request
See linked issue for more information.
PR Checklist
Detailed Description of the Pull Request / Additional comments
There's quite a lot of refactoring and code-quality improvements here. A non-exhaustive list:
DataPackage
) rather than on the system clipboard. The system clipboard is only read once per operation and written to only just before pasting (if needed). There are multiple reasons for this:Async
within AdvancedPaste (this was also done as part of Additional Actions but there's much more of it now).CanPreview
property (and Image to Text has been marked as having it to allow it to be previewed)lastQuery.json
functionality is removedIFileSystem
is used within Advanced Paste to improve testability.Validation Steps Performed