Bump version of Xtermjs to 4.4.0

[alexl0gan]

What it does

Bump version of Xtermjs to 4.4.0

Review checklist

• as an author, I have thoroughly tested my changes and carefully followed the review guidelines

Reminder for reviewers

• as a reviewer, I agree to behave in accordance with the review guidelines

Signed-off-by: Alex Logan Alex.Logan@arm.com

[akosyakov]

@alexl0gan How did you test that it does not introduce any regressions? Usually a contributor should feel in How to test explanation.

[ghost]

While doing yarn, I hit an a minor problem in my local:

The leaveCallback function takes no args currently as per the spec, but we are supplying a function with unused args? Can we remove it?

Didn't you hit that error?

[akosyakov]

@varmanishant Where is it used?

[ghost]

@akosyakov , I have hyperlinked the usage and specification. Making it explicit for clarity:

Usage:
https://github.com/ARMmbed/theia/blob/8dcd4319b36f3acb9aa238b39cca10511a62251c/packages/terminal/src/browser/terminal-linkmatcher.ts#L53-L55

Specification:
https://xtermjs.org/docs/api/terminal/interfaces/ilinkmatcheroptions/#optional-leavecallback

I think we need to remove the args? I got this error while doing a yarn, I think @alexl0gan should have got it as well? Apart from that this PR looks good for my usecase and I am able to do the basic things like move the terminal around etc. If this is merged, I will add the PR for the options as well.

[akosyakov]

args are not used, so you can remove them

[alexl0gan]

@varmanishant yes, just using .fit() would be better.

I didn't have any issues with yarn...

[akosyakov]

@varmanishant yes, just using .fit() would be better.

@alexl0gan it was not about fit, but https://travis-ci.com/eclipse-theia/theia/jobs/288778200#L672

Would you be able to look at other build failures?

[ghost]

@alexl0gan and @akosyakov ,Thanks, now it looks correct. I removed the comment regarding fit. In Theia we are actually subtracting the row/col before resizing. Since I don't know why that is done, it is better to use the equivalent API in this case. Otherwise fit would have been better!

[akosyakov]

@alexl0gan could you squash commits which fix issues in the original commit, please see https://github.com/eclipse-theia/theia/blob/master/doc/pull-requests.md#checklist-commit-history

thank you for fixing everything, it looks good now, but someone has to double check

@marcdumais-work Do you think it would be fine to merge after the next release?

[marcdumais-work]

@marcdumais-work Do you think it would be fine to merge after the next release?

Yes, I think so.

Has anyone done the dependencies license check for this new dependency?

[akosyakov]

@marcdumais-work I don't think so

[ghost]

Looks like the new dependency xterm-addon-fit has come because xterm.js has been split into various component for modularity. However, I will check if I can run what you have mentioned above over the weekend.

[marcdumais-work]

However,I will check if I can run what you have mentioned above over the weekend.

Awesome, If you have any questions or are unsure about anything, please ping me.

[ghost]

@marcdumais-work , here is the package-lock.json:

{
  "name": "license-testing",
  "version": "1.0.0",
  "lockfileVersion": 1,
  "requires": true,
  "dependencies": {
    "xterm-addon-fit": {
      "version": "0.3.0",
      "resolved": "https://registry.yarnpkg.com/xterm-addon-fit/-/xterm-addon-fit-0.3.0.tgz",
      "integrity": "sha1-NBcQdBAn3p1kip+EQVoB3f275xU="
    }
  }
}

I dropped it to ClearlyDefined and it said MIT Licence which should be OK? It doesn't have any nested dependencies; but there is a peer dependency xterm.js ... but I didn't consider that since it is an existing library. Please let me know if this is fine?

@akosyakov, once this is added I will add the cursorWidth feature.

[AndrienkoAleksandr]

Hello @alexl0gan @akosyakov . I created CQ for xterm.js 4.4.0 and xterm-addon-fit 0.3.0:
https://dev.eclipse.org/ipzilla/show_bug.cgi?id=21696
https://dev.eclipse.org/ipzilla/show_bug.cgi?id=21697

[AndrienkoAleksandr]

I tested a bit this pr, for me it looks not bad. I will continue testing in Monday.

[akosyakov]

@AndrienkoAleksandr we only need that someone checks that license is compatible with ours, Eclipse Foundation does not require CQs for prod dependencies for Theia project: https://github.com/eclipse-theia/theia/wiki/Registering-CQs#wip---new-ecd-theia-intellectual-property-clearance-approach-experimental both deps are MIT so it should be fine, we await release and merge it

[marcdumais-work]

Hi @varmanishant

I dropped it to ClearlyDefined and it said MIT Licence which should be OK?

MIT confirmed. I think we're good to go.

[akosyakov]

@alexl0gan Release is through. Could you rebase it please? I will test it again and if everything is fine going to merge.

[akosyakov]

@alexl0gan please let me know if you have time, if not i can rebase this PR, it is good time to merge it to have the entire month for testing.

[alexl0gan]

@akosyakov just rebased

[akosyakov]

@alexl0gan thanks, but it does not compile 😬

[AndrienkoAleksandr]

Hello, I think we should appy one more dependency:
https://dev.eclipse.org/ipzilla/show_bug.cgi?id=21698
xterm-addon-search 0.5.0

[AndrienkoAleksandr]

P.S.: it doesn't matter, but all CQ passed:
https://dev.eclipse.org/ipzilla/show_bug.cgi?id=21696
https://dev.eclipse.org/ipzilla/show_bug.cgi?id=21697
https://dev.eclipse.org/ipzilla/show_bug.cgi?id=21698

[akosyakov]

@AndrienkoAleksandr Could you finish the review and merge it if it looks good to you?

[AndrienkoAleksandr]

Pr works good. And code is fine.

[akosyakov]

There seem to be regressions #7304 If someone can have a look it would help.

[AndrienkoAleksandr]

There seem to be regressions #7304 If someone can have a look it would help.

@akosyakov No it's regression after #7179