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

Remove unused caret_background_color theme item from TextEdit #49513

Conversation

Calinou
Copy link
Member

@Calinou Calinou commented Jun 11, 2021

This color wasn't used anywhere.

Since this is a theme item (and not a property), removing it shouldn't break existing any projects, so we can probably cherry-pick this for 3.4. This would also be useful to remove the associated editor setting which does nothing.

I found this out while working on #48548.

@Calinou Calinou requested review from a team as code owners June 11, 2021 14:07
@Calinou Calinou added enhancement topic:editor topic:gui cherrypick:3.x Considered for cherry-picking into a future 3.x release labels Jun 11, 2021
@Calinou Calinou added this to the 4.0 milestone Jun 11, 2021
@Calinou Calinou changed the title Remove unused caret_background_color property from TextEdit Remove unused caret_background_color theme item from TextEdit Jun 11, 2021
@Calinou Calinou force-pushed the textedit-remove-unused-caret-background-color-property branch from 3abb9d7 to b4c92e8 Compare June 11, 2021 14:08
@Sslaxx
Copy link

Sslaxx commented Jun 11, 2021

Any reason not to implement it?

@Calinou
Copy link
Member Author

Calinou commented Jun 11, 2021

Any reason not to implement it?

I don't see any practical reason for it. You can already configure the caret color or change it into a block caret.

Copy link
Member

@Paulb23 Paulb23 left a comment

Choose a reason for hiding this comment

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

This is a bug, looks like it was missed as part of the RTL refactor.

caret_background_color is the colour of the text behind the caret when block caret is enabled.

@Calinou
Copy link
Member Author

Calinou commented Jun 11, 2021

This is a bug, looks like it was missed as part of the RTL refactor.

caret_background_color is the colour of the text behind the caret when block caret is enabled.

I see. Shouldn't we automatically determine this color instead (e.g. by inverting the text color if the caret's opacity is high enough)? Since very few people use the block caret in the first place, I don't think it needs a dedicated setting.

@Paulb23
Copy link
Member

Paulb23 commented Jun 11, 2021

Shouldn't we automatically determine this color instead (e.g. by inverting the text color if the caret's opacity is high enough)?

Yeah, I don't see a problem with doing that.

@akien-mga akien-mga removed the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Jun 15, 2021
@Calinou
Copy link
Member Author

Calinou commented Jul 30, 2022

This should no longer be needed now in latest master, closing.

@Calinou Calinou closed this Jul 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants