-
Notifications
You must be signed in to change notification settings - Fork 2.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
upgrade xterm to fix terminal dragging between areas #4680
Conversation
@marcdumais-work What is the process now to get CQs for runtime dependencies? This fix is quite important moving terminals around is basically broken on master. |
@svenefftinge Does it address #4319? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does! 🎉
Moving terminals around seem to work much better, @akosyakov. Have you noticed errors on the console? UPDATE These errors are related to resize and reflow. Try this: |
@AlexTugarev It seem to be xtermjs/xterm.js#1932 in xtermjs. Do you think it should be fixed before we upgrade? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! This improves moving terminals around a lot!
Personally, I'd say this improves the terminal a lot and it looks like there are just errors on the JS console. Despite of still broken reflow in some cases it, I haven't seen any issues using it. 👍 |
From the user perspective it does not work so bad, logs get really polluted. @svenefftinge suggested to have a filter for this error for time being, @AlexTugarev sounds good? |
@akosyakov If I move the terminal to the right tab bar panel, then I resize the terminal and I restart the front-end: See attachment: |
I don't see the same behaviour as you @lmcbout, everything works correctly for me, maybe you can show me in person later? |
This here is a good example case, that should be very easy to deal-with, just being an updated (so previously analysed/permitted) dependency. No official change yet to the process has been announced: we have one "NPM production dependencies CQ" for this whole repo and in theory should update it for new stuff. If we have only 1 PR waiting for the CQ, this works. But with multiple, we either need to wait until the CQ is resolved and then deal with the next one, and so on... Or somehow merge all those waiting PRs together and so obtain a single NPM "state" to submit in the CQ. As we have seen, it can take a while for those CQs to be approved, so having a single one scales poorly. Having a "CQ branch" seems like a lot of work for not much gain. I think we should rather have as many such CQs as needed, one per PR introducing a new or updated production dependency, each being independent from the others. Then when we submit e.g. the CQ for this PR here, the emphasis (for the Foundation) should be to check the differences vs latest approved CQ (i.e. master or close). IP issues that are applicable at large and not PR-specific (e.g. Electron) should be handled separately and not block the other CQs. WDYT? |
@svenefftinge @marcdumais-work Can we discuss tomorrow on dev meeting?Do we need a CQ now or we can open later and merger a PR now? I don't want to open any CQs before we agree on something to avoid confusion. |
@lmcbout I believe it is expected: you are confusing the toolbar with the caption is a sidebar. It is not relevant to this PR. |
@lmcbout It was like that always one can drag only using the caption in sidebar, not a toolbar. If you want to allow dragging with the toolbar please file an issue. It is not related to changes in this PR. |
@lmcbout I cannot reproduce it. Could you specify what you type? |
|
This reflow feature does not feel stable, maybe we wait till it is sorted out in xterm and leave with terminals only in the bottom panel. Or help to fix it in xterm. |
@akosyakov May be the re-flow is missing, but I think we should consider to uplift xterm to the latest version, at least if someone moves it elsewhere, the terminal won't be frozen . |
@akosyakov Let me ping Sharon about this. |
@Tyriar pointed out that bogus output on resize it is not xterm issue, but result produced by used shell: xtermjs/xterm.js#2002 (comment) Since #4680 (comment) is going to be fixed in 3.13.0. I suggest we wait for it and then rebase and merge this PR. I hope that 3.13.0 is already around the corner :) |
Signed-off-by: Anton Kosyakov <anton.kosyakov@typefox.io>
3.13.0 was released, good to merge? |
@@ -8,7 +8,7 @@ | |||
"@theia/filesystem": "^0.6.0", | |||
"@theia/process": "^0.6.0", | |||
"@theia/workspace": "^0.6.0", | |||
"xterm": "3.9.2" | |||
"xterm": "3.13.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@marcdumais-work Is it fine?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @akosyakov , let me run the "ECD Theia Intellectual Property Clearance, Approach" on this PR branch and get back to you. ETA ~30 mins.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix #3838