-
Notifications
You must be signed in to change notification settings - Fork 17
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
Adds Support for More Emoji Styles & Improves Preferences Menu #8
Conversation
- Data looks different: `icon` changed to `icon_${style}`
- noto-emoji is now noto, since idk if dashes will be happy - main.py should now get icon data from the correct place
- twemoji seems to work rly well w/ url naming scheme - noto/blobmoji don't quote work as well
See discussion in commit 8115b76 for more information
Also, DB Updated with new specs thanks to EmojiSpider modifications! :D
- Fixed scraper's incorrect var refs - Added Fallback Emoji Style Setting to alleviate emoji version differences - Added System Emoji display setting to further alleviate emoji version differences
Also, gave up on trying to make the extension icon dynamic. It's now hardcoded to twemoji
- Default Emoji style is now twemoji to support recognizability betwee the extension icon (also now twemoji) - Default fallback is noto, because that works better with twemoji than apple IMO
Ignore my musings, they were just working notes as I debugged my implementation. All of these threads should be considered resolved. |
Wow! A lot of work has been put into this 👍 I'll try to find time to review it this week. |
@gornostal Any updates? |
I just noticed through a bit of use that I'm experiencing one last bug. If the fallback emoji style doesn't have one of the emojis, but the normal style does, then the plugin will fail to get the emoji. I'm gonna work on resolving that now. |
If the EmojiSpider can't get the resource, it will not insert the path of where the image would be into the database. This way, when we try to figure out whether to use the fallback, we'll know based on whether or not we get a None from row['icon_{ICON STYLE}']
Alright, that should fix it. I basically changed the DB structure such that, if an icon for an emoji style couldn't be retrieved, it's just not inserted into the database. So, then we can check in main.py if the database returned anything useful, and if not, go for the fallback. |
@gornostal I just wanted to quickly point out that I've been waiting for this to be merged before starting work on #7, because I'd likely want to use logic from this branch. I could just create a new branch from this one and start working on it anyways, but I don't know what your views on proper process are for this, and I want to respect them first and foremost, since it's your project. Do you recommend I keep waiting, or is it fine with you if I make a new branch in my fork to start implementing #7? |
@gornostal just found another bug in my daily use! It's mentioned in #9 (comment) and appears like it might be fixed by a10bc58 in #9, which makes things a bit tricky (I probably shouldn't have patched it there when I did). I'm currently running the EmojiSpider, so we'll see if this edit fixed things. To fix the weird merging issues, we'll probably want to have the review of #9 block this PR's merge. That way, you can just review both when you have time, and we'll merge them both in at the same time so we can bring the relevant commits in #9 to fix this bug. |
@gornostal Bump? I'm gonna start school soon so I won't be able to keep tabs on this. Hope you get some time to review soon! Remember, review this first, then do #9! |
Delivers and closes #6. Also updates and improves the preferences available to the user.
Blocks #9. Merge is blocked by review of #9 (see #8 (comment)).
While this MR is being reviewed, users can install my fork via Ulauncher's plugin install system, since the
emoji-packs
branch has been merged intomaster
courtesy of MyriaCore#1.Known Bug
The skin tone selection doesn't work, and just returns emojis of the default skin tone. This happens because the unicode.org site that's scraped to update the emoji data in
emoji.sqlite
no longer shows skin tone variants of emojis, or emoji sequences / variations in general. Those can be found at this page.This appears to be fixed by a10bc58, but that's on another branch, and therefore isn't available in this PR. It is available in the master branch of my fork, as per MyriaCore#2.
New Features / changes
EmojiSpider.py
now pulls in emoji icons from twemoji, noto-emoji, and blobmoji, as well as the unicode.org (we'll call icons from unicode.org 'apple' style, since that's what they look most similar to)EmojiSpider.py
, which means Emoji v13 is now supported by twemoji and apple emojis, and Emoji v12 is supported by blobmoji and noto-emoji.Technical Information
The structure of the
emoji.sqlite
database has been changed to accomplish multiple emoji styles. Here are some screenshots of what theemoji
andskin_tone
tables look like now:All I've done is removed the
icon
column, and added anicon
column for each supported emoji style. Changes have been made to all relevant areas of the extension to account for this, but it's technically a breaking change, so I figured I'd include it in this report.Another important note is that the
icon_{STYLE}
columns can be null. If they're null, it means that the icon for that style could not be retrieved by the scraper. This is checked by the main extension logic to determine if the user's fallback style should be used.