-
Notifications
You must be signed in to change notification settings - Fork 681
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
Keep autocompletion sort whenever user list updates #217
Conversation
24de830
to
9050195
Compare
Looks good to me, pretty clever solution the sort trick here! 👍 |
Hmmm, not sure I like that. This could cause problems, I feel, if I'm talking to someone and then someone else joins with a similar name and suddenly I'm auto-completing their name. |
It's not explicitly put at the top, but that appears to be how it works, might need more testing in that regard. |
9050195
to
a082039
Compare
So if I understand correctly, whenever a user joins, it gets added to the bottom of the completion list. So if we have users |
Behaviour in master is to push talking people to the top, but the bug is that whenever the user list is refreshed, that order is destroyed (thus becomes alphabetical), this PR fixes it by keeping the order, and pushing new users to the bottom. And yes it gets reset on page refresh as there is no server tracking for this. |
Yeah, I think it's much better to either do what this PR does, or to always do alphabetical entirely. If you do a kinda inbetween, you end up with confusion. I'm happy with this PR, and give me 👍, but won't merge because it has changed since @maxpoulin64 gave it 👍
FWIW, we don't need server tracking for it. We just need to keep it in local storage. That might be a good thing to do in general. |
@@ -340,7 +340,7 @@ Client.prototype.names = function(data) { | |||
} | |||
|
|||
client.emit("names", { | |||
chan: target.chan.id, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we keep this as-is, for consistency with the remaining of the code? While I admit id
makes a lot more sense, everywhere else sends chan: chan.id
:
➜ lounge git:(PR-217) grep -F 'chan.id' -B1 -R src/
src/plugins/irc-events/quit.js- client.emit("users", {
src/plugins/irc-events/quit.js: chan: chan.id
--
src/plugins/irc-events/join.js- client.emit("users", {
src/plugins/irc-events/join.js: chan: chan.id
--
src/plugins/irc-events/topic.js- client.emit("topic", {
src/plugins/irc-events/topic.js: chan: chan.id,
--
src/plugins/irc-events/message.js-
src/plugins/irc-events/message.js: if (!self && chan.id !== client.activeChannel) {
--
src/plugins/irc-events/nick.js- client.emit("users", {
src/plugins/irc-events/nick.js: chan: chan.id
--
src/plugins/irc-events/kick.js- client.emit("users", {
src/plugins/irc-events/kick.js: chan: chan.id
--
src/plugins/irc-events/names.js- client.emit("users", {
src/plugins/irc-events/names.js: chan: chan.id
--
src/plugins/irc-events/part.js- client.emit("part", {
src/plugins/irc-events/part.js: chan: chan.id
--
src/plugins/irc-events/part.js- client.emit("users", {
src/plugins/irc-events/part.js: chan: chan.id
--
src/plugins/inputs/part.js- this.emit("part", {
src/plugins/inputs/part.js: chan: chan.id
--
src/client.js- client.emit("more", {
src/client.js: chan: chan.id,
--
src/client.js- target.chan.highlight = false;
src/client.js: this.activeChannel = target.chan.id;
--
src/client.js- client.emit("names", {
src/client.js: id: target.chan.id,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean by consistency with the remaining of the code
? Changing chan
to id
allows the client side to use the same function to render user list without thinking about which variable to use.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, I see what you mean now. Yeah we should just fix it in a separate PR to use chan
everywhere then.
Alright, the more I think about this, the less it makes sense to me, but here is a shift: why not just do like the entire world is doing and sort completion alphabetically? Think about it, from a user standpoint, keeping a list of users based on how recently they joined is not intuitive at all, especially on a tool that helps users stay always connected! In the end, I'm glad the autocomplete has been "broken" for longer than I can remember: it's been alphabetical ever since. Am I out of line here? If I'm missing an obvious usability concern / use case for least recently joined vs. alphabetical autocomplete, I'd be happy to hear it. Removing the "second review needed" badge until then, to make sure that either we're all on the same page or I am dumb and missed something obvious :-) |
@astorije Auto completion becomes infinitely more useful when it's context aware. Having to deal with alphabetical auto completion in big channels is complete pain in the ass. Recently joined users are kept at the bottom of the sort list. People that talk are pushed all the way up, so they are guaranteed to be completed first. |
As discussed on the IRC channel, I understand the use case of keeping the 3-5 most recent people who talked at the top, and in case I wasn't clear, I like the idea. However, in all other use cases, it ends up building a list of nicks in a complete random order. So I'm overall not OK with the feature, but as @xPaw said, let's fix this and build upon it. 👍 for the PR. For archive purpose, here is what @xPaw and I said on the topic today:
|
I will merge as @maxpoulin64 and @YaManicKill agreed on it, and @maxpoulin64 had a previous 👍. |
Keep autocompletion sort whenever user list updates
~~This solution is not ideal for two reasons:
I didn't fix these two because it would get rather expensive to do a lot of manipulation on the arrays. Perhaps we should take names list from the server and merely sort it based on the old users array client has, probably would need to invert the users array so that nicks become order.Fixes #92
This PR keeps previous order whenever user list is updated, newly joined users are pushed to the bottom, quit users are removed from the list.