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

Use packed emoji #69

Merged
merged 5 commits into from
Jan 30, 2022
Merged

Use packed emoji #69

merged 5 commits into from
Jan 30, 2022

Conversation

jsumners
Copy link
Owner

I'm not sure how this is going to be possible. The idea is to create mapped emoji data for faster searching. But in order to serialize the data and rehydrate it quickly at run time, we need access to the file system. I don't think JXA has anything that we can shim into the code to make this happen. It might be time to investigate various methods of integrating https://github.com/bellard/quickjs into the build.

lib/genpack.js Outdated Show resolved Hide resolved
const packedEmoji = {
keywords: keywordsMap,
searchTerms: Array.from(keywordsMap.keys()),
emoji: emmojiInfo,
Copy link
Owner Author

Choose a reason for hiding this comment

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

I'm thinking we can pre-build all of the "alfredItems" and store them here. The benefit would be that we don't need to generate item data during search. The down side would be that we have to load an even larger data object on every search term. Alfred doesn't keep the script in memory. It's re-invoking it for every search term (i.e. every key press).

Copy link
Owner Author

@jsumners jsumners left a comment

Choose a reason for hiding this comment

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

@rmm5t what is your opinion of this? It's meant to address #66 (comment)

@jsumners
Copy link
Owner Author

Hmm, "wink" is only returning 😜.

@jsumners jsumners marked this pull request as ready for review January 30, 2022 13:09
@jsumners
Copy link
Owner Author

I'm going to go ahead and merge this and tag it for posterity. It's at least as fast as the current implementation on master. But it's clear to me that we need to serialize to plain JavaScript objects instead of Maps to avoid the multiple layers of decoding necessary on startup. We also really can't make use of the Map features for searching, as shown by the "wink" results.

@jsumners jsumners merged commit 69f1fc1 into master Jan 30, 2022
@jsumners jsumners deleted the msgpacked branch January 30, 2022 13:11
@devnoname120
Copy link

@jsumners Tangentially, startup time is a bit too slow for me. Usually I press the emoji shortcut then start typing straight away, e.g. ‶wink″. Usually the box takes too much time to load so it types ‶w″ in the field, and ‶ink″ in the emoji search bar.

I haven't profiled the workflow so I'm not sure why it takes time to start up. Would it be possible to asynchronously load everything so that the search box appears straight away? Results would then start appearing after something like 200ms.

@jsumners
Copy link
Owner Author

@devnoname120 I don't quite follow what you are saying. I:

  1. press cmd+space to bring up Alfred
  2. type "emoji" and press enter
  3. start typing "wink" and get results

Please try the latest release and file a new issue if you still have problems with it.

@miharekar miharekar mentioned this pull request Jan 31, 2022
@devnoname120
Copy link

devnoname120 commented Jan 31, 2022

@jsumners I directly open the menu by pressing a hotkey:

image

Here is a video that illustrates my issue:

illustration.mov

In the video, I'm pressing Cmd+Control+c and then I type wink. The input bar doesn't appear fast enough so some letters are not captured. I suspect that this latency may be due to some kind of warmup. I don't have that issue with the general Alfred bar, it appears straight away.

Currently I'm using emoji v2, it's considerably worse on #66. Unsure about the latest improvements. I'll try the latest version and report back.

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.

2 participants