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.

Sign up for GitHub

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

Jump to bottom

Lazily load user list in channels on init, keep autocompletion sort on server #1194

Merged
merged 4 commits into from
Jul 21, 2017
Merged

Lazily load user list in channels on init, keep autocompletion sort on server #1194

merged 4 commits into from
Jul 21, 2017

Conversation

xPaw
Copy link
Member

@xPaw xPaw commented Jun 1, 2017 •
edited
Loading

Less bandwidth and processing on initial page load.

@xPaw xPaw added the Type: Feature Tickets that describe a desired feature or PRs that add them to the project. label Jun 1, 2017
@AlMcKinlay AlMcKinlay self-requested a review June 4, 2017 17:08
AlMcKinlay
AlMcKinlay approved these changes Jun 15, 2017
View reviewed changes
Copy link
Member

@AlMcKinlay AlMcKinlay left a comment

Choose a reason for hiding this comment

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

I think it took me longer to figure out how this worked than it took for you to write it :-P

src/models/chan.js Outdated
@@ -95,6 +95,7 @@ Chan.prototype.getMode = function(name) {

Chan.prototype.toJSON = function() {
var clone = _.clone(this);
clone.users = [];
Copy link
Member

Choose a reason for hiding this comment

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

Maybe a quick comment here just mentioning why we do this. I can see it would be confusing for people coming to it.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, still want this comment.

@xPaw
Copy link
Member Author

xPaw commented Jun 15, 2017

Don't merge it, it breaks auto completion (sorting) as users will be empty.

@astorije astorije added the Meta: Do Not Merge This PR should not be merged. label Jun 20, 2017
@xPaw xPaw mentioned this pull request Jul 8, 2017
Closed
@xPaw xPaw removed the Meta: Do Not Merge This PR should not be merged. label Jul 10, 2017
@xPaw
Copy link
Member Author

xPaw commented Jul 10, 2017

I have fixed auto completion!

@xPaw xPaw mentioned this pull request Jul 10, 2017
Closed
AlMcKinlay
AlMcKinlay requested changes Jul 10, 2017
View reviewed changes
client/js/render.js Outdated
return (oldSortOrder[a] || Number.MAX_VALUE) - (oldSortOrder[b] || Number.MAX_VALUE);
});
const nicks = data.users
.concat()
Copy link
Member

Choose a reason for hiding this comment

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

This does nothing, unless I'm missing something.

Copy link
Member Author

Choose a reason for hiding this comment

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

Make a copy of the object, because it sorts in-place.

Copy link
Member

Choose a reason for hiding this comment

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

Oh. Well that's a very confusing way of doing it. I thin kthe standard way to do it is with slice(0), isn't it?

Copy link
Member Author

Choose a reason for hiding this comment

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

There's no difference. i'll add a comment here

client/js/render.js Outdated
});
const nicks = data.users
.concat()
.sort((a, b) => b.lastMessage - a.lastMessage)
Copy link
Member

Choose a reason for hiding this comment

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

I think I must have missed the point of renderChannelUsers. Is it not for the users for the sidebar? Because it looks like we are sorting based on when they last sent a message, which wouldn't make sense for the sidebar. So is this for the autocompletion?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is sorting auto completion, yes.

src/models/chan.js Outdated
@@ -95,6 +95,7 @@ Chan.prototype.getMode = function(name) {

Chan.prototype.toJSON = function() {
var clone = _.clone(this);
clone.users = [];
Copy link
Member

Choose a reason for hiding this comment

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

Yeah, still want this comment.

@xPaw xPaw requested a review from astorije July 10, 2017 11:39
AlMcKinlay
AlMcKinlay approved these changes Jul 10, 2017
View reviewed changes
Copy link
Member

@AlMcKinlay AlMcKinlay left a comment

Choose a reason for hiding this comment

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

Alright, looks good. I haven't tested it, but the code looks fine. Will test sometime (probably not today)

@xPaw
Copy link
Member Author

xPaw commented Jul 11, 2017

Fixed a case where last message time would be reset when NAMES event is received. This change should also improve GC performance as we will keep User objects instead of creating new ones for every user.

@xPaw xPaw changed the title Lazily load user list in channels on init Lazily load user list in channels on init, keep autocompletion sort on server Jul 19, 2017
astorije
astorije approved these changes Jul 21, 2017
View reviewed changes
Copy link
Member

@astorije astorije left a comment

Choose a reason for hiding this comment

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

Looks good to me (squash commits?) and works great.

If you can think of a single test to add, that would be super great, but I can't, so up to you.

Should PR description say that this resolves #289?

client/js/render.js Outdated
@@ -83,7 +83,7 @@ function buildChatMessage(data) {
text.html(templates.actions[type](data.msg));
}

if ((type === "message" || type === "action") && chan.hasClass("channel")) {
if ((type === "message" || type === "action" || type === "notice") && chan.hasClass("channel")) {
Copy link
Member

Choose a reason for hiding this comment

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

Why did you need this addition? Sorry, not questioning, I'm just not seeing it 😅.

Copy link
Member Author

Choose a reason for hiding this comment

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

Notices are processed in same callback on the server as normal messages and actions. So this change was needed to match ordering correctly.

@xPaw
Copy link
Member Author

xPaw commented Jul 21, 2017

@astorije Commits are better left without squashing.

xPaw added 4 commits July 21, 2017 11:05
@xPaw
Handle auto completion order on the server
7af573f 
Fixes #289.
@xPaw
Lazily load user list
d06c279
@xPaw
Use findUser
48d367e
@xPaw
Recycle existing User objects in names event
7d981d6 
This is required to keep lastMessage correct. This will also be useful for the away tracking PR.
@xPaw xPaw added this to the 2.4.0 milestone Jul 21, 2017
@xPaw xPaw merged commit ed9bfcf into master Jul 21, 2017
@xPaw xPaw deleted the xpaw/lazy-user-list branch July 21, 2017 14:25
@astorije astorije mentioned this pull request Aug 1, 2017
Closed
matburnham pushed a commit to matburnham/lounge that referenced this pull request Sep 6, 2017
@xPaw
Merge pull request thelounge#1194 from thelounge/xpaw/lazy-user-list
35a018d 
Lazily load user list in channels on init, keep autocompletion sort on server
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AlMcKinlay AlMcKinlay AlMcKinlay approved these changes

@astorije astorije astorije approved these changes

Assignees
No one assigned
Labels
Type: Feature Tickets that describe a desired feature or PRs that add them to the project.
Projects
None yet
Milestone
2.4.0
Development

Successfully merging this pull request may close these issues.
3 participants
@xPaw @astorije @AlMcKinlay