Update xterm to v4

[Stanzilla]

No description provided.

[Stanzilla]

Can't get it to work atm, setting to WIP. The process launches but just does not show the Hyper window.

[Stanzilla]

TypeError: Invalid value used as weak map key
    at WeakMap.set (<anonymous>)
    at TermGroup_.bind (bundle.js:1)
    at TermGroup_.renderTerm (bundle.js:1)
    at TermGroup_.render (bundle.js:1)
    at h (bundle.js:1)
    at beginWork (bundle.js:1)
    at d (bundle.js:1)
    at f (bundle.js:1)
    at g (bundle.js:1)
    at m (bundle.js:1)
h @ bundle.js:1

on

class TermGroup_ extends react__WEBPACK_IMPORTED_MODULE_0___default.a.PureComponent {
        constructor(a, b) {
            super(a, b),
            this.bound = new WeakMap,
            this.termRefs = {},
            this.onTermRef = this.onTermRef.bind(this)
        }
        bind(a, b, c) {
            this.bound.has(a) || this.bound.set(a, {});
            const d = this.bound.get(a);
            return d[c] || (d[c] = a.bind(b, c)),
            d[c]
        }

[LabhanshAgrawal]

Hi, I tried to do this and I'm able to get the terminal window and the shell, but there are some random characters appearing at the top(when using webgl). You can take a look here

[LabhanshAgrawal]

TypeError: Invalid value used as weak map key
    at WeakMap.set (<anonymous>)
    at TermGroup_.bind (bundle.js:1)
    at TermGroup_.renderTerm (bundle.js:1)
    at TermGroup_.render (bundle.js:1)
    at h (bundle.js:1)
    at beginWork (bundle.js:1)
    at d (bundle.js:1)
    at f (bundle.js:1)
    at g (bundle.js:1)
    at m (bundle.js:1)
h @ bundle.js:1

on

class TermGroup_ extends react__WEBPACK_IMPORTED_MODULE_0___default.a.PureComponent {
        constructor(a, b) {
            super(a, b),
            this.bound = new WeakMap,
            this.termRefs = {},
            this.onTermRef = this.onTermRef.bind(this)
        }
        bind(a, b, c) {
            this.bound.has(a) || this.bound.set(a, {});
            const d = this.bound.get(a);
            return d[c] || (d[c] = a.bind(b, c)),
            d[c]
        }

@Stanzilla this is coming because of onURLAbort in term-group.js as it is undefined in props

[Stanzilla]

oh thank you, that's indeed better. Those random characters are the xterm atlas, I think

[Stanzilla]

@LabhanshAgrawal try this

[LabhanshAgrawal]

Facing problems with yarn install with the changed node-pty version. Also the rendererType TermOption cannot be 'webgl' in the new xterm.

[Stanzilla]

@LabhanshAgrawal why not? it can be if we load the plugin?

[LabhanshAgrawal]

That's the error I'm getting. I think since the renderer has been moved to a plugin, it would set the renderer type when loaded.
@Tyriar might be able to clarify better.

[Stanzilla]

Oh I see

[LabhanshAgrawal]

Removing the rendererType TermOption and reverting the node-pty version is working for me.

[LabhanshAgrawal]

The random characters at the top are also gone

[Stanzilla]

Yup. pushed your suggestion, looks good!

[LabhanshAgrawal]

Is there any specific reason for changing the node-pty version? 0.9.0-beta26 is not compiling for me (on mac)

[Stanzilla]

@LabhanshAgrawal yes we need it for the next Electron update. Which node version are you on?

[LabhanshAgrawal]

my node version is 12.6.0

[Stanzilla]

@GitSquared afaik all the fork did was bundling the webgl addon and enabling it. sadly @juancampa is no longer responding on the hyper project.

[Stanzilla]

@ivanwonder yeah, it's fine in VS code, I tried.

[ivanwonder]

after resize, continue to input and check if it's layout correctly. maybe it's not the problem of xterm. there are no windows on my side

[Tyriar]

@ivanwonder haven't seen that, something like it happened in Firefox but we fixed that bug a long time ago. Note VS Code doesn't use the webgl renderer yet so it could be related to that?

[ivanwonder]

[Stanzilla]

My video about the resizing problem was with the canvas renderer btw

[ivanwonder]

@Tyriar it's canvas render. I search the issues and find fixed. but still exist.

[Tyriar]

I think this is the line that controls that:

https://github.com/xtermjs/xterm.js/blob/ff790236c1b205469f17a21246141f512d844295/src/browser/renderer/atlas/DynamicCharAtlas.ts#L254

Unless there's a bug in that version of Chrome? 🤷‍♂

[Stanzilla]

I just noticed that resizing is fine for all visible lines but off for the ones currently out of view. See https://dl.dropboxusercontent.com/s/7fcwjxyw0yivl86/2019-10-04_16-16-11.mp4

do we have to call .fit() on scroll events?

[Tyriar]

I just noticed that resizing is fine for all visible lines but off for the ones currently out of view.

That's just how winpty/conpty work because they wrap lines before xterm.js gets them. With a native pty they let the terminal emulator wrap them, see microsoft/terminal#405

[Stanzilla]

Hrm, how do we best handle that then or just have to live with it?

[Tyriar]

@Stanzilla you just have to live with it, as you have already been doing if you're on Windows. Until the upstream issue is fixed.

[Stanzilla]

meh, okay. I also noticed that resizing duplicates content in this case, looks like this then

[ivanwonder]

this is new xterm 4 with chrome 66 in electron
my local chrome version is the latest 77.0.3865.90. 😂

[Tyriar]

@ivanwonder oh it's in the demo? I don't see any issues on latest Edge Dev (Chromium 79):

Might be related to the font?

[ivanwonder]

DejaVu Sans Mono for Powerline the font I use

[LabhanshAgrawal]

I am not facing any text rendering issues with this pr (using canvas renderer). The issue I had mentioned was on merging xterm4 with electron6 branch (which on itself has rendering issues for me)
The only issue in this pr for me is the initial load resize.
(Note: This is all on mac os, can't say about others)

[Stanzilla]

Yeah I think we have to ignore the various Windows quirks and accept them. I only have Windows so here, so kinda need you guys to judge if we are otherwise ready or not.

[ivanwonder]

It's ok for me. I can live with that.

[GitSquared]

Everything works great for me on Linux, however the "About" pop-up shows the renderer is set to canvas - I'm not sure if that's expected?
Latest Hyper stable release defaults to WebGL instead.

[LabhanshAgrawal]

@GitSquared WebGL is disabled in this pr because it had some issues. See line 139 in term.js

[GitSquared]

@LabhanshAgrawal Oh OK, got it. I wrote this comment but didn't think it was copied here.

[Stanzilla]

Are we ready to merge then?

[LabhanshAgrawal]

There is the initial load resize issue, which is small and the fixing of webgl renderer, both can be tracked in new issues. I think it should be ok to merge now.