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

prioritize emotes based on case #7

Merged
merged 1 commit into from
Sep 16, 2018
Merged

Conversation

tollus
Copy link
Contributor

@tollus tollus commented Sep 4, 2018

Describe the pull request

Added a sort for emotes to prioritize case when auto completing. (addresses #6)

I didn't add a setting, as I'm not sure it's noticable enough to need one. But I'll definitely leave that up to your decision. I also only implemented on the emote side, and not the username side. I didn't think it made as much sense there.

Why

Lots of detail from @BlueberryKing on #6, but mostly so you don't have to type kapp for Kappa.

How

A relatively simple sort after the emotes are filtered before they're autocompleted for the user.

Screenshots

Before:
before

After:
after

@HiDeoo
Copy link
Owner

HiDeoo commented Sep 4, 2018

Thanks for the PR 😃

Very interesting, that's definitely an approach I didn't think about. The solution we discussed in #6 were definitely way more complicated 😝

I wonder if this change could have the opposite effect tho, aka in the case of "Kappa" it fixes the current issue, but could it be in other cases suggest an emote we wouldn't have expected?

I think I'll pull this down and play with it for a few days and check how it behave. If it's fine as-is, I'll merge, if weird, I'll add a setting, push & merge.

@tollus
Copy link
Contributor Author

tollus commented Sep 4, 2018

Yeah, I thought about the 'remembering' approach, which would probably be more useful in the end, but obviously harder to do.

@BlueberryKing
Copy link

Oh nice!

I think this solution should fix the issue I was having and it makes a lot of sense. I'm fine with closing #6 for now.

I wonder if this change could have the opposite effect tho, aka in the case of "Kappa" it fixes the current issue, but could it be in other cases suggest an emote we wouldn't have expected?

Definitely agree, but as you say, it's hard to predict without trying it out, so we'll just test it and see if we're happy :)

Thank you tollus for the quick solution!

@tollus
Copy link
Contributor Author

tollus commented Sep 4, 2018

Though I did just find that lu or L both produce LuL before LUL. Not sure if that's good or not. That's different than current behavior.

@BlueberryKing
Copy link

Ok, that's interesting. I don't think I mind that though. I'll keep the issue open until merged anyways.

@HiDeoo
Copy link
Owner

HiDeoo commented Sep 6, 2018

I'm still giving it a little bit more testing time, but so far, personally only LuL & LUL have been an unexpected change.

@HiDeoo HiDeoo merged commit 1a433cb into HiDeoo:master Sep 16, 2018
@BlueberryKing
Copy link

Thank you, @HiDeoo and @tollus!

@HiDeoo
Copy link
Owner

HiDeoo commented Sep 16, 2018

I did nothing ^^ It's all @tollus .

After 1+ week of test, I think it's not an annoying change at all, I couldn't find a lot of occurrences that would change the existing behavior so the PR is now merged and version v1.5.2 has been released 🎉

Thanks for the PR 😄

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.

3 participants