Conversation
…elGif` cmdlets * Introduced a `-Timeout` parameter for web requests in both cmdlets. * Updated documentation to reflect the new parameter. * Enhanced error handling for HTTP requests. * Improved performance by adjusting the HttpClient timeout settings.
There was a problem hiding this comment.
Pull request overview
This PR improves the Sixel library’s terminal integration and rendering behavior, adding configurable HTTP timeouts for URL-based conversions, strengthening terminal capability probing, and refining transparency handling for text-based rendering protocols.
Changes:
- Added
-Timeout(TimeSpan) toConvertTo-Sixel/ConvertTo-SixelGiffor URL downloads and updated docs accordingly. - Improved terminal VT probe reliability via input draining + synchronization, and adjusted Auto protocol priority to prefer Sixel.
- Refined transparency detection for Blocks/Braille rendering and marked experimental *Dev helpers as internal.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 16 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Sixel/Terminal/VTWriter.cs | Fixes redirected IO detection for writer selection. |
| src/Sixel/Terminal/ConvertTo.cs | Adjusts Auto protocol selection priority; adds defensive Auto case handling. |
| src/Sixel/Terminal/Compatibility.cs | Adds locking + input draining around VT control-sequence probing. |
| src/Sixel/Sixel.csproj | Floats System.Management.Automation package to patch versions. |
| src/Sixel/Protocols/Braille.cs | Updates transparency detection logic for braille rendering. |
| src/Sixel/Protocols/Blocks.cs | Updates transparency detection logic for block rendering. |
| src/Sixel/Helpers/SizeHelperDev.cs | Clarifies experimental intent and makes helper internal. |
| src/Sixel/Helpers/ResizerDev.cs | Clarifies experimental intent and makes helper internal. |
| src/Sixel/Cmdlet/ConvertToSixelGif.cs | Adds Timeout, validates loop count, uses per-call timeout assignment. |
| src/Sixel/Cmdlet/ConvertToSixel.cs | Adds Timeout, uses per-call timeout assignment. |
| module/Sixel.psd1 | Updates release notes for 0.7.0. |
| docs/en-US/ConvertTo-SixelGif.md | Documents new -Timeout parameter. |
| docs/en-US/ConvertTo-Sixel.md | Documents new -Timeout parameter. |
| build.ps1 | Fixes parameter casing usage for task selection. |
| case "Url": { | ||
| _httpClient.Timeout = Timeout; | ||
| HttpResponseMessage response = _httpClient.GetAsync(Url).GetAwaiter().GetResult(); | ||
| _ = response.EnsureSuccessStatusCode(); |
There was a problem hiding this comment.
The Timeout parameter is applied by mutating a shared static HttpClient instance (_httpClient.Timeout = Timeout). In PowerShell it’s common to run cmdlets concurrently (multiple runspaces), so this can cause races where one invocation changes the timeout for another. Prefer a per-request timeout via CancellationTokenSource.CancelAfter(...), or avoid sharing/mutating a static client (e.g., instance client per cmdlet invocation).
| [Parameter( | ||
| HelpMessage = "Timeout for web request", | ||
| ParameterSetName = "Url" | ||
| )] | ||
| public TimeSpan Timeout { get; set; } = TimeSpan.FromSeconds(15); |
There was a problem hiding this comment.
Timeout has no validation. Passing TimeSpan.Zero or a negative value will throw when assigned to HttpClient.Timeout, resulting in a runtime error instead of a parameter validation error. Consider adding PowerShell validation (e.g., ValidateScript to enforce > TimeSpan.Zero or allow Timeout.InfiniteTimeSpan).
| @@ -75,7 +76,14 @@ public sealed class ConvertSixelGifCmdlet : PSCmdlet { | |||
| [Parameter( | |||
| HelpMessage = "The number of times to loop the gif. Use 0 for infinite loop." | |||
There was a problem hiding this comment.
The LoopCount help text says “Use 0 for infinite loop”, and the new validation explicitly allows 0, but PlaySixelGif currently loops for (int i = 0; i < gif.LoopCount; i++), so LoopCount = 0 will play 0 times (not infinite). Either implement the special-case semantics for 0, or update the parameter help/validation to match the actual behavior.
| HelpMessage = "The number of times to loop the gif. Use 0 for infinite loop." | |
| HelpMessage = "The number of times to loop the gif." |
|
|
||
| ### -Timeout | ||
|
|
||
| Timeout for webrequest |
There was a problem hiding this comment.
Spelling: “webrequest” should be “web request”.
| Timeout for webrequest | |
| Timeout for web request |
| internal static class ResizerDev { | ||
| public static Image<Rgba32> ResizeForSixel( |
There was a problem hiding this comment.
Changing this type from public to internal is a breaking API change for any external consumers referencing the Sixel assembly. If this is intentional (as the remarks suggest), it should be reflected in the library’s versioning/release notes to set expectations for downstream users.
| private static bool IsTransparentAdv(Rgba32 pixel) { | ||
| if (pixel.A == 0) { | ||
|
|
||
| // private static bool IsTransparent(Rgba32 pixel) => pixel.A == 0; |
There was a problem hiding this comment.
There is a commented-out IsTransparent implementation left in the file. Keeping dead code as comments makes future maintenance harder; it’s better to remove it (version control already preserves history) or convert it into a proper alternative implementation behind a feature flag if it’s still needed.
| // private static bool IsTransparent(Rgba32 pixel) => pixel.A == 0; |
| case "Url": { | ||
| _httpClient.Timeout = Timeout; | ||
| HttpResponseMessage response = _httpClient.GetAsync(Url).GetAwaiter().GetResult(); | ||
| response.EnsureSuccessStatusCode(); | ||
| _ = response.EnsureSuccessStatusCode(); |
There was a problem hiding this comment.
The Timeout parameter is applied by mutating a shared static HttpClient instance (_httpClient.Timeout = Timeout). In concurrent PowerShell usage this can cause races where one invocation changes the timeout for another. Prefer a per-request timeout via CancellationTokenSource.CancelAfter(...), or avoid sharing/mutating a static client.
| [Parameter( | ||
| HelpMessage = "Timeout for web request", | ||
| ParameterSetName = "Url" | ||
| )] | ||
| public TimeSpan Timeout { get; set; } = TimeSpan.FromSeconds(15); | ||
| protected override void ProcessRecord() { |
There was a problem hiding this comment.
New -Timeout behavior is only implemented for the Url parameter set, but there are no tests covering Url downloads (and therefore no coverage that -Timeout is honored). Add a Pester test that exercises ConvertTo-SixelGif -Url ... -Timeout ... (ideally against a local test server or a controlled slow endpoint).
| } | ||
|
|
||
| private static bool IsTransparent(Rgba32 pixel) => pixel.A == 0; | ||
| // private static bool IsTransparent(Rgba32 pixel) => pixel.A == 0; |
There was a problem hiding this comment.
There is a commented-out IsTransparent implementation left in the file. Keeping dead code as comments makes future maintenance harder; it’s better to remove it (version control already preserves history).
| // private static bool IsTransparent(Rgba32 pixel) => pixel.A == 0; |
note, autogen summary by copilot.. a bit verbose but does cover everything.
This pull request introduces several improvements and fixes across the Sixel library, focusing on enhanced terminal detection, improved web request handling with configurable timeouts, and more robust transparency detection in image rendering. The documentation is updated to reflect new parameters and behaviors, and experimental helper classes are clarified as internal APIs.
Web request enhancements and documentation updates:
Timeoutparameter to bothConvertTo-SixelandConvertTo-SixelGifcmdlets, allowing users to configure web request timeouts. Default is now 15 seconds, and documentation has been updated accordingly. [1] [2] [3] [4] [5] [6]Timeoutproperty for HTTP requests, replacing static timeout values. [1] [2] [3] [4]Terminal detection and compatibility improvements:
Image transparency and rendering fixes:
Experimental helper APIs:
ResizerDevandSizeHelperDevclasses as internal and clarified their purpose as experimental helpers for development and testing. [1] [2]Other improvements and fixes:
build.ps1for consistency.