-
Notifications
You must be signed in to change notification settings - Fork 6.8k
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 paste actions to allow transcoding of media files #37188
base: main
Are you sure you want to change the base?
Conversation
…/advanced-paste-semantic-kernel
…/advanced-paste-semantic-kernel
…/advanced-paste-semantic-kernel
…vanced-paste-transcoding
…vanced-paste-transcoding
…vanced-paste-transcoding
This is going to be so 🔥 |
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.
Merged the latest main into this branch so we can see the CI kick off properly, but it LGTM! Worked perfectly with transcoding a few mp4 videos, a few AVI files, transcoding a video to mp3, converting files on my QNAP, and didn't crash when I tried to feed it something invalid. Awesome job @drawbyperpetual!
Everything else looks great! This is a fantastic PR—really appreciate your contribution. ❤️ |
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.
Copilot reviewed 24 out of 39 changed files in this pull request and generated 1 comment.
Files not reviewed (15)
- src/modules/AdvancedPaste/AdvancedPaste/AdvancedPasteXAML/Controls/AnimatedContentControl/AnimatedContentControl.xaml: Language not supported
- src/modules/AdvancedPaste/AdvancedPaste/AdvancedPasteXAML/Controls/PromptBox.xaml: Language not supported
- src/modules/AdvancedPaste/AdvancedPaste.UnitTests/Mocks/NoOpKernelQueryCacheService.cs: Evaluated as low risk
- src/modules/AdvancedPaste/AdvancedPaste/Helpers/UserSettings.cs: Evaluated as low risk
- src/modules/AdvancedPaste/AdvancedPaste.UnitTests/ServicesTests/AIServiceBatchIntegrationTests.cs: Evaluated as low risk
- src/modules/AdvancedPaste/AdvancedPaste/Helpers/TransformHelpers.cs: Evaluated as low risk
- src/modules/AdvancedPaste/AdvancedPaste/AdvancedPasteXAML/Controls/PromptBox.xaml.cs: Evaluated as low risk
- src/modules/AdvancedPaste/AdvancedPaste.UnitTests/ServicesTests/KernelServiceIntegrationTests.cs: Evaluated as low risk
- src/modules/AdvancedPaste/AdvancedPaste/Helpers/OcrHelpers.cs: Evaluated as low risk
- src/modules/AdvancedPaste/AdvancedPaste/Helpers/KernelExtensions.cs: Evaluated as low risk
- src/modules/AdvancedPaste/AdvancedPaste/AdvancedPasteXAML/MainWindow.xaml.cs: Evaluated as low risk
- src/modules/AdvancedPaste/AdvancedPaste/Models/ClipboardFormat.cs: Evaluated as low risk
- src/modules/AdvancedPaste/AdvancedPaste/Models/PasteActionError.cs: Evaluated as low risk
- src/modules/AdvancedPaste/AdvancedPaste/Models/PasteFormats.cs: Evaluated as low risk
- src/modules/AdvancedPaste/AdvancedPaste/Services/ICustomTextTransformService.cs: Evaluated as low risk
Comments suppressed due to low confidence (2)
src/modules/AdvancedPaste/AdvancedPaste/Helpers/DataPackageHelpers.cs:53
- The array passed to File.WriteAllBytesAsync is empty, which will result in an empty file being created. This may not be the intended behavior.
await File.WriteAllBytesAsync(outputFilePath, [], cancellationToken); // TranscodeAsync seems to require the output file to exist
src/modules/AdvancedPaste/AdvancedPaste/Helpers/DataPackageHelpers.cs:239
- [nitpick] The variable name extension is ambiguous. It should be renamed to fileExtension for clarity.
where extension.StartsWith('.')
await File.WriteAllBytesAsync(outputFilePath, [], cancellationToken); // TranscodeAsync seems to require the output file to exist | ||
|
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.
Replace empty array [] with a valid byte array, e.g., new byte[0].
await File.WriteAllBytesAsync(outputFilePath, [], cancellationToken); // TranscodeAsync seems to require the output file to exist | |
await File.WriteAllBytesAsync(outputFilePath, new byte[0], cancellationToken); // TranscodeAsync seems to require the output file to exist |
Copilot is powered by AI, so mistakes are possible. Review output carefully before use.
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.
LGTM
|
||
if (!prepareOp.CanTranscode) | ||
{ | ||
throw new InvalidOperationException($"Error transcoding; {nameof(prepareOp.FailureReason)}={prepareOp.FailureReason}"); |
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.
} | ||
|
||
private static async Task TranscodeMediaAsync(StorageFile inputFile, StorageFile outputFile, MediaEncodingProfile outputProfile, CancellationToken cancellationToken, IProgress<double> progress) | ||
{ |
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.
Can we check if the media is already in the target format before starting transcoding to avoid redundant processing?
Summary of the Pull Request
See linked issue for more information.
PR Checklist
Detailed Description of the Pull Request / Additional comments
A
CancellationToken
is now propagated to all leaf paste actions to allow for cancellation, as is anIProgress<double>
for progress reporting.Validation Steps Performed