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

Ensure that cursor state change is always emitted upon restoring state #203451

Merged
merged 2 commits into from
Jan 29, 2024

Conversation

aramikuto
Copy link
Contributor

Fixes #203345

It seems that the distinction between cases where the scrollbar background appears and where it does not lies in whether onCursorStateChanged is called or not. When a file is opened, restoreCursorState fires events that initialize the cursor state, and _emitStateChangedIfNecessary determines whether the CursorStateChangedEvent should be fired by comparing the previous and current cursor states. However, it appears that the default value for the cursor position in a newly opened view is (1,1), which results in the CursorStateChangedEvent not being fired if the cursor position after restoration is also (1,1) – the cursor positioned before the first character on the first line. Without this event, certain parts of DecorationsOverviewRuler do not initialize properly, leading to the problem I linked above.

To address this, I've ensured that cursor events are emitted if they were caused by the restoreState.

@hediet
Copy link
Member

hediet commented Jan 26, 2024

Thanks for your analysis and bug report!

I think the bug fix is at the wrong location though. The event is correct, it should only fire when it changes. When the default value is (1,1) and the workbench restores the value to (1,1), no event should be fired.

However, the DecorationsOverviewRuler should not assume that this even is fired before it gets rendered!
So in the initialization of that class, it should copy over the necessary state.

Copy link
Member

@hediet hediet left a comment

Choose a reason for hiding this comment

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

See comment.

@aramikuto
Copy link
Contributor Author

@hediet Thank you for reviewing my changes and providing feedback! I have adjusted my implementation according to the information provided.

This time, I explicitly added Position(1, 1) as the initial value for the cursor position in the DecorationsOverviewRuler constructor. This ensures that the initial value for the cursor position in both the newly opened view and the decorations overview are the same, allowing for proper rendering even when onCursorStateChanged is never called (such as when the editor's cursor is at its initial position (1,1)).

In cases where the cursor is not at the default position after state restoration, onCursorStateChanged is called, and everything should work as before.

@aramikuto aramikuto requested a review from hediet January 28, 2024 13:53
@hediet
Copy link
Member

hediet commented Jan 29, 2024

Much nicer change! Thank you!

@hediet hediet enabled auto-merge (squash) January 29, 2024 09:27
@hediet hediet added this to the February 2024 milestone Jan 29, 2024
@hediet hediet merged commit 16f38b9 into microsoft:main Jan 29, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Scrollbar background is missing when file first opened
3 participants