Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Order tab complete by most recently spoke #341

Merged
merged 8 commits into from
Jul 19, 2016
Merged

Order tab complete by most recently spoke #341

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
0dde891
Order tab complete by most recently spoke
dbkr Jul 14, 2016
2ce521f
Fix null error in TabComplete
dbkr Jul 14, 2016
f1d7229
Fix last-spoke order
dbkr Jul 14, 2016
d5bed78
Rejig tab complete to make it faster
dbkr Jul 15, 2016
7d712d0
Move code to make diff less confusing
dbkr Jul 15, 2016
327015b
Tidy up autocomplete updating
dbkr Jul 15, 2016
ccf8e26
Comments & required props
dbkr Jul 15, 2016
5c566ca
typo
dbkr Jul 15, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
82 changes: 72 additions & 10 deletions src/TabComplete.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,10 @@ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/
var Entry = require("./TabCompleteEntries").Entry;

import { Entry, MemberEntry, CommandEntry } from './TabCompleteEntries';
import SlashCommands from './SlashCommands';
import MatrixClientPeg from './MatrixClientPeg';

const DELAY_TIME_MS = 1000;
const KEY_TAB = 9;
Expand Down Expand Up @@ -45,23 +48,39 @@ class TabComplete {
this.isFirstWord = false; // true if you tab-complete on the first word
this.enterTabCompleteTimerId = null;
this.inPassiveMode = false;

// Map tracking ordering of the room members.
// userId: integer, highest comes first.
this.memberTabOrder = {};
Copy link
Member

Choose a reason for hiding this comment

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

What is this? A map? What are the keys? Integers to represent the ordering? User IDs? What are the values? Comment please!


// monotonically increasing counter used for tracking ordering of members
this.memberOrderSeq = 0;
Copy link
Member

Choose a reason for hiding this comment

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

What is this? A number to represent the member order? But how? Is it a max size? Missing explanatory comment.

}

/**
* @param {Entry[]} completeList
* Call this when a a UI element representing a tab complete entry has been clicked
* @param {entry} The entry that was clicked
*/
setCompletionList(completeList) {
this.list = completeList;
onEntryClick(entry) {
if (this.opts.onClickCompletes) {
// assign onClick listeners for each entry to complete the text
this.list.forEach((l) => {
l.onClick = () => {
this.completeTo(l);
}
});
this.completeTo(entry);
}
}

loadEntries(room) {
Copy link
Member

Choose a reason for hiding this comment

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

So you've changed the public API of TabComplete to take a Room rather than Entry[], meaning it cannot be reused across multiple components if they want different behaviours - we're baking in RoomView behaviour.

This may turn out to be fine - but I'm worried that there will be a number of components which would want to use the functionality which TabComplete provides - e.g. partially typing a user into the invite box then tab completing to the user to invite, ditto with searching rooms. This is why TabComplete was designed to accept a set of Entry objects rather than bake in Room specific semantics, and is why some tab-complete logic existed in RoomView. This made it really easy to extend when @ara4n wanted to include RoomMembers in addition to /commands which is what this was originally designed to do.

Copy link
Member Author

@dbkr dbkr Jul 15, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, noticed this was designed to be independent from what it was completing. It would have required a lot more fiddling to maintain this property whilst not re-doing the search every time. This means we can just pass around the TabComplete object rather than sometimes the tabcomplete object and sometimes the entries. If we ever actually want to use tabcomplete elsewhere, we can rearrange again. If we do, I'd propose adding RoomTabComplete and FooTabComplete which both use TabComplete which would be generic again.

this._makeEntries(room);
this._initSorting(room);
this._sortEntries();
}

onMemberSpoke(member) {
if (this.memberTabOrder[member.userId] === undefined) {
this.list.push(new MemberEntry(member));
}
this.memberTabOrder[member.userId] = this.memberOrderSeq++;
this._sortEntries();
Copy link
Member

Choose a reason for hiding this comment

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

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.

}

/**
* @param {DOMElement}
*/
Expand Down Expand Up @@ -307,6 +326,49 @@ class TabComplete {
this.opts.onStateChange(this.completing);
}
}

_sortEntries() {
// largest comes first
const KIND_ORDER = {
command: 1,
member: 2,
};

this.list.sort((a, b) => {
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

const kindOrderDifference = KIND_ORDER[b.kind] - KIND_ORDER[a.kind];
if (kindOrderDifference != 0) {
return kindOrderDifference;
}

if (a.kind == 'member') {
return this.memberTabOrder[b.member.userId] - this.memberTabOrder[a.member.userId];
}

// anything else we have no ordering for
return 0;
});
}

