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

Add colored emoji support for TextInputs #7997

Closed
wants to merge 2 commits into from

Conversation

RobinPicard
Copy link
Contributor

@RobinPicard RobinPicard commented Sep 4, 2022

Description

This PR aims at answering the following issue: #7995

Currently, Labels render colored emojis correctly but not TextInputs. I believe the origin of the problem is that when a TextInput creates Labels to get the textures for each line, it does not do it in the same way as if it were a standard stand-alone Label. More specifically, the Labels are created without a value for the color property. As a result, the texture is made of pixels in shades of grey (except for emojis). Later, when the textures are added to the Canvas of the TextInput, they are colored by a Color instruction based on the value of the foreground_color property of TextInput. That way, the whome texture is made in shades of that given color. This is not a problem for regular text but that does not work for emojis that end being recolored monochromatically.

The solution is to treat Labels used in TextInputs like regular Labels by setting a value the color property following the same logic that was contained in the Color instruction (the graphic provider will not apply the color to emojis by itself I believe). Then, the Color instruction of the canvas is set to (1, 1, 1, 1) such that the texture's color are not modified.

Testing

The tests test_uix_textinput are successful and the few tests I've done with the example in the issue work fine

Maintainer merge checklist

  • Title is descriptive/clear for inclusion in release notes.
  • Applied a Component: xxx label.
  • Applied the api-deprecation or api-break label.
  • Applied the release-highlight label to be highlighted in release notes.
  • Added to the milestone version it was merged into.
  • Unittests are included in PR.
  • Properly documented, including versionadded, versionchanged as needed.

@welcome
Copy link

welcome bot commented Sep 4, 2022

Thanks for opening your first pull request here! 💖 Please check out our contributing guidelines.

Copy link
Member

@tshirtman tshirtman left a comment

Choose a reason for hiding this comment

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

Looks promising! I think it creates an issue with binding, see comment, but shouldn't be too hard to check and fix.

@@ -196,7 +196,7 @@
pos: self._cursor_visual_pos
size: root.cursor_width, -self._cursor_visual_height
Color:
rgba: self.disabled_foreground_color if self.disabled else (self.hint_text_color if not self.text else self.foreground_color)
rgba: (1, 1, 1, 1)
Copy link
Member

Choose a reason for hiding this comment

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

The previous version had the advantage of automatically adjusting the rendering if any of the xxx_color properties, or disabled was updated, the new way means lines have to be rerendered, for it to work. so binding on these properties, probably around line 568, of textinput.py, would be necessary, otherwise these color/disabled changes won't be applied until the text (or some other property that is bound) is actually changed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes, thanks for the help!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does it look fine to you now @tshirtman?

Comment on lines +569 to +571
fbind('foreground_color', refresh_line_options)
fbind('disabled_foreground_color', refresh_line_options)
fbind('hint_text_color', refresh_line_options)
Copy link
Contributor

Choose a reason for hiding this comment

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

I recommend binding to update_text_options, binding to refresh_line_options will move the cursor to the end of the text.

So you can move this code below, to the line below line 588

fbind('foreground_color', update_text_options)
fbind('disabled_foreground_color', update_text_options)
fbind('hint_text_color', update_text_options)

@misl6 misl6 added this to the 2.3.0 milestone Apr 8, 2023
@Julian-O
Copy link
Contributor

Julian-O commented Dec 2, 2023

OP has declared they are not working on Kivy any more. This is an orphan PR that needs fostering.

@misl6
Copy link
Member

misl6 commented Dec 2, 2023

Superseded by #8491

@misl6 misl6 closed this Dec 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

5 participants