feat: Automatically complete the output file extension#1487
feat: Automatically complete the output file extension#1487
Conversation
Automatically append or correct the file extension when the user enters an output filename without an extension or with an unsupported extension.
|
No TODO comments were found. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e8358bc7dc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| { | ||
| string? ext = encoder.SupportExtensions().FirstOrDefault(); | ||
| if (ext != null) | ||
| DestinationFile.Value = file + ext; |
There was a problem hiding this comment.
When the destination has no extension, this branch uses file + ext directly, which produces malformed names in common edge cases (e.g. typing video. becomes video..mp4) and also fails for encoders that return extensions without a leading dot (videomp4). Using normalized extension replacement (instead of raw concatenation) avoids generating invalid output paths during filename auto-completion.
Useful? React with 👍 / 👎.
Minimum allowed line rate is |
There was a problem hiding this comment.
Pull request overview
This pull request implements automatic file extension completion in the output dialog. When a user enters a filename without an extension or with an unsupported extension, the system automatically appends or changes to the appropriate extension based on the selected encoder.
Changes:
- Added a reactive subscription that monitors both SelectedEncoder and DestinationFile changes
- Automatically appends the encoder's default extension when filename has no extension
- Automatically changes to encoder's default extension when current extension is unsupported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| else if (!encoder.IsSupported(file)) | ||
| { | ||
| string? ext = encoder.SupportExtensions().FirstOrDefault(); | ||
| if (ext != null) | ||
| DestinationFile.Value = Path.ChangeExtension(file, ext); | ||
| } |
There was a problem hiding this comment.
The IsSupported method in ControllableEncodingExtension uses case-sensitive comparison (line 7 of ControllableEncodingExtension.cs: Contains without a StringComparer), but file extensions on file systems can be case-insensitive (e.g., Windows) or case-sensitive (e.g., Linux). This means a file with extension ".MP4" might not be recognized as supported when the encoder lists ".mp4".
While this is an issue with the IsSupported implementation rather than the code in this diff, it affects the behavior of this new feature. If a user enters "video.MP4", the code at line 115 will treat it as unsupported and replace it with lowercase ".mp4", which may not be the user's intent. Consider whether this behavior is acceptable or if the extension comparison should be case-insensitive.
| DestinationFile.Value = file + ext; | ||
| } | ||
| else if (!encoder.IsSupported(file)) | ||
| { | ||
| string? ext = encoder.SupportExtensions().FirstOrDefault(); | ||
| if (ext != null) | ||
| DestinationFile.Value = Path.ChangeExtension(file, ext); |
There was a problem hiding this comment.
When adding an extension on line 113, the code concatenates the extension directly to the filename. However, when changing an extension on line 119, it uses Path.ChangeExtension. These two approaches may handle edge cases differently. For consistency and robustness, consider using Path.ChangeExtension in both cases, or ensure both approaches handle all edge cases identically (e.g., filenames ending with dots, or containing multiple dots).
| SelectedEncoder | ||
| .CombineLatest(DestinationFile) | ||
| .Subscribe(tuple => | ||
| { | ||
| var (encoder, file) = tuple; | ||
| if (encoder == null || string.IsNullOrEmpty(file)) return; | ||
|
|
||
| string currentExt = Path.GetExtension(file); | ||
| if (string.IsNullOrEmpty(currentExt)) | ||
| { | ||
| string? ext = encoder.SupportExtensions().FirstOrDefault(); | ||
| if (ext != null) | ||
| DestinationFile.Value = file + ext; | ||
| } | ||
| else if (!encoder.IsSupported(file)) | ||
| { | ||
| string? ext = encoder.SupportExtensions().FirstOrDefault(); | ||
| if (ext != null) | ||
| DestinationFile.Value = Path.ChangeExtension(file, ext); | ||
| } | ||
| }) | ||
| .DisposeWith(_disposable); |
There was a problem hiding this comment.
The new subscription (lines 101-122) interacts with the existing encoder filtering logic (lines 91-99). When DestinationFile changes to a file with an unsupported extension, both subscriptions fire:
- Lines 91-99 filter the encoder list, potentially removing the current SelectedEncoder
- Lines 101-122 attempt to fix the extension
This interaction could cause unpredictable behavior. For example, if the SelectedEncoder becomes null due to filtering before line 106 is evaluated, the extension won't be fixed. Consider the order of these operations and whether they should be coordinated.
| SelectedEncoder | ||
| .CombineLatest(DestinationFile) | ||
| .Subscribe(tuple => | ||
| { | ||
| var (encoder, file) = tuple; | ||
| if (encoder == null || string.IsNullOrEmpty(file)) return; | ||
|
|
||
| string currentExt = Path.GetExtension(file); | ||
| if (string.IsNullOrEmpty(currentExt)) | ||
| { | ||
| string? ext = encoder.SupportExtensions().FirstOrDefault(); | ||
| if (ext != null) | ||
| DestinationFile.Value = file + ext; | ||
| } | ||
| else if (!encoder.IsSupported(file)) | ||
| { | ||
| string? ext = encoder.SupportExtensions().FirstOrDefault(); | ||
| if (ext != null) | ||
| DestinationFile.Value = Path.ChangeExtension(file, ext); | ||
| } | ||
| }) | ||
| .DisposeWith(_disposable); |
There was a problem hiding this comment.
This subscription has several critical issues:
-
User Input Interference: The subscription triggers on every keystroke as the user types the filename. For example, if a user types "video" character by character, after typing "v", it would immediately be changed to "v.mp4", making it difficult to type the desired filename.
-
Re-entrancy Risk: Setting DestinationFile.Value inside a subscription that observes DestinationFile creates a re-entrancy pattern. While the logic appears to terminate in most cases, this pattern is fragile and could cause infinite loops if the encoder's IsSupported method or SupportExtensions behave unexpectedly.
Consider these alternatives:
- Use a debouncing mechanism (Throttle/Debounce operators) to only trigger after the user stops typing
- Apply extension logic only on specific events (e.g., dialog confirmation, TextBox LostFocus)
- Add a guard flag to prevent re-entry when modifying DestinationFile programmatically
- Use DistinctUntilChanged() to avoid unnecessary re-triggering
yuto-trd
changed the title
1 participant
Description
Added a feature to automatically complete file extensions when entering a file name in the output dialog.
CombineLatestsubscription forSelectedEncoderandDestinationFile.Breaking changes
None
Fixed issues
None