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

Don't show addresses that have already been picked #1678

Closed
wants to merge 11 commits into from
10 changes: 10 additions & 0 deletions src/GroupAddressPicker.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,13 +32,18 @@ export function showGroupInviteDialog(groupId) {
</div>
</div>;

const groupStore = GroupStoreCache.getGroupStore(groupId);
const AddressPickerDialog = sdk.getComponent("dialogs.AddressPickerDialog");
Modal.createTrackedDialog('Group Invite', '', AddressPickerDialog, {
title: _t("Invite new community members"),
description: description,
placeholder: _t("Name or matrix ID"),
button: _t("Invite to Community"),
validAddressTypes: ['mx-user-id'],
excludedAddresses: groupStore.getGroupMembers().concat(groupStore.getGroupInvitedMembers()).map((member) => ({
addressType: 'mx-user-id',
address: member.userId,
})),
onFinished: (success, addrs) => {
if (!success) return;

Expand All @@ -64,6 +69,7 @@ export function showGroupAddRoomDialog(groupId) {
</div>
</label>;

const groupStore = GroupStoreCache.getGroupStore(groupId);
const AddressPickerDialog = sdk.getComponent("dialogs.AddressPickerDialog");
Modal.createTrackedDialog('Add Rooms to Group', '', AddressPickerDialog, {
title: _t("Add rooms to the community"),
Expand All @@ -73,6 +79,10 @@ export function showGroupAddRoomDialog(groupId) {
button: _t("Add to community"),
pickerType: 'room',
validAddressTypes: ['mx-room-id'],
excludedAddresses: groupStore.getGroupRooms().map((room) => ({
addressType: 'mx-room-id',
address: room.roomId,
})),
onFinished: (success, addrs) => {
if (!success) return;

Expand Down
11 changes: 11 additions & 0 deletions src/RoomInvite.js
Original file line number Diff line number Diff line change
Expand Up @@ -63,10 +63,21 @@ export function showStartChatInviteDialog() {
}

export function showRoomInviteDialog(roomId) {
const cli = MatrixClientPeg.get();
const room = cli.getRoom(roomId);
Copy link
Contributor

Choose a reason for hiding this comment

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

Could probably put the above two lines onto one line - cli is otherwise unused.


const AddressPickerDialog = sdk.getComponent("dialogs.AddressPickerDialog");
Modal.createTrackedDialog('Chat Invite', '', AddressPickerDialog, {
title: _t('Invite new room members'),
description: _t('Who would you like to add to this room?'),
excludedAddresses: room.currentState.getMembers().filter((member) => {
const membership = member.membership;
// Filter to users which are in the room, have already been invited or have been banned.
return membership === 'join' || membership === 'invite' || membership === 'ban';
}).map((member) => ({
addressType: 'mx-user-id',
address: member.userId,
})),
button: _t('Send Invites'),
placeholder: _t("Email, name or matrix ID"),
onFinished: (shouldInvite, addrs) => {
Expand Down
8 changes: 8 additions & 0 deletions src/components/structures/GroupView.js
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,10 @@ const CategoryRoomList = React.createClass({
button: _t("Add to summary"),
pickerType: 'room',
validAddressTypes: ['mx-room-id'],
excludedAddresses: this.props.rooms.map((room) => ({
addressType: 'mx-room-id',
address: room.room_id,
})),
groupId: this.props.groupId,
onFinished: (success, addrs) => {
if (!success) return;
Expand Down Expand Up @@ -262,6 +266,10 @@ const RoleUserList = React.createClass({
placeholder: _t("Name or matrix ID"),
button: _t("Add to summary"),
validAddressTypes: ['mx-user-id'],
excludedAddresses: this.props.users.map((user) => ({
addressType: 'mx-user-id',
address: user.summaryInfo.user_id,
})),
groupId: this.props.groupId,
shouldOmitSelf: false,
onFinished: (success, addrs) => {
Expand Down
71 changes: 37 additions & 34 deletions src/components/views/dialogs/AddressPickerDialog.js
Original file line number Diff line number Diff line change
Expand Up @@ -42,13 +42,14 @@ module.exports = React.createClass({
button: PropTypes.string,
focus: PropTypes.bool,
validAddressTypes: PropTypes.arrayOf(PropTypes.oneOf(addressTypes)),
excludedAddresses: PropTypes.arrayOf(PropTypes.shape({
addressType: PropTypes.oneOf(addressTypes),
address: PropTypes.string.isRequired,
})),
onFinished: PropTypes.func.isRequired,
groupId: PropTypes.string,
// The type of entity to search for. Default: 'user'.
pickerType: PropTypes.oneOf(['user', 'room']),
// Whether the current user should be included in the addresses returned. Only
// applicable when pickerType is `user`. Default: false.
includeSelf: PropTypes.bool,
},

getDefaultProps: function() {
Expand All @@ -57,7 +58,6 @@ module.exports = React.createClass({
focus: true,
validAddressTypes: addressTypes,
pickerType: 'user',
includeSelf: false,
};
},

Expand Down Expand Up @@ -130,7 +130,7 @@ module.exports = React.createClass({
} else if (e.keyCode === 13) { // enter
e.stopPropagation();
e.preventDefault();
if (this.refs.textinput.value == '') {
if (this.refs.textinput.value === '') {
// if there's nothing in the input box, submit the form
this.onButtonClick();
} else {
Expand All @@ -149,7 +149,7 @@ module.exports = React.createClass({
clearTimeout(this.queryChangedDebouncer);
}
// Only do search if there is something to search
if (query.length > 0 && query != '@' && query.length >= 2) {
if (query.length > 0 && query !== '@' && query.length >= 2) {
this.queryChangedDebouncer = setTimeout(() => {
if (this.props.pickerType === 'user') {
if (this.props.groupId) {
Expand Down Expand Up @@ -383,31 +383,33 @@ module.exports = React.createClass({
_processResults: function(results, query) {
const queryList = [];
results.forEach((result) => {
let newEntry;

if (result.room_id) {
queryList.push({
newEntry = {
addressType: 'mx-room-id',
address: result.room_id,
displayName: result.name,
avatarMxc: result.avatar_url,
isKnown: true,
});
return;
}
if (!this.props.includeSelf &&
result.user_id === MatrixClientPeg.get().credentials.userId
) {
return;
};
} else {
// Return objects, structure of which is defined
// by UserAddressType
newEntry = {
addressType: 'mx-user-id',
address: result.user_id,
displayName: result.display_name,
avatarMxc: result.avatar_url,
isKnown: true,
};
}

// Return objects, structure of which is defined
// by UserAddressType
queryList.push({
addressType: 'mx-user-id',
address: result.user_id,
displayName: result.display_name,
avatarMxc: result.avatar_url,
isKnown: true,
});
// Don't add it if it is in the excludedAddresses
if (this.props.excludedAddresses.find((entry) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is O(n^2) but optimising it would probably be premature.

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 one could be optimized by changing the format of exludedAddresses to be a map, for constant time access

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, but I'd suggest we wait until there's an actual perf problem (so for now we choose readability over speed)

return entry.address === newEntry.address && entry.addressType === newEntry.addressType;
})) return;
queryList.push(newEntry);
});

// If the query is a valid address, add an entry for that
Expand All @@ -421,7 +423,7 @@ module.exports = React.createClass({
isKnown: false,
});
if (this._cancelThreepidLookup) this._cancelThreepidLookup();
if (addrType == 'email') {
if (addrType === 'email') {
this._lookupThreepid(addrType, query).done();
}
}
Expand All @@ -444,14 +446,14 @@ module.exports = React.createClass({
if (!this.props.validAddressTypes.includes(addrType)) {
this.setState({ error: true });
return null;
} else if (addrType == 'mx-user-id') {
} else if (addrType === 'mx-user-id') {
const user = MatrixClientPeg.get().getUser(addrObj.address);
if (user) {
addrObj.displayName = user.displayName;
addrObj.avatarMxc = user.avatarUrl;
addrObj.isKnown = true;
}
} else if (addrType == 'mx-room-id') {
} else if (addrType === 'mx-room-id') {
const room = MatrixClientPeg.get().getRoom(addrObj.address);
if (room) {
addrObj.displayName = room.name;
Expand Down Expand Up @@ -511,6 +513,12 @@ module.exports = React.createClass({
const AddressSelector = sdk.getComponent("elements.AddressSelector");
this.scrollElement = null;

const queryList = this.state.queryList.filter((query) => {
return !this.state.userList.find((entry) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is O(n^2) but optimising it would also probably be premature.

return entry.address === query.address && entry.addressType === query.addressType;
});
});

const query = [];
// create the invite list
if (this.state.userList.length > 0) {
Expand Down Expand Up @@ -544,15 +552,14 @@ module.exports = React.createClass({
let error;
let addressSelector;
if (this.state.error) {
let tryUsing = '';
const validTypeDescriptions = this.props.validAddressTypes.map((t) => {
return {
'mx-user-id': _t("Matrix ID"),
'mx-room-id': _t("Matrix Room ID"),
'email': _t("email address"),
}[t];
});
tryUsing = _t("Try using one of the following valid address types: %(validTypesList)s.", {
const tryUsing = _t("Try using one of the following valid address types: %(validTypesList)s.", {
validTypesList: validTypeDescriptions.join(", "),
});
error = <div className="mx_ChatInviteDialog_error">
Expand All @@ -562,16 +569,12 @@ module.exports = React.createClass({
</div>;
} else if (this.state.searchError) {
error = <div className="mx_ChatInviteDialog_error">{ this.state.searchError }</div>;
} else if (
this.state.query.length > 0 &&
this.state.queryList.length === 0 &&
!this.state.busy
) {
} else if (this.state.query.length > 0 && queryList.length === 0 && !this.state.busy) {
error = <div className="mx_ChatInviteDialog_error">{ _t("No results") }</div>;
} else {
addressSelector = (
<AddressSelector ref={(ref) => {this.addressSelector = ref;}}
addressList={this.state.queryList}
addressList={queryList}
showAddress={this.props.pickerType === 'user'}
onSelected={this.onSelected}
truncateAt={TRUNCATE_QUERY_LIST}
Expand Down