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

Change primary selection color... #5315

Conversation

goyalyashpal
Copy link
Contributor

@goyalyashpal goyalyashpal commented Dec 27, 2022

Fix #5314
in continuation (or maybe now in precession) of #5309

Please squash the commits

Sample text

(for myself as i don't have latest tutor)

=================================================================
=             10.1 CYCLING AND REMOVING SELECTIONS              =
=================================================================

 Type ) and ( to cycle the primary selection forward and backward
 through selections respectively.

 Type Alt-, to remove the primary selection.

 1. Move the cursor to the line marked '-->' below.
 2. Select both lines with xx or 2x.
 3. Type s to select, type "would" and enter.
 4. Use ( and ) to cycle the primary selection and remove the
    very second "would" with Alt-, .
 5. Type c "wood" to change the remaining "would"s to "wood".

 --> How much would would a wouldchuck chuck
 --> if a wouldchuck could chuck would?

Screenshots:



wezterm-gui_dczOYB1t3h.mp4

The colors in this pr are

key = value color (hsl) comment contrast
"ui.background" = { bg = "midnight" } #3b224c = hsl(276, 38%, 22%) default
"ui.text" = { fg = "lavender" } #a4a0e8 = hsl(243, 61%, 77%) default 5.77
"ui.selection" = { bg = " #540099 " } hsl(273, 100%, 30%) default 4.77
"ui.cursor" = { fg = " #8E89E6
", modifiers = ["reversed"] }
hsl(243, 65%, 72%) proposed 4.52
"ui.selection.primary" = { bg = " #1313BE " } hsl(240, 82%, 41%) proposed 4.71
"ui.cursor.primary" = { modifiers = ["reversed"] } default-ish 5.77
vivaldi_GdJI9jxNMp.mp4

Constrast scale used

  • Higher is better
  • It is a ratio of some sorts between the two colors,
  • so, naturally, 1:1 means both are equal i.e. same color, i.e. no contrast
  • it measures 21:1 for black on white
  • WCAG 2.0 level AA requires a contrast ratio of at least 4.5:1 for normal text and 3:1 for large text. ref: WebAIM

Contrast Constraints

  • So many constraints, oh god. Selection should contrast with background, with other selections, and most importantly with text while matching the color scheme harmonically.
  • I wanted to keep the currently used selection/cursor colors, and add only two more (rather than tweaking all 4).
  • I preferred to maintain contrast with text and background; while sacrificing inter-element contrast (i.e. with other selections).

This was the best i could do. It's noticeable when you know it, and when cycling between; may not be so in a new static screenshot - i ain't sure.

Following colors (now rejected) were chosen before this readability consideration:

  • #1212CE = hsl(240, 84%, 44%) or #8700F5 = hsl(273, 100%, 48%) for selection.primary
  • #8984E1 = hsl(243, 61%, 70%) or #756FDC = hsl(243, 61%, 65%) for ui.cursor

How this color is obtained:

  • Inspired from paletton site's preview widget, I made a small local browser page (html/css) for live testing how t'd look.
  • Contrast with text was maximised manually in hsl space using this cool contrast-checker from coolors. It wouldn't have been possible without it, as the WebAIM were too limited with their color picking sliders.

@goyalyashpal

This comment was marked as resolved.

@goyalyashpal goyalyashpal marked this pull request as draft December 27, 2022 14:15
@gibbz00
Copy link
Contributor

gibbz00 commented Dec 27, 2022

See also #5130

@goyalyashpal

This comment was marked as outdated.

@goyalyashpal
Copy link
Contributor Author

sorry i mistook, please ignore that

@gibbz00
Copy link
Contributor

gibbz00 commented Dec 27, 2022

Ahh ok, was just about to send this write-up:

Almost!

There is no secondary cursor, only primary and "normal". Currently ui.cursor sets both, unless ui.cursor.primary exists. The PR I sent adds the ability to set the primary cursor for each mode (normal, select, insert).

@goyalyashpal

This comment was marked as outdated.

@goyalyashpal
Copy link
Contributor Author

goyalyashpal commented Dec 28, 2022

hey!
i am pretty content with the colors after this last commit,
however, " there's just one problem "

For the ui.selection.primary, i want to use the specified color only when there are multiple selections, and fallback to the ui.selection otherwise when there's single selection.

Is there a way to do that?

As otherwise, ...

It looks like this (the "after" part) for single selections, whereas "before" is what's wanted

image

@goyalyashpal goyalyashpal marked this pull request as ready for review December 28, 2022 20:00
@goyalyashpal
Copy link
Contributor Author

goyalyashpal commented Dec 28, 2022

  • both the suggested colors are final from my side,
  • I've marked this as ready to indicate that as separator

i wanted to cleanup the commit history a bit too, but don't have skills and time to do it

remaining is:

  • the above mentioned issue, which i think is possible to resolve, but don't know how
  • ... as i ain't any familiar with toml or helix configs
  • but that can be handled in the commit review suggestions and it doesn't need my intervention

... so, I will not mark this as draft now, as that may deter any reviewer from thinking that i am still working on it (while any further, I will not as (hopefully) my work is done

@kirawi kirawi added A-theme Area: Theme and appearence related S-waiting-on-review Status: Awaiting review from a maintainer. labels Dec 28, 2022
@kirawi kirawi requested a review from archseer December 28, 2022 21:53
@archseer
Copy link
Member

The blue is too strong and doesn't fit with the rest of the theme. The current selection color should be made primary and a different tint chosen for secondary selections -- I had some ideas but it didn't work well in all modes, requires #5130 first

@goyalyashpal
Copy link
Contributor Author

goyalyashpal commented Dec 31, 2022

The blue is too strong

i agree with that... that's why i wanted to keep it to minimum... but i see your point. I will try suggesting other colors, but it won't be untill a month later.

@the-mikedavis the-mikedavis added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from a maintainer. labels Dec 31, 2022
Comment on lines 62 to +66
"ui.selection" = { bg = "#540099" }
"ui.selection.primary" = { bg = "#540099" }
"ui.selection.primary" = { bg = "#1313BE" }
# TODO: namespace ui.cursor as ui.selection.cursor?
"ui.cursor" = { fg = "#8E89E6", modifiers = ["reversed"] }
"ui.cursor.primary" = { modifiers = ["reversed"] }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
"ui.selection" = { bg = "#540099" }
"ui.selection.primary" = { bg = "#540099" }
"ui.selection.primary" = { bg = "#1313BE" }
# TODO: namespace ui.cursor as ui.selection.cursor?
"ui.cursor" = { fg = "#8E89E6", modifiers = ["reversed"] }
"ui.cursor.primary" = { modifiers = ["reversed"] }
"ui.selection" = { bg = "#5F005C" }
"ui.selection.primary" = { bg = "#540099" }
# TODO: namespace ui.cursor as ui.selection.cursor?
"ui.cursor" = { fg = "#B081CC", modifiers = ["reversed"] }
"ui.cursor.primary" = { modifiers = ["reversed"] }

how's this looking? @archseer

pretty muted, fits well with theme. overall seems nicely distinguishable as well, decent contrast ratio as well (5.34, 4.51). some tweaking can be done but this ain't bad either imo.

Copy link
Contributor Author

@goyalyashpal goyalyashpal Dec 31, 2022

Choose a reason for hiding this comment

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

"ui.selection" = { bg = "#64005c" } is also looking a bit more distinct from background,

Copy link
Contributor Author

@goyalyashpal goyalyashpal Jan 1, 2023

Choose a reason for hiding this comment

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

I was just watching the video at this comment @ 0:14: texstudio-org/texstudio#2467 (comment)

and, the primary/secondary color felt similar .... but yeah, the background being one extrema (dark or ligh) leaves many other contrasting color options for other stuff, as compared to it being relatively in between-ish.

@the-mikedavis the-mikedavis added S-waiting-on-review Status: Awaiting review from a maintainer. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 31, 2022
@goyalyashpal goyalyashpal mentioned this pull request Jan 16, 2023
3 tasks
@goyalyashpal
Copy link
Contributor Author

goyalyashpal commented Jan 21, 2023

Making it work with reverse will be easier once we land #5130
- @ archseer at #5281 (comment)

I had some ideas but it didn't work well in all modes, requires #5130 first

hey @archseer , now that's done, can u share your ideas?

@goyalyashpal
Copy link
Contributor Author

one more thing - i don't know if possible; can be to set the cursorcolumn (and cursorline too) to ON when in multiple selection - this would save this hassle with theming and such.

@goyalyashpal goyalyashpal deleted the fix-primary-selection-color branch July 28, 2023 15:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-theme Area: Theme and appearence related S-waiting-on-review Status: Awaiting review from a maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug with default theme color ui.selection.primary same as ui.selection
5 participants