_makeEntries(room) {
const myUserId = MatrixClientPeg.get().credentials.userId;

const members = room.getJoinedMembers().filter(function(member) {
if (member.userId !== myUserId) return true;
});

this.list = MemberEntry.fromMemberList(members).concat(
CommandEntry.fromCommands(SlashCommands.getCommandList())
);
}

_initSorting(room) {
this.memberTabOrder = {};
this.memberOrderSeq = 0;

for (const ev of room.getLiveTimeline().getEvents()) {
this.memberTabOrder[ev.getSender()] = this.memberOrderSeq++;
}
}
};

module.exports = TabComplete;
21 changes: 3 additions & 18 deletions src/TabCompleteEntries.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ class Entry {
class CommandEntry extends Entry {
constructor(cmd, cmdWithArgs) {
super(cmdWithArgs);
this.kind = 'command';
this.cmd = cmd;
}

Expand All @@ -95,6 +96,7 @@ class MemberEntry extends Entry {
constructor(member) {
super(member.name || member.userId);
this.member = member;
this.kind = 'member';
}

getImageJsx() {
Expand All @@ -114,24 +116,7 @@ class MemberEntry extends Entry {
}

MemberEntry.fromMemberList = function(members) {
return members.sort(function(a, b) {
var userA = a.user;
var userB = b.user;
if (userA && !userB) {
return -1; // a comes first
}
else if (!userA && userB) {
return 1; // b comes first
}
else if (!userA && !userB) {
return 0; // don't care
}
else { // both User objects exist
var lastActiveAgoA = userA.lastActiveAgo || Number.MAX_SAFE_INTEGER;
var lastActiveAgoB = userB.lastActiveAgo || Number.MAX_SAFE_INTEGER;
return lastActiveAgoA - lastActiveAgoB;
}
}).map(function(m) {
return members.map(function(m) {
return new MemberEntry(m);
});
}
Expand Down
16 changes: 8 additions & 8 deletions src/components/structures/RoomStatusBar.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,9 @@ module.exports = React.createClass({
propTypes: {
// the room this statusbar is representing.
room: React.PropTypes.object.isRequired,
// a list of TabCompleteEntries.Entry objects
tabCompleteEntries: React.PropTypes.array,

// a TabComplete object
tabComplete: React.PropTypes.object.isRequired,

// the number of messages which have arrived since we've been scrolled up
numUnreadMessages: React.PropTypes.number,
Expand Down Expand Up @@ -208,11 +208,11 @@ module.exports = React.createClass({
);
}

if (this.props.tabCompleteEntries) {
if (this.props.tabComplete.isTabCompleting()) {
return (
<div className="mx_RoomStatusBar_tabCompleteBar">
<div className="mx_RoomStatusBar_tabCompleteWrapper">
<TabCompleteBar entries={this.props.tabCompleteEntries} />
<TabCompleteBar tabComplete={this.props.tabComplete} />
<div className="mx_RoomStatusBar_tabCompleteEol" title="->|">
<TintableSvg src="img/eol.svg" width="22" height="16"/>
Auto-complete
Expand All @@ -233,7 +233,7 @@ module.exports = React.createClass({
<a className="mx_RoomStatusBar_resend_link"
onClick={ this.props.onResendAllClick }>
Resend all
</a> or <a
</a> or <a
className="mx_RoomStatusBar_resend_link"
onClick={ this.props.onCancelAllClick }>
cancel all
Expand All @@ -247,7 +247,7 @@ module.exports = React.createClass({
// unread count trumps who is typing since the unread count is only
// set when you've scrolled up
if (this.props.numUnreadMessages) {
var unreadMsgs = this.props.numUnreadMessages + " new message" +
var unreadMsgs = this.props.numUnreadMessages + " new message" +
(this.props.numUnreadMessages > 1 ? "s" : "");

return (
Expand Down Expand Up @@ -291,5 +291,5 @@ module.exports = React.createClass({
{content}
</div>
);
},
},
});
50 changes: 21 additions & 29 deletions src/components/structures/RoomView.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,10 +31,7 @@ var Modal = require("../../Modal");
var sdk = require('../../index');
var CallHandler = require('../../CallHandler');
var TabComplete = require("../../TabComplete");
var MemberEntry = require("../../TabCompleteEntries").MemberEntry;
var CommandEntry = require("../../TabCompleteEntries").CommandEntry;
var Resend = require("../../Resend");
var SlashCommands = require("../../SlashCommands");
var dis = require("../../dispatcher");
var Tinter = require("../../Tinter");
var rate_limited_func = require('../../ratelimitedfunc');
Expand Down Expand Up @@ -205,8 +202,8 @@ module.exports = React.createClass({
MatrixClientPeg.get().credentials.userId, 'join'
);

// update the tab complete list now we have a room
this._updateTabCompleteList();
this._updateAutoComplete();
this.tabComplete.loadEntries(this.state.room);
}

if (!user_is_in_room && this.state.roomId) {
Expand Down Expand Up @@ -360,6 +357,14 @@ module.exports = React.createClass({
});
}
}

// update the tab complete list as it depends on who most recently spoke,
// and that has probably just changed
Copy link
Member

Choose a reason for hiding this comment

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

I'm worried about the possible performance implications of this. For every new event which appears in the room, we will do num_timeline_events operations to populate the lastSpoke dictionary. This will then be followed by an NlogN sorting operation on the member list. For rooms like Matrix HQ (or basically any large IRC bridged room), this could prove costly.

A better way to sort them would be to sort them once on componentDidMount, and then as new messages come in, just take that one element (the user who sent the event) and bump it to the top of the list. Is it a preemie? Probably. But I suspect it might be one we want to make.

if (ev.sender) {
this.tabComplete.onMemberSpoke(ev.sender);
// nb. we don't need to update the new autocomplete here since
// its results are currently ordered purely by search score.
}
},

// called when state.room is first initialised (either at initial load,
Expand Down Expand Up @@ -437,7 +442,8 @@ module.exports = React.createClass({
}

// a member state changed in this room, refresh the tab complete list
this._updateTabCompleteList();
this.tabComplete.loadEntries(this.state.room);
this._updateAutoComplete();

// if we are now a member of the room, where we were not before, that
// means we have finished joining a room we were previously peeking
Expand Down Expand Up @@ -502,8 +508,6 @@ module.exports = React.createClass({
window.addEventListener('resize', this.onResize);
this.onResize();

this._updateTabCompleteList();

// XXX: EVIL HACK to autofocus inviting on empty rooms.
// We use the setTimeout to avoid racing with focus_composer.
if (this.state.room &&
Expand All @@ -521,24 +525,6 @@ module.exports = React.createClass({
}
},

_updateTabCompleteList: function() {
var cli = MatrixClientPeg.get();

if (!this.state.room) {
return;
}
var members = this.state.room.getJoinedMembers().filter(function(member) {
if (member.userId !== cli.credentials.userId) return true;
});

UserProvider.getInstance().setUserList(members);
this.tabComplete.setCompletionList(
MemberEntry.fromMemberList(members).concat(
CommandEntry.fromCommands(SlashCommands.getCommandList())
)
);
},

componentDidUpdate: function() {
if (this.refs.roomView) {
var roomView = ReactDOM.findDOMNode(this.refs.roomView);
Expand Down Expand Up @@ -1263,6 +1249,14 @@ module.exports = React.createClass({
}
},

_updateAutoComplete: function() {
const myUserId = MatrixClientPeg.get().credentials.userId;
const members = this.state.room.getJoinedMembers().filter(function(member) {
if (member.userId !== myUserId) return true;
});
UserProvider.getInstance().setUserList(members);
},

render: function() {
var RoomHeader = sdk.getComponent('rooms.RoomHeader');
var MessageComposer = sdk.getComponent('rooms.MessageComposer');
Expand Down Expand Up @@ -1376,12 +1370,10 @@ module.exports = React.createClass({
statusBar = <UploadBar room={this.state.room} />
} else if (!this.state.searchResults) {
var RoomStatusBar = sdk.getComponent('structures.RoomStatusBar');
var tabEntries = this.tabComplete.isTabCompleting() ?
this.tabComplete.peek(6) : null;

statusBar = <RoomStatusBar
room={this.state.room}
tabCompleteEntries={tabEntries}
tabComplete={this.tabComplete}
numUnreadMessages={this.state.numUnreadMessages}
hasUnsentMessages={this.state.hasUnsentMessages}
atEndOfLiveTimeline={this.state.atEndOfLiveTimeline}
Expand Down
8 changes: 4 additions & 4 deletions src/components/views/rooms/TabCompleteBar.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,17 +24,17 @@ module.exports = React.createClass({
displayName: 'TabCompleteBar',

propTypes: {
entries: React.PropTypes.array.isRequired
tabComplete: React.PropTypes.object.isRequired
},

render: function() {
return (
<div className="mx_TabCompleteBar">
{this.props.entries.map(function(entry, i) {
{this.props.tabComplete.peek(6).map((entry, i) => {
return (
<div key={entry.getKey() || i + ""}
className={ "mx_TabCompleteBar_item " + (entry instanceof CommandEntry ? "mx_TabCompleteBar_command" : "") }
onClick={entry.onClick.bind(entry)} >
className={ "mx_TabCompleteBar_item " + (entry instanceof CommandEntry ? "mx_TabCompleteBar_command" : "") }
onClick={this.props.tabComplete.onEntryClick.bind(this.props.tabComplete, entry)} >
{entry.getImageJsx()}
<span className="mx_TabCompleteBar_text">
{entry.getText()}
Expand Down