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

Implement separate colors for primary and secondary cursors when multiple cursors are present #181991

Merged
merged 6 commits into from
Mar 20, 2024

Conversation

akbyrd
Copy link
Contributor

@akbyrd akbyrd commented May 10, 2023

Closes #85630

I took a stab at implementing a separate color for the primary cursor when multiple cursors exist. First time touching CSS so if there's a more appropriate way to theme secondary cursors let me know.

I chose to add new colors for "primary" and "secondary" cursors. These are only used when there is more than one cursor. They use the same defaults as the existing cursor color. Currently these colors do not fall back to the existing color in the case that they are undefined. There are default values in the call to registerColor so I'm unsure if this is a case that actually needs to be handled. I haven't been able to find a way to cause the colors to be undefined, even by creating a completely empty theme.

The reason I chose to add 2 new colors instead of just a single "secondary" color is for flexibility. This way it's possible to highlight the primary cursor in a bright color but only when there are multiple cursors.

image
image

The overview ruler has been updated to use these colors as well. Confirmed this updates when using editor.action.focusNextCursor.

I tried to keep everything backwards compatible. This shouldn't affect existing themes. However, this changes the HTML class when there are multiple cursors from cursor to either cursor-primary or cursor-secondary. This will break any monkey patching that targets the cursor class, but I assume that's acceptable.

akbyrd added 2 commits May 9, 2023 21:08
…re present

- Does not change the existing behavior when there's a single cursor. editorCursor.foreground and background are still used.
- Add editorCursor.multiple.primary.foreground and background theme colors for the primary cursor. Only used when multiple cursors exist. Fallback to editorCursor.foreground/background when theme colors aren't set.
- Add editorCursor.multiple.secondary.foreground and `background theme colors for non-primary cursors. Only used when multiple cursors exist. Fallback to editorCursor.foreground/background when theme colors aren't set.
Add cursor-primary and cursor-secondary html classes to target with cursor color styles. No new class is introduced in the single-cursor case.
- Currently does not affect overview ruler colors. editorCursor.foreground is still used, even when multiple cursors are present.
- This maintains the existing handling for colors being undefined. However, each of these colors have defaults do I'm not sure if it's actually possible for them to be undefined
@akbyrd akbyrd changed the title Primary cursor color Implement separate colors for primary and secondary cursors when multiple cursors are present May 10, 2023
@akbyrd
Copy link
Contributor Author

akbyrd commented May 10, 2023

@microsoft-github-policy-service agree

@akbyrd
Copy link
Contributor Author

akbyrd commented May 10, 2023

[hygiene ] File not formatted. Run the 'Format Document' command to fix it: src/vs/editor/browser/viewParts/viewCursors/viewCursor.ts

I'm not sure how to address this. When I format the document it changes all the braces. What formatter or config do I need to setup to get this to work properly?

If you have personal typescript settings those will get used. The expected settings for this repo aren't set in the workspace settings. I have to either temporarily disable my settings when working on vscode or locally modify the workspace settings to set them back to defaults.

[148630:0510/041714.104121:ERROR:ozone_platform_x11.cc(239)] Missing X server or $DISPLAY
[148630:0510/041714.104149:ERROR:env.cc(255)] The platform failed to initialize. Exiting.

Also not sure why Linux tests are failing. Looks like a CI issue?

Yup, went away the second time.

Copy link
Member

@alexdima alexdima 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!

@alexdima alexdima added this to the March 2024 milestone Mar 20, 2024
@alexdima alexdima merged commit c37edea into microsoft:main Mar 20, 2024
6 checks passed
@akbyrd akbyrd deleted the primary-cursor-color branch March 20, 2024 18:28
@microsoft microsoft locked and limited conversation to collaborators Jun 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Highlight primary cursor differently than other cursors
3 participants