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

Sort by Recently Used #10

Merged
merged 9 commits into from Jul 20, 2021
Merged

Conversation

tomershvueli
Copy link
Contributor

@tomershvueli tomershvueli commented Jul 18, 2021

Hey @virejdasani !

This is a PR to support the following improvements that we discussed over email:

  • When filtering, the search query will also be run against the emoji names, not just keywords. This helps for things like flag names where filtering by country wouldn't find the flag you were looking for
  • When a user hits enter, it will immediately select the top most emoji, even if not explicitly selected. I found this to be amazing in the user's performance of quickly getting to where they need to go. You can even start searching and hit enter when you see your preferred emoji at the top, without having to hit 'down'
  • The vast majority of the code here is to support a 'recently used' feature. Right now the list contains up to 10 items, and those items will show up first in the modal window. One can of course still search and filter through, but by default, the most recently used will show up near the top.
    • One thing to note is that the recently used I believe will be wiped out when the app restarts. If we would want to persist this across app runs, we would need to write the LRU Map object to disk or something similar probably.
    • I also wasn't able to test this functionality (or the others come to think of it... 😅) on Windows or Linux, so would be best to test it out to make sure all is working well.

Happy to hear your thoughts!

No rush on this since I know you're busy.

@@ -105,6 +112,36 @@ ipcMain.on("typeEmoji", (_event, arg) => {
robot.typeString(arg);
});

// Return filtered and sorted emojis based on a search query
ipcMain.handle("getEmojisForSearchString", (_event, arg) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since the LRU Map is defined in main.js instead of index.js, it made sense to do the filtering and sorting here and pass along that array to index.js.

let tray = undefined;
let window = undefined;

// Let's preset our LRU Map that we'll use to keep track of recent uses
let lruMap = new LRUMap(10);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We want the LRU Map in main.js instead of index.js to persist the object across its entire runtime, not restart every time the modal pops up.

@virejdasani
Copy link
Owner

virejdasani commented Jul 19, 2021

@tomershvueli, this looks absolutely amazing! It works flawlessly on my MacBook. I'll test it on Windows and Linux then go ahead with the merge.
Also, to persist data, we can just set the recent emoji data to localStorage and then every time the app is restarted, we can use localStorage.getItem

@virejdasani
Copy link
Owner

@tomershvueli, I tried doing npm start on my MacBook in the windowsNlinux folder and there are a lot of errors. Could you check it out please, thanks!

@tomershvueli
Copy link
Contributor Author

@virejdasani take a look after the latest few pulls, I believe I fixed the windowsNlinux hierarchy and added a store to both as well to save recents in between app runs!

@virejdasani
Copy link
Owner

This looks awesome and works perfectly across all operating systems.
Going ahead with the merge, thanks a lot!
Also, just wanted your opinion on whether I should replace the Ctrl + E shortcut with the : (colon key) to make Geniemoji work more like emoji in Discord, Slack, or even here in GitHub?

@virejdasani virejdasani merged commit 9ca9aac into virejdasani:master Jul 20, 2021
@tomershvueli
Copy link
Contributor Author

Also, just wanted your opinion on whether I should replace the Ctrl + E shortcut with the : (colon key) to make Geniemoji work more like emoji in Discord, Slack, or even here in GitHub?

This is an interesting thought! I would be cautious because the colon key can also be used regularly without trying to type emojis sometimes. Would you be able to trigger it using a colon followed by any letter of the alphabet or a + key? That way if I type a colon followed by a space it wouldn't trigger the modal, but would in other cases.

@tomershvueli tomershvueli deleted the recents branch July 20, 2021 16:48
@virejdasani
Copy link
Owner

This is an interesting thought! I would be cautious because the colon key can also be used regularly without trying to type emojis sometimes. Would you be able to trigger it using a colon followed by any letter of the alphabet or a + key? That way if I type a colon followed by a space it wouldn't trigger the modal, but would in other cases.

I'll try to make it so that if there is a letter typed after the colon, then it triggers Geniemoji and automatically writes that letter in the search box so : [space] doesn't trigger anything. What do you think?

@tomershvueli
Copy link
Contributor Author

I'll try to make it so that if there is a letter typed after the colon, then it triggers Geniemoji and automatically writes that letter in the search box so : [space] doesn't trigger anything. What do you think?

I think that sounds awesome! Would love to see that in action!

@virejdasani
Copy link
Owner

virejdasani commented Jul 21, 2021

I think that sounds awesome! Would love to see that in action!

@tomershvueli, today was a holiday so I had some free time to implement the feature. It's in the master branch right now! I haven't packaged the app yet so you'll have to npm install for now. Check it out and let me know what your thoughts are!

@tomershvueli
Copy link
Contributor Author

@virejdasani neat! 2 things that might make this just perfect:

  • If I type in :s can the s already be prepopulated in the modal window?
  • More importantly, if I type in :s, find my emoji and hit 'enter', can the emoji I selected replace the :s that I typed in? For example, right now it would output :s🤩 where I just want the emoji. How plausible is this?

@virejdasani
Copy link
Owner

Thanks, I'm working on it now, I'll let you know once it's done 👍

@virejdasani
Copy link
Owner

@tomershvueli, It finally works as expected now! Check it out and let me know, thanks a lot!

@tomershvueli
Copy link
Contributor Author

Looks great!
Some small notes:

  • Change keyName to a + instead of = for code 13 in keys.js, since the modal only triggers for a + and not for an =
  • Let's manually call search() once in index.js on line 111 to trigger a search when the modal first pops up and gets populated with a letter, otherwise the user has to keep typing to try and filter for their emoji
  • The letterToWrite code in index.js can be simplified using the Array.find method. Something like this: const key = keysArr.find(i => i.keyCode === letterCode); if (key) ...

Besides that, this is looking great!

@virejdasani
Copy link
Owner

Thanks a bunch!

@virejdasani
Copy link
Owner

I decided to hold off on the colon feature because it was presenting too many bugs and entirely not working on windows and linux. I think ctlr+e is fine for now. Maybe in a future update, I'll implement this feature. Anyways, thanks a lot for your help!

@tomershvueli
Copy link
Contributor Author

@virejdasani I totally understand that, as this is starting to get into the territory of implementing things that Electron wasn't exactly built for... happy to give any input as needed in the future!

@virejdasani
Copy link
Owner

Thanks a lot :)

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