Order tab complete by most recently spoke #341
Conversation
|
I thought we had |
|
I think this is when they performed an action in any room though, as opposed to just the one we're in. |
| // build up a dict of when, in the history we have cached, | ||
| // each member last spoke | ||
| const lastSpoke = {}; | ||
| const timelineEvents = room.getLiveTimeline().getEvents(); |
There was a problem hiding this comment.
We now depend on the events in the room to determine ordering, but we don't call _updateTabCompleteList when we get incoming events. We probably should.
Oh that rings a bell, carry on then. |
.sende ris sometimes null: use getSender() which isn't and returns the userId which is what we actually want
Turns out this timeline is the other way around, so loop through the other way
|
ptal |
|
|
||
| // update ther tab complete list as it depends on who most recently spoke, | ||
| // and that has probably just changed | ||
| this._updateTabCompleteList(); |
There was a problem hiding this comment.
This should probably be moved up so it is inside:
if (ev.getSender() !== MatrixClientPeg.get().credentials.userId) {
// ...
}I don't think there is any point in updating the tab-completion ordering with your own user.
|
LGTM aside from performance concern. |
Now do a lot less when people speak. Also move more of the tab completion logic into TabComplete.js and out of RoomView.
| @@ -45,21 +48,32 @@ class TabComplete { | |||
| this.isFirstWord = false; // true if you tab-complete on the first word | |||
| this.enterTabCompleteTimerId = null; | |||
| this.inPassiveMode = false; | |||
| this.memberTabOrder = {}; | |||
There was a problem hiding this comment.
What is this? A map? What are the keys? Integers to represent the ordering? User IDs? What are the values? Comment please!
..into a function
| } | ||
| this.memberTabOrder[member.userId] = this.memberOrderSeq++; | ||
| this._sortEntries(); |
There was a problem hiding this comment.
Do we really have to go through and re-sort the entire member list when someone speaks? I thought the whole point of this was to avoid doing that, and yet here we are. onMemberSpoke is called when RoomView.onRoomTimeline is called, and we don't save ourselves a sort operation (we know that member should be at the head of the list; they just spoke!). Likewise, _sortEntries doesn't appear to do any optimisations beyond saving ourselves the O(N) scan of the timeline which was done in _initSorting.
| member: 2, | ||
| }; | ||
|
|
||
| this.list.sort((a, b) => { |
There was a problem hiding this comment.
Do we know what sorting algorithm this uses? The first sort will contain completely unsorted data, but subsequent sorts are done with nearly-sorted data (only 1 element needs shifting; the person who spoke). I care because this function is called for every incoming timeline event. If we use insertion sort this can be made very quick since we're operating on nearly-sorted data sets.
There was a problem hiding this comment.
I don't believe it's specified, although you're correct: I'm assuming that sorting a very-nearly sorted list should be fast: at worst, I think it's about the same which, for the number of things we're sorting, is probably fine.
There was a problem hiding this comment.
Just did a bit of digging, and indeed it is not specified in ECMAScript. Each browser implements different sorting algorithms for Array#sort():
- MergeSort for Mozilla
- a mix of QuickSort/MergeSort/SelectionSort(!!!) for Chrome, depending on the types of elements inside the array.
Fun times.
|
LGTM |
Fixes element-hq/element-web#1741