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 to TextInput #8491

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

DexerBR
Copy link
Contributor

@DexerBR DexerBR commented Dec 2, 2023

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

  • 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.

Copy link
Member

@misl6 misl6 left a comment

Choose a reason for hiding this comment

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

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.

@DexerBR
Copy link
Contributor Author

DexerBR commented Dec 2, 2023

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
Copy link
Contributor Author

DexerBR commented Dec 2, 2023

@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)
    )

@misl6
Copy link
Member

misl6 commented Dec 4, 2023

@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 misl6 added this to the 3.0.0 milestone Dec 4, 2023
@DexerBR
Copy link
Contributor Author

DexerBR commented Dec 4, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants