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

Fix Console Scrolling #14748

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

parisa-mchp
Copy link

@parisa-mchp parisa-mchp commented Jan 20, 2025

What it does

Fixes autoscrolling functionality in debug console. How this should work, is if the console is scrolled to the bottom, then when new content is output, it should be shown automatically, but if the user scrolls up, the view should stay put.

How to test

This functionality can be tested with any debuggable project. For example I used the cpp-debug-workspace, which after installing cdt-gdb-vscode, can be debugged following the instructions in the README.md file. To test, place a breakpoint within the main loop i.e. a.cpp:35. Allow the debugger to continue a few times. Each time it does, any new output to the debug console should be shown automatically. Then, scroll up a few lines and continue a few more times. Now the view should remain fixed.

Explanation

Here's how it worked previously:

When ConsoleContentWidget.model.onChanged() is fired, this triggers 3 notable events: ConsoleWidget.revealLastOutputIfNeeded, TreeWidget.updateScrollToRow, and TreeWidget.updateRows usually in that order. revealLastOutputIfNeeded uses the selection service to focus the newly added node. Next, updateScrollToRow attempts to find the row corresponding the the focused node, and fails to find it and thus doesn't scroll. Finally, after that happens updateRows creates the corresponding row.

Furthermore, the logic to determine when the console should be scrolling or not uses the onScrollUp, and onScrollYReachEnd methods, which don't appear fire for tree widgets (they work for other widgets). I believe this is because it's not the root widget container that is scrolling, but one of it's child elements, but wasn't able to figure this out.

Follow-ups

Breaking changes

  • This PR introduces breaking changes and requires careful review. If yes, the breaking changes section in the changelog has been updated.

Attribution

Review checklist

Reminder for reviewers

Signed-off-by: Parisa Betel Miri <[email protected]>
@parisa-mchp
Copy link
Author

Would it be good to also remove the onScrollReachEndEmitter and onScrollUpEmitter? Those appear to have been added specifically for this use case.

@JonasHelming JonasHelming requested a review from msujew January 21, 2025 07:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Waiting on reviewers
Development

Successfully merging this pull request may close these issues.

1 participant