Skip to content
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

[bug fix] Fix the flickering terminal widget when resizing. #12320

Conversation

meng-jpg
Copy link
Contributor

What it does

Call debounce for the method resizeTerminal to fix the issue of widget content flickering when the terminal widget is resizing.
Issue: #12280

How to test

1.start theia electron
2.open a new terminal
3.resize the window

Review checklist

Reminder for reviewers

Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

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

Thank you for your first contribution 👍

@msujew msujew added the terminal issues related to the terminal label Mar 20, 2023
@tsmaeder
Copy link
Contributor

tsmaeder commented Apr 4, 2023

@msujew @vince-fugnitto this looks like a low-hanging fruit for getting our PR count down :-)

@vince-fugnitto
Copy link
Member

@meng-jpg just so you're aware, it has been difficult to merge the pull-request due to a CI option we have where the branch needs to be constantly rebased to master and not out-of-date. Unfortunately we cannot perform the update ourselves unless you grant committers access to perform edits.

@meng-jpg
Copy link
Contributor Author

@meng-jpg just so you're aware, it has been difficult to merge the pull-request due to a CI option we have where the branch needs to be constantly rebased to master and not out-of-date. Unfortunately we cannot perform the update ourselves unless you grant committers access to perform edits.

Sorry, the PR was created from a fork living under an organization (rather than an individual). I cannot find the Allow edits from maintainers button.
https://github.com/orgs/community/discussions/5634

vince-fugnitto
vince-fugnitto previously approved these changes Apr 11, 2023
Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

LGTM 👍 I confirmed that the resizing behavior is much better after the updates.


The only impediment is the "out-of-date" check which I (or other committers) unfortunately cannot update the pull-request for you.

@msujew
Copy link
Member

msujew commented Apr 13, 2023

@meng-jpg Sorry, do you mind rebasing again? Someone else merged their PR before we could merge yours.

protected resizeTerminal(): void {
protected resizeTerminal = debounce(() => this.doResizeTerminal(), 50);

protected doResizeTerminal(): void {
const geo = this.fitAddon.proposeDimensions();
Copy link
Member

@msujew msujew Apr 14, 2023

Choose a reason for hiding this comment

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

During automated tests, this proposeDimensions function call yields undefined for some reason, which makes our tests fail. I'm not sure why, since it really shouldnt. However, we should just guard against it to fix the tests. See also here

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe we have a race condition. The previous test creates a terminal widget and it's then disposed it in the afterEach() before the following (failing) test is run. What I think happens is that between the moment the resize is triggered and the debounce delay has expired, the previous test is finished, and the terminal widget has been disposed.

It's not something likely to happen at the human scale, but since we introduce a delay here, it seems reasonable to make sure that the widget is not disposed before trying to access its internals. e.g.:

protected doResizeTerminal(): void {
   if (this.isDisposed) {
      return;
   }
   const geo = this.fitAddon.proposeDimensions();
[...]

@msujew
Copy link
Member

msujew commented Apr 17, 2023

@meng-jpg Do you mind addressing the issue that I outlined here? It makes our test suite fail.

@meng-jpg
Copy link
Contributor Author

@meng-jpg Do you mind addressing the issue that I outlined here? It makes our test suite fail.

I will try, but this issue seems a bit strange, I don't know if I can resolve it.

@vince-fugnitto vince-fugnitto modified the milestone: 1.37.0 Apr 27, 2023
@marcdumais-work
Copy link
Contributor

I will try, but this issue seems a bit strange, I don't know if I can resolve it.

@meng-jpg Please see my comment above - i think it will be easily fixed. Let us know if you need help with anything.

@vince-fugnitto vince-fugnitto self-requested a review May 25, 2023 17:03
@vince-fugnitto
Copy link
Member

@meng-jpg are you still interested in contributing the pull-request, for the failing test please see #12320 (comment).

@vince-fugnitto vince-fugnitto dismissed their stale review May 30, 2023 19:26

The current state breaks our integration test suite.

@msujew
Copy link
Member

msujew commented Jun 1, 2023

I have created #12587 to fix this issue, since the original contributor doesn't answer.

@msujew msujew closed this in #12587 Jun 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
terminal issues related to the terminal
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants