Skip to content

Comments

Feature/rlsprep#48

Merged
trackd merged 2 commits intomainfrom
feature/rlsprep
Feb 16, 2026
Merged

Feature/rlsprep#48
trackd merged 2 commits intomainfrom
feature/rlsprep

Conversation

@trackd
Copy link
Owner

@trackd trackd commented Feb 16, 2026

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:

  • Added a Timeout parameter to both ConvertTo-Sixel and ConvertTo-SixelGif cmdlets, allowing users to configure web request timeouts. Default is now 15 seconds, and documentation has been updated accordingly. [1] [2] [3] [4] [5] [6]
  • Updated cmdlet implementations to use the new Timeout property for HTTP requests, replacing static timeout values. [1] [2] [3] [4]

Terminal detection and compatibility improvements:

  • Improved terminal control sequence handling by adding input draining to prevent probe responses from leaking into user input, and synchronizing VT probe operations with a lock. This increases reliability of terminal detection routines. [1] [2] [3]
  • Updated protocol selection priority to prefer Sixel over Kitty, aligning with typical terminal support.

Image transparency and rendering fixes:

  • Refined transparency detection logic in both block and braille rendering protocols, using luminance and alpha thresholds for more accurate handling of semi-transparent pixels. [1] [2]

Experimental helper APIs:

  • Marked ResizerDev and SizeHelperDev classes as internal and clarified their purpose as experimental helpers for development and testing. [1] [2]

Other improvements and fixes:

  • Updated release notes to highlight new Braille protocol support and improved terminal detection.
  • Fixed parameter casing in build.ps1 for consistency.
  • Relaxed PowerShell package reference version to allow patch updates.

trackd and others added 2 commits February 16, 2026 22:13
…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.
Copilot AI review requested due to automatic review settings February 16, 2026 21:43
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) to ConvertTo-Sixel / ConvertTo-SixelGif for 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.

Comment on lines 113 to 116
case "Url": {
_httpClient.Timeout = Timeout;
HttpResponseMessage response = _httpClient.GetAsync(Url).GetAwaiter().GetResult();
_ = response.EnsureSuccessStatusCode();
Copy link

Copilot AI Feb 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copilot uses AI. Check for mistakes.
Comment on lines +82 to +86
[Parameter(
HelpMessage = "Timeout for web request",
ParameterSetName = "Url"
)]
public TimeSpan Timeout { get; set; } = TimeSpan.FromSeconds(15);
Copy link

Copilot AI Feb 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copilot uses AI. Check for mistakes.
@@ -75,7 +76,14 @@ public sealed class ConvertSixelGifCmdlet : PSCmdlet {
[Parameter(
HelpMessage = "The number of times to loop the gif. Use 0 for infinite loop."
Copy link

Copilot AI Feb 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
HelpMessage = "The number of times to loop the gif. Use 0 for infinite loop."
HelpMessage = "The number of times to loop the gif."

Copilot uses AI. Check for mistakes.

### -Timeout

Timeout for webrequest
Copy link

Copilot AI Feb 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spelling: “webrequest” should be “web request”.

Suggested change
Timeout for webrequest
Timeout for web request

Copilot uses AI. Check for mistakes.
Comment on lines +23 to 24
internal static class ResizerDev {
public static Image<Rgba32> ResizeForSixel(
Copy link

Copilot AI Feb 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
private static bool IsTransparentAdv(Rgba32 pixel) {
if (pixel.A == 0) {

// private static bool IsTransparent(Rgba32 pixel) => pixel.A == 0;
Copy link

Copilot AI Feb 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
// private static bool IsTransparent(Rgba32 pixel) => pixel.A == 0;

Copilot uses AI. Check for mistakes.
Comment on lines 110 to +113
case "Url": {
_httpClient.Timeout = Timeout;
HttpResponseMessage response = _httpClient.GetAsync(Url).GetAwaiter().GetResult();
response.EnsureSuccessStatusCode();
_ = response.EnsureSuccessStatusCode();
Copy link

Copilot AI Feb 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
Comment on lines +82 to 87
[Parameter(
HelpMessage = "Timeout for web request",
ParameterSetName = "Url"
)]
public TimeSpan Timeout { get; set; } = TimeSpan.FromSeconds(15);
protected override void ProcessRecord() {
Copy link

Copilot AI Feb 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copilot generated this review using guidance from repository custom instructions.
}

private static bool IsTransparent(Rgba32 pixel) => pixel.A == 0;
// private static bool IsTransparent(Rgba32 pixel) => pixel.A == 0;
Copy link

Copilot AI Feb 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Suggested change
// private static bool IsTransparent(Rgba32 pixel) => pixel.A == 0;

Copilot uses AI. Check for mistakes.
@trackd trackd merged commit afba3e2 into main Feb 16, 2026
9 checks passed
@trackd trackd deleted the feature/rlsprep branch February 16, 2026 22:16
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.

1 participant