Skip to content

Conversation

@Highstaker
Copy link

Added the country flags to emoji file for easier calling.

Took them from here and deleted the ones that Telegram doesn't support yet. Maybe not all flags supported by Telegram were included.

@jsmnbom
Copy link
Member

jsmnbom commented Aug 17, 2016

I personally don't believe that this is the way that it should be implemented... Instead of having the individual countries, I would implement the individual letters instead (U+1F1E6 through U+1F1FF), since a country's flags and borders can change at any time (and would then require an update).
Then if you want say the Australian flag you'd just use a 🇦 (A) and a 🇺 (U). 🇦🇺
They could be implemented under the official names REGIONAL INDICATOR SYMBOL LETTER A-Z, though it's a bit long... FLAG_AUSTRALIA is not really a "proper" name for A + U according to the Unicode standard (http://unicode.org/emoji/charts/full-emoji-list.html).
Feel free to come with reasons why we should keep a list of all the countries in the world though :) (I admit it might be easier in some cases... but I'd value maintainability higher I think. I'd love to hear what others have to say though)

@Highstaker
Copy link
Author

Highstaker commented Aug 17, 2016

Maybe it would be better to put them into a separate file.
If a flag of a country changes, Telegram will probably change the look of the emoji as well.
To me it's much simpler to use actual region names than region codes like AU, US, etc. Not to mention that some regions might not have them at all (I'm not sure about that, but there is a possibility that some small regions the flags of which exist among Telegram emojis don't have an official letter representation).

EDIT1: On the other hand, maybe I could implement both names and country codes. 8)
Or maybe create a subclass, like Emoji.Flags.Australia or something (doesn't look too good to me, tbh)?

EDIT2: Would be great if we could doFLAGS = {("AU","Australia"): n(b'\xF0\x9F\x87\xA6\xF0\x9F\x87\xBA'), ...} and access the same values by both of these keys (both FLAGS["AU"] and FLAGS["Australia"] would refer to the same object - the flag emoji). There is a library that allows that, but creating a new dependency just for that might be unacceptable.

@jsmnbom
Copy link
Member

jsmnbom commented Aug 17, 2016

I really don't think it's necessary to have it be reference-able as both AU and Australia... I think it would be nice to have it be referenced as Emoji.Flags.Australia.
And just throwing it out there that every single country has a ISO code, and if it doesn't it wouldn't even be possible for the browser/app to have an emoji, since the Emojis are associated with the letters not the other way around.
I do however think it should be possible to construct the AU flag using Emoji.REGIONAL_INDICATOR_SYMBOL_LETTER_A + Emoji.REGIONAL_INDICATOR_SYMBOL_LETTER_U
Also: while this may break backwards compatibility please remove or at least redefine REGIONAL_INDICATOR_SYMBOL_LETTER_D_PLUS_REGIONAL_INDICATOR_SYMBOL_LETTER_E and so on, if you're adding these flags, as it seems kinda redundant to have them both...

However

If I'm being 100% honest I believe that the current Emoji module needs a serious overhaul. I think it would be a lot better if it worked more similar to the emoji module (pypi link). And I believe @jh0ker also had some ideas about refactoring the module.

@jh0ker
Copy link
Member

jh0ker commented Aug 17, 2016

@bomjacob Maybe we can just remove it all together and tell users to install that emoji module?

@jsmnbom
Copy link
Member

jsmnbom commented Aug 17, 2016

That could definitely work, we should probably test that there's not some weird edge case with telegram we're missing (missing emojis or whatever). But I definitely like that module a whole lot better than the current solution.
Also: is this a big thing in terms of backwards compatibility? (maybe set as 5.1 milestone?)

Edit: If there's any of the emojis that telegram doesn't support or something like that. We could create a fork of it https://github.com/carpedm20/emoji at https://github.com/python-telegram-bot ?
Edit 2: After testing literally all of them, it would seem that we don't need to change anything... Especially since you can't say "supported by telegram" it should really be "supported by the host platform"

@Highstaker
Copy link
Author

Sounds like a plan. Not sure how to do it properly though. Shall we add it to requirements.txt or get the actual code of carpedm20/emoji and implement into python-telegram-bot?

@jsmnbom
Copy link
Member

jsmnbom commented Aug 17, 2016

Neither...
Just recommend people install it from pypi in the readme.
pip install emoji
At least that's what I think we should do. @jh0ker, thoughts?

@Highstaker
Copy link
Author

Maybe. After all, in Python3 we can simply copypaste needed emojis right into our code and refrain from using any emoji module altogether.

@Lumiukko
Copy link

Lumiukko commented Aug 19, 2016

I have just extended the Emoji with this class, which contains Unicode characters I need that are not included in the Emoji package, and a very short function that takes a country code and returns the flags Emoji:
https://github.com/Lumiukko/begbot/blob/rewrite/extended_emoji.py

@jsmnbom
Copy link
Member

jsmnbom commented Aug 19, 2016

@Lumiukko I'm really not sure that we wanna include that in the library for two reasons:

  1. We're probably gonna rip out the Emoji module and recommend people just do pip install emoji instead.
  2. The code that you've written really only works with python3 since the Emojis are written directly in the file.

@Lumiukko
Copy link

Sorry for the confusion. That was supposed to be an argument against including flag and other emoji into this library, because extending is quite simple -- of which my code was supposed to be an example for.

@jsmnbom
Copy link
Member

jsmnbom commented Aug 19, 2016

@Lumiukko Ohhh, okay, sorry for the misunderstanding then :)

@rahiel
Copy link
Contributor

rahiel commented Aug 19, 2016

@bomjacob Maybe we can just remove it all together and tell users to install that emoji module?

I agree with @jh0ker and @bomjacob that recommending the emoji module instead is the way to go. We can start by removing the docs for telegram.Emoji and mentioning the proper emoji module somewhere.

We'll have to keep the emoji class for backwards compatibility for a while though.

@rahiel rahiel mentioned this pull request Aug 21, 2016
@jh0ker
Copy link
Member

jh0ker commented Aug 25, 2016

I will close this then, thank you for your contribution still!

@jh0ker jh0ker closed this Aug 25, 2016
@github-actions github-actions bot locked and limited conversation to collaborators Aug 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants