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

Adjust cell diff editor height based on viewport size #115150

Open
Tyriar opened this issue Jan 26, 2021 · 6 comments
Open

Adjust cell diff editor height based on viewport size #115150

Tyriar opened this issue Jan 26, 2021 · 6 comments
Assignees
Labels
debt Code quality issues diff-editor Diff editor issues freeze-slow-crash-leak VS Code crashing, performance, freeze and memory leak issues notebook-perf
Milestone

Comments

@Tyriar
Copy link
Member

Tyriar commented Jan 26, 2021

Testing #114969

Notice how the icon is blurry when clicking, that's how long it remains slow. Selection/editing in the diff editor is really slow as well, I believe this is because monaco is so large so it's creating so many dom elements.

recording (52)

@rebornix rebornix added bug Issue identified by VS Code Team member as probable bug notebook labels Jan 27, 2021
@rebornix rebornix added this to the January 2021 milestone Jan 27, 2021
@rebornix
Copy link
Member

Good catch!

@Tyriar
Copy link
Member Author

Tyriar commented Jan 27, 2021

The inner monaco instance is now larger than the viewport. We're also getting into ugly scroll inside scroll territory here. I'd say the giant slow monaco instance is better than this:

recording (54)

@Tyriar Tyriar reopened this Jan 27, 2021
@Tyriar Tyriar added the verification-found Issue verification failed label Jan 27, 2021
@rebornix
Copy link
Member

The inner monaco instance is now larger than the viewport. We're also getting into ugly scroll inside scroll territory here. I'd say the giant slow monaco instance is better than this:

This one is a difference case. Previously we always do a JSON format for the outputs and then render all lines, so when it has many lines, it would be more than one viewport, which is the same as now. The difference is now we limit the height to 1440px. Let's polish this in next iteration.

@rebornix rebornix modified the milestones: January 2021, February 2021 Jan 27, 2021
@Tyriar
Copy link
Member Author

Tyriar commented Jan 28, 2021

The difference is now we limit the height to 1440px.

@rebornix my monitor isn't that tall, the gif shows the scrollbar going out of view which is really bad. I'm pretty certain the majority of users are still on 1080p monitors, I suggest you base it off the viewport size instead of a magic number in this version.

@rebornix rebornix modified the milestones: February 2021, Backlog Feb 22, 2021
@rebornix rebornix added freeze-slow-crash-leak VS Code crashing, performance, freeze and memory leak issues polish Cleanup and polish issue and removed bug Issue identified by VS Code Team member as probable bug verification-found Issue verification failed labels Aug 5, 2021
@rebornix rebornix added debt Code quality issues and removed polish Cleanup and polish issue labels Oct 11, 2021
@rebornix rebornix removed the notebook label Oct 21, 2021
@rebornix rebornix changed the title Large output text renderer is slow Adjust cell diff editor height based on viewport size Dec 28, 2022
@rebornix rebornix added the diff-editor Diff editor issues label Dec 11, 2024
@rebornix rebornix assigned DonJayamanne and unassigned rebornix Dec 11, 2024
@rebornix
Copy link
Member

We now do this in notebook editor and the multi file diff editor does the same. We could look into adopting this approach for the DiffEditorWidgets in the notebook diff view.

@DonJayamanne
Copy link
Contributor

We now do this in notebook editor and the m

Please can you clarify what you mean by this?
@rebornix /cc

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
debt Code quality issues diff-editor Diff editor issues freeze-slow-crash-leak VS Code crashing, performance, freeze and memory leak issues notebook-perf
Projects
None yet
Development

No branches or pull requests

5 participants
@rebornix @DonJayamanne @Tyriar @tanhakabir and others