OmarTawfik / terminal-screenshot

Render terminal ANSI output into images!
MIT License
9 stars 2 forks source link

feat: migrate to xterm.js 5.3.0 #10

Closed edouard-lopez closed 7 months ago

edouard-lopez commented 7 months ago
edouard-lopez commented 7 months ago

I restrict the MR to a commit bumping xterm and the related update. I will push another MR for the tooling

Do you have constraints on the tooling versions? Do you have external tests?

edouard-lopez commented 7 months ago

I checked again by rebuild the one from main from a clean slate:

❯ git checkout main
❯ rm yarn.lock node_modules/ -rf
❯ rm tests/screenshots/* 
❯ yarn install --frozen-lockfile
❯ jest tests/screenshots.test.ts

Then back on the feature branch

❯ git checkout feat/migrate-to-xterm.js-5.3.0
❯ rm yarn.lock node_modules/ -rf
❯ yarn install --frozen-lockfile
❯ jest tests/screenshots.test.ts

All are failing white-background-diff emojis-diff error-message-diff custom-background-diff custom-margin-diff

Looking closer, the ! (exclamation point) and . final dot are missing

edouard-lopez commented 7 months ago

Feature Branch

The template I got for the error-message test:

<div class="xterm-rows" style="line-height: normal; letter-spacing: 0px;" aria-hidden="true">
    <div style="width: 270px; height: 17px; line-height: 17px; overflow: hidden;">
        <span class="xterm-fg-1">ERROR:</span>
        <span class="xterm-fg-8"> something went wrong!!</span>
        <span class="xterm-cursor xterm-cursor-outline xterm-fg-8">!</span>
    </div>
</div>

There is something weird happening with the cursor it seems image

Main branch

Template render a <canvas> element. The 5.0.0 switch away from canvas for rendering (look for Canvas renderer addon section) image

edouard-lopez commented 7 months ago

@OmarTawfik Could you check again, I believe it was due to the way you hide the cursor that was not behaving in the same way with 5.3.0.

I set cursorInactiveStyle: none introduced in 5.3.0 instead

OmarTawfik commented 7 months ago

@edouard-lopez Thank you for the investigation. I did some digging and discovered a couple of issues (fixed in #16). Please feel free to rebase, and it should be able to run/test successfully now.

edouard-lopez commented 7 months ago

Rebased and fixed a bug in CI, due to @ts-node

edouard-lopez commented 7 months ago

I rebased following #14 merge