community icon indicating copy to clipboard operation
community copied to clipboard

Add colored emoji support to `TextInput`

Open DexerBR opened this issue 1 year ago • 4 comments

This is a PR based on and replacement of #7997 (mentioned as an orphan PR by @Julian-O).

@RobinPicard Thanks for the original PR!

Test code:

from kivy.core.window import Window
from kivy.app import App, Builder

Window.clearcolor = (1, 1, 1, 1)


class TestApp(App):
    def build(self):
        return Builder.load_string("""
BoxLayout:
    TextInput:
        id: ti
        font_name:"seguiemj"
        foreground_color: (0, 0, 0, 1)
        cursor_width: dp(2)
    Label:
        text: ti.text
        font_name:"seguiemj"
        color: (0, 0, 0, 1)

""")


if __name__ == "__main__":
    TestApp().run()

On Windows, using seguiemj:

image

Maintainer merge checklist

  • [x] 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.

DexerBR avatar Dec 02 '23 03:12 DexerBR

I was tempted to press "merge", but I'm also concerned about breaking an undocumented behavior that someone might have used for more than a decade.

If someone (and I have, but I can live with that), created a custom TextInput, with custom KV code, without using the default implem, now that developer may have something like that in their codebase:

<MyCustomTextInput>:
    canvas.before:
        Color:
            rgba: self.background_color
        Rectangle:
            pos: self.pos
            size: self.size
        Color:
            rgba:
                (self.cursor_color
                if self.focus and not self._cursor_blink
                and int(self.x + self.padding[0]) <= self._cursor_visual_pos[0] <= int(self.x + self.width - self.padding[2])
                else (0, 0, 0, 0))
        Rectangle:
             pos: self._cursor_visual_pos
             size: root.cursor_width, -self._cursor_visual_height
         Color:
             rgba: 1, 0, 0, 1

This leverages the thing that Kivy's TextInput label has been rendered as full white, and the color has been applied via canvas for a very long time.

If we apply this PR as-is, in a feature release, someone will certainly not so happy when discovers that all the TextInput labels are rendered in a color different from the expected one (red in this case).

... so, here we have 2 proposals:

  1. Add a experimental_color_in_corelabel (or one with a better name) property that defaults to False (so the label color is applied via canvas by default), and merge this feature for 2.3.0
  2. Wait after the 2.3.0 release, merge the PR as-is and break the "undocumented API", as we already likely need to increment our major version (3.x.x)

... since we're really near to 2.3.0.rc1, I would suggest avoiding the complexity of having an experimental switch.

If someone needs this feature out there (but is useless without a proper logic for font fallback), can likely live by using the master version of Kivy for the next year or so.

So do you suggest moving this feature to 3.x.x release?

DexerBR avatar Dec 02 '23 16:12 DexerBR

@misl6 If the intention is to maintain support for the existing (undocumented) behavior and only that, an approach similar to this could be used. What do you think?

last_instruction = self.canvas.before.children[-1]
if isinstance(last_instruction, Color) and tuple(last_instruction.rgba) == (1, 1, 1, 1):
    kw["color"] = (
        self.disabled_foreground_color if self.disabled
        else (self.hint_text_color if hint else self.foreground_color)
    )

DexerBR avatar Dec 02 '23 17:12 DexerBR

@misl6 If the intention is to maintain support for the existing (undocumented) behavior and only that, an approach similar to this could be used. What do you think?

last_instruction = self.canvas.before.children[-1]
if isinstance(last_instruction, Color) and tuple(last_instruction.rgba) == (1, 1, 1, 1):
    kw["color"] = (
        self.disabled_foreground_color if self.disabled
        else (self.hint_text_color if hint else self.foreground_color)
    )

This is pretty hacky, even if may cover our need.

IMHO: Better to wait for the chance to break the API (and the undocumented behaviors).

misl6 avatar Dec 04 '23 18:12 misl6

@misl6 If the intention is to maintain support for the existing (undocumented) behavior and only that, an approach similar to this could be used. What do you think?

last_instruction = self.canvas.before.children[-1]
if isinstance(last_instruction, Color) and tuple(last_instruction.rgba) == (1, 1, 1, 1):
    kw["color"] = (
        self.disabled_foreground_color if self.disabled
        else (self.hint_text_color if hint else self.foreground_color)
    )

This is pretty hacky, even if may cover our need.

IMHO: Better to wait for the chance to break the API (and the undocumented behaviors).

Ok, agreed! 👍

DexerBR avatar Dec 04 '23 19:12 DexerBR