mirrors/signal-desktop
2
0
Fork 0
Code Issues Pull requests Projects Releases Packages 2 Wiki Activity Actions

Fine-tuning of conversation lists (compose, forward, left pane)

Browse source
This commit is contained in:
Scott Nonnenberg 2021-05-04 09:17:32 -07:00 committed by GitHub
parent f0b3c43313
commit fb00464033
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
4 changed files with 514 additions and 71 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

87
ts/state/selectors/conversations.ts
View file

@ -252,7 +252,6 @@ export const _getLeftPaneLists = (
const max = values.length;
for (let i = 0; i < max; i += 1) {
let conversation = values[i];
if (conversation.activeAt) {
if (selectedConversation === conversation.id) {
conversation = {
...conversation,
@ -260,10 +259,14 @@ export const _getLeftPaneLists = (
};
}
// We always show pinned conversations
if (conversation.isPinned) {
pinnedConversations.push(conversation);
}
if (conversation.activeAt) {
if (conversation.isArchived) {
archivedConversations.push(conversation);
} else if (conversation.isPinned) {
pinnedConversations.push(conversation);
} else {
conversations.push(conversation);
}
@ -352,12 +355,37 @@ export const getComposerConversationSearchTerm = createSelector(
}
);
function isTrusted(conversation: ConversationType): boolean {
if (conversation.type === 'group') {
return true;
}
return Boolean(
isString(conversation.name) ||
conversation.profileSharing ||
conversation.isMe
);
}
function hasDisplayInfo(conversation: ConversationType): boolean {
if (conversation.type === 'group') {
return Boolean(conversation.name);
}
return Boolean(
conversation.name ||
conversation.profileName ||
conversation.phoneNumber ||
conversation.isMe
);
}
function canComposeConversation(conversation: ConversationType): boolean {
return Boolean(
!conversation.isMe &&
!conversation.isBlocked &&
!isConversationUnregistered(conversation) &&
(isString(conversation.name) || conversation.profileSharing)
hasDisplayInfo(conversation) &&
isTrusted(conversation)
);
}
@ -371,28 +399,22 @@ export const getAllComposableConversations = createSelector(
!isConversationUnregistered(conversation) &&
// All conversation should have a title except in weird cases where
// they don't, in that case we don't want to show these for Forwarding.
conversation.title
)
);
const getContactsAndMe = createSelector(
getConversationLookup,
(conversationLookup: ConversationLookupType): Array<ConversationType> =>
Object.values(conversationLookup).filter(
contact =>
contact.type === 'direct' &&
!contact.isBlocked &&
!isConversationUnregistered(contact) &&
(isString(contact.name) || contact.profileSharing)
conversation.title &&
hasDisplayInfo(conversation)
)
);
/**
* This returns contacts for the composer and group members, which isn't just your primary
* system contacts. It may include false positives, which is better than missing contacts.
* getComposableContacts/getCandidateContactsForNewGroup both return contacts for the
* composer and group members, a different list from your primary system contacts.
* This list may include false positives, which is better than missing contacts.
*
* Because it filters unregistered contacts and that's (partially) determined by the
* current time, it's possible for this to return stale contacts that have unregistered
* Note: the key difference between them:
* getComposableContacts includes Note to Self
* getCandidateContactsForNewGroup does not include Note to Self
*
* Because they filter unregistered contacts and that's (partially) determined by the
* current time, it's possible for them to return stale contacts that have unregistered
* if no other conversations change. This should be a rare false positive.
*/
export const getComposableContacts = createSelector(
@ -404,6 +426,17 @@ export const getComposableContacts = createSelector(
)
);
export const getCandidateContactsForNewGroup = createSelector(
getConversationLookup,
(conversationLookup: ConversationLookupType): Array<ConversationType> =>
Object.values(conversationLookup).filter(
conversation =>
conversation.type === 'direct' &&
!conversation.isMe &&
canComposeConversation(conversation)
)
);
export const getComposableGroups = createSelector(
getConversationLookup,
(conversationLookup: ConversationLookupType): Array<ConversationType> =>
@ -418,9 +451,9 @@ const getNormalizedComposerConversationSearchTerm = createSelector(
(searchTerm: string): string => searchTerm.trim()
);
export const getComposeContacts = createSelector(
export const getFilteredComposeContacts = createSelector(
getNormalizedComposerConversationSearchTerm,
getContactsAndMe,
getComposableContacts,
(
searchTerm: string,
contacts: Array<ConversationType>
@ -429,7 +462,7 @@ export const getComposeContacts = createSelector(
}
);
export const getComposeGroups = createSelector(
export const getFilteredComposeGroups = createSelector(
getNormalizedComposerConversationSearchTerm,
getComposableGroups,
(
@ -440,8 +473,8 @@ export const getComposeGroups = createSelector(
}
);
export const getCandidateContactsForNewGroup = createSelector(
getComposableContacts,
export const getFilteredCandidateContactsForNewGroup = createSelector(
getCandidateContactsForNewGroup,
getNormalizedComposerConversationSearchTerm,
filterAndSortConversationsByTitle
);

4
ts/state/smart/ConversationDetails.tsx
View file

@ -9,7 +9,7 @@ import {
StateProps,
} from '../../components/conversation/conversation-details/ConversationDetails';
import {
getComposableContacts,
getCandidateContactsForNewGroup,
getConversationByIdSelector,
} from '../selectors/conversations';
import { GroupV2Membership } from '../../components/conversation/conversation-details/ConversationDetailsMembershipList';
@ -71,7 +71,7 @@ const mapStateToProps = (
);
const isAdmin = Boolean(conversation?.areWeAdmin);
const candidateContactsToAdd = getComposableContacts(state);
const candidateContactsToAdd = getCandidateContactsForNewGroup(state);
return {
...props,

12
ts/state/smart/LeftPane.tsx
View file

@ -16,10 +16,10 @@ import { ComposerStep, OneTimeModalState } from '../ducks/conversations';
import { getSearchResults, isSearching } from '../selectors/search';
import { getIntl, getRegionCode } from '../selectors/user';
import {
getCandidateContactsForNewGroup,
getFilteredCandidateContactsForNewGroup,
getCantAddContactForModal,
getComposeContacts,
getComposeGroups,
getFilteredComposeContacts,
getFilteredComposeGroups,
getComposeGroupAvatar,
getComposeGroupName,
getComposeSelectedContacts,
@ -96,15 +96,15 @@ const getModeSpecificProps = (
case ComposerStep.StartDirectConversation:
return {
mode: LeftPaneMode.Compose,
composeContacts: getComposeContacts(state),
composeGroups: getComposeGroups(state),
composeContacts: getFilteredComposeContacts(state),
composeGroups: getFilteredComposeGroups(state),
regionCode: getRegionCode(state),
searchTerm: getComposerConversationSearchTerm(state),
};
case ComposerStep.ChooseGroupMembers:
return {
mode: LeftPaneMode.ChooseGroupMembers,
candidateContacts: getCandidateContactsForNewGroup(state),
candidateContacts: getFilteredCandidateContactsForNewGroup(state),
cantAddContactForModal: getCantAddContactForModal(state),
isShowingRecommendedGroupSizeModal:
getRecommendedGroupSizeModalState(state) ===

468
ts/test-both/state/selectors/conversations_test.ts
View file

@ -16,16 +16,19 @@ import {
getAllComposableConversations,
getCandidateContactsForNewGroup,
getCantAddContactForModal,
getComposeContacts,
getComposeGroups,
getComposableContacts,
getComposableGroups,
getComposeGroupAvatar,
getComposeGroupName,
getComposeSelectedContacts,
getComposerConversationSearchTerm,
getComposerStep,
getComposeSelectedContacts,
getConversationByIdSelector,
getConversationSelector,
getConversationsByTitleSelector,
getConversationSelector,
getFilteredCandidateContactsForNewGroup,
getFilteredComposeContacts,
getFilteredComposeGroups,
getInvitedContactsForNewlyCreatedGroup,
getMaximumGroupSizeModalState,
getPlaceholderContact,
@ -486,6 +489,7 @@ describe('both/state/selectors/conversations', () => {
'our-conversation-id': {
...getDefaultConversation('our-conversation-id'),
isMe: true,
profileName: 'My own name',
},
},
},
@ -502,27 +506,33 @@ describe('both/state/selectors/conversations', () => {
Object.assign(result.conversations.conversationLookup, {
'convo-1': {
...getDefaultConversation('convo-1'),
type: 'direct',
profileName: 'A',
title: 'A',
},
'convo-2': {
...getDefaultConversation('convo-2'),
type: 'group',
isGroupV1AndDisabled: true,
name: '2',
title: 'Should Be Dropped (GV1)',
},
'convo-3': {
...getDefaultConversation('convo-3'),
type: 'group',
name: 'B',
title: 'B',
},
'convo-4': {
...getDefaultConversation('convo-4'),
isBlocked: true,
name: '4',
title: 'Should Be Dropped (blocked)',
},
'convo-5': {
...getDefaultConversation('convo-5'),
discoveredUnregisteredAt: new Date(1999, 3, 20).getTime(),
name: 'C',
title: 'C',
},
'convo-6': {
@ -534,6 +544,7 @@ describe('both/state/selectors/conversations', () => {
'convo-7': {
...getDefaultConversation('convo-7'),
discoveredUnregisteredAt: Date.now(),
name: '7',
title: 'Should Be Dropped (unregistered)',
},
});
@ -554,7 +565,254 @@ describe('both/state/selectors/conversations', () => {
});
});
describe('#getComposeContacts', () => {
describe('#getComposableContacts', () => {
const getRootState = (): StateType => {
const rootState = getEmptyRootState();
return {
...rootState,
conversations: {
...getEmptyState(),
conversationLookup: {
'our-conversation-id': {
...getDefaultConversation('our-conversation-id'),
isMe: true,
},
},
},
user: {
...rootState.user,
ourConversationId: 'our-conversation-id',
i18n,
},
};
};
it('returns only direct contacts, including me', () => {
const state = {
...getRootState(),
conversations: {
...getEmptyState(),
conversationLookup: {
'convo-0': {
...getDefaultConversation('convo-0'),
isMe: true,
profileSharing: false,
},
'convo-1': {
...getDefaultConversation('convo-1'),
type: 'group' as const,
name: 'Friends!',
},
'convo-2': {
...getDefaultConversation('convo-2'),
name: 'Alice',
},
},
},
};
const result = getComposableContacts(state);
const ids = result.map(group => group.id);
assert.deepEqual(ids, ['convo-0', 'convo-2']);
});
it('excludes blocked, unregistered, and missing name/profileSharing', () => {
const state = {
...getRootState(),
conversations: {
...getEmptyState(),
conversationLookup: {
'convo-0': {
...getDefaultConversation('convo-0'),
name: 'Ex',
isBlocked: true,
},
'convo-1': {
...getDefaultConversation('convo-1'),
name: 'Bob',
discoveredUnregisteredAt: Date.now(),
},
'convo-2': {
...getDefaultConversation('convo-2'),
name: 'Charlie',
},
},
},
};
const result = getComposableContacts(state);
const ids = result.map(group => group.id);
assert.deepEqual(ids, ['convo-2']);
});
});
describe('#getCandidateContactsForNewGroup', () => {
const getRootState = (): StateType => {
const rootState = getEmptyRootState();
return {
...rootState,
conversations: {
...getEmptyState(),
conversationLookup: {
'our-conversation-id': {
...getDefaultConversation('our-conversation-id'),
isMe: true,
},
},
},
user: {
...rootState.user,
ourConversationId: 'our-conversation-id',
i18n,
},
};
};
it('returns only direct contacts, without me', () => {
const state = {
...getRootState(),
conversations: {
...getEmptyState(),
conversationLookup: {
'convo-0': {
...getDefaultConversation('convo-0'),
isMe: true,
name: 'Me!',
},
'convo-1': {
...getDefaultConversation('convo-1'),
type: 'group' as const,
name: 'Friends!',
},
'convo-2': {
...getDefaultConversation('convo-2'),
name: 'Alice',
},
},
},
};
const result = getCandidateContactsForNewGroup(state);
const ids = result.map(group => group.id);
assert.deepEqual(ids, ['convo-2']);
});
it('excludes blocked, unregistered, and missing name/profileSharing', () => {
const state = {
...getRootState(),
conversations: {
...getEmptyState(),
conversationLookup: {
'convo-0': {
...getDefaultConversation('convo-0'),
name: 'Ex',
isBlocked: true,
},
'convo-1': {
...getDefaultConversation('convo-1'),
name: 'Bob',
discoveredUnregisteredAt: Date.now(),
},
'convo-2': {
...getDefaultConversation('convo-2'),
name: 'Charlie',
},
},
},
};
const result = getCandidateContactsForNewGroup(state);
const ids = result.map(group => group.id);
assert.deepEqual(ids, ['convo-2']);
});
});
describe('#getComposableGroups', () => {
const getRootState = (): StateType => {
const rootState = getEmptyRootState();
return {
...rootState,
conversations: {
...getEmptyState(),
conversationLookup: {
'our-conversation-id': {
...getDefaultConversation('our-conversation-id'),
isMe: true,
},
},
},
user: {
...rootState.user,
ourConversationId: 'our-conversation-id',
i18n,
},
};
};
it('returns only groups with name', () => {
const state = {
...getRootState(),
conversations: {
...getEmptyState(),
conversationLookup: {
'convo-0': {
...getDefaultConversation('convo-0'),
isMe: true,
name: 'Me!',
},
'convo-1': {
...getDefaultConversation('convo-1'),
type: 'group' as const,
name: 'Friends!',
},
'convo-2': {
...getDefaultConversation('convo-2'),
type: 'group' as const,
},
},
},
};
const result = getComposableGroups(state);
const ids = result.map(group => group.id);
assert.deepEqual(ids, ['convo-1']);
});
it('excludes blocked, and missing name/profileSharing', () => {
const state = {
...getRootState(),
conversations: {
...getEmptyState(),
conversationLookup: {
'convo-0': {
...getDefaultConversation('convo-0'),
type: 'group' as const,
name: 'Family!',
isBlocked: true,
},
'convo-1': {
...getDefaultConversation('convo-1'),
type: 'group' as const,
name: 'Friends!',
},
'convo-2': {
...getDefaultConversation('convo-2'),
type: 'group' as const,
},
},
},
};
const result = getComposableGroups(state);
const ids = result.map(group => group.id);
assert.deepEqual(ids, ['convo-1']);
});
});
describe('#getFilteredComposeContacts', () => {
const getRootState = (searchTerm = ''): StateType => {
const rootState = getEmptyRootState();
return {
@ -564,6 +822,9 @@ describe('both/state/selectors/conversations', () => {
conversationLookup: {
'our-conversation-id': {
...getDefaultConversation('our-conversation-id'),
name: 'Me, Myself, and I',
title: 'Me, Myself, and I',
searchableTitle: 'Note to Self',
isMe: true,
},
},
@ -583,13 +844,6 @@ describe('both/state/selectors/conversations', () => {
const getRootStateWithConversations = (searchTerm = ''): StateType => {
const result = getRootState(searchTerm);
Object.assign(result.conversations.conversationLookup, {
'convo-0': {
...getDefaultConversation('convo-0'),
name: 'Me, Myself, and I',
title: 'Me, Myself, and I',
searchableTitle: 'Note to Self',
isMe: true,
},
'convo-1': {
...getDefaultConversation('convo-1'),
name: 'In System Contacts',
@ -618,6 +872,7 @@ describe('both/state/selectors/conversations', () => {
'convo-6': {
...getDefaultConversation('convo-6'),
profileSharing: true,
profileName: 'C. Has Profile Sharing',
title: 'C. Has Profile Sharing',
},
'convo-7': {
@ -631,23 +886,27 @@ describe('both/state/selectors/conversations', () => {
it('returns no results when there are no contacts', () => {
const state = getRootState('foo bar baz');
const result = getComposeContacts(state);
const result = getFilteredComposeContacts(state);
assert.isEmpty(result);
});
it('includes Note to Self', () => {
it('includes Note to Self with no search term', () => {
const state = getRootStateWithConversations();
const result = getComposeContacts(state);
const result = getFilteredComposeContacts(state);
const ids = result.map(contact => contact.id);
// convo-6 is sorted last because it doesn't have a name
assert.deepEqual(ids, ['convo-1', 'convo-5', 'convo-0', 'convo-6']);
assert.deepEqual(ids, [
'convo-1',
'convo-5',
'convo-6',
'our-conversation-id',
]);
});
it('can search for contacts', () => {
const state = getRootStateWithConversations('in system');
const result = getComposeContacts(state);
const result = getFilteredComposeContacts(state);
const ids = result.map(contact => contact.id);
// NOTE: convo-6 matches because you can't write "Sharing" without "in"
@ -656,22 +915,22 @@ describe('both/state/selectors/conversations', () => {
it('can search for note to self', () => {
const state = getRootStateWithConversations('note');
const result = getComposeContacts(state);
const result = getFilteredComposeContacts(state);
const ids = result.map(contact => contact.id);
assert.deepEqual(ids, ['convo-0']);
assert.deepEqual(ids, ['our-conversation-id']);
});
it('returns note to self when searching for your own name', () => {
const state = getRootStateWithConversations('Myself');
const result = getComposeContacts(state);
const result = getFilteredComposeContacts(state);
const ids = result.map(contact => contact.id);
assert.deepEqual(ids, ['convo-0']);
assert.deepEqual(ids, ['our-conversation-id']);
});
});
describe('#getComposeGroups', () => {
describe('#getFilteredComposeGroups', () => {
const getState = (searchTerm = ''): StateType => {
const rootState = getEmptyRootState();
return {
@ -738,7 +997,7 @@ describe('both/state/selectors/conversations', () => {
it('can search for groups', () => {
const state = getState('hello');
const result = getComposeGroups(state);
const result = getFilteredComposeGroups(state);
const ids = result.map(group => group.id);
assert.deepEqual(ids, ['convo-3']);
@ -746,14 +1005,14 @@ describe('both/state/selectors/conversations', () => {
it('does not return unknown groups when getting all groups (no search term)', () => {
const state = getState();
const result = getComposeGroups(state);
const result = getFilteredComposeGroups(state);
const ids = result.map(group => group.id);
assert.deepEqual(ids, ['convo-3', 'convo-6', 'convo-7']);
});
});
describe('#getCandidateContactsForNewGroup', () => {
describe('#getFilteredCandidateContactsForNewGroup', () => {
const getRootState = (searchTerm = ''): StateType => {
const rootState = getEmptyRootState();
return {
@ -819,7 +1078,7 @@ describe('both/state/selectors/conversations', () => {
it('returns sorted contacts when there is no search term', () => {
const state = getRootState();
const result = getCandidateContactsForNewGroup(state);
const result = getFilteredCandidateContactsForNewGroup(state);
const ids = result.map(contact => contact.id);
assert.deepEqual(ids, ['convo-1', 'convo-5']);
@ -827,7 +1086,7 @@ describe('both/state/selectors/conversations', () => {
it('can search for contacts', () => {
const state = getRootState('system contacts');
const result = getCandidateContactsForNewGroup(state);
const result = getFilteredCandidateContactsForNewGroup(state);
const ids = result.map(contact => contact.id);
assert.deepEqual(ids, ['convo-1', 'convo-5']);
@ -917,7 +1176,7 @@ describe('both/state/selectors/conversations', () => {
});
});
describe('#getLeftPaneList', () => {
describe('#_getLeftPaneLists', () => {
it('sorts conversations based on timestamp then by intl-friendly title', () => {
const data: ConversationLookupType = {
id1: {
@ -1152,6 +1411,157 @@ describe('both/state/selectors/conversations', () => {
assert.strictEqual(pinnedConversations[1].name, 'Pin Two');
assert.strictEqual(pinnedConversations[2].name, 'Pin Three');
});
it('includes archived and pinned conversations with no active_at', () => {
const data: ConversationLookupType = {
pin2: {
id: 'pin2',
e164: '+18005551111',
name: 'Pin Two',
timestamp: 30,
inboxPosition: 30,
phoneNumber: 'notused',
isArchived: false,
isPinned: true,
markedUnread: false,
type: 'direct',
isMe: false,
lastUpdated: Date.now(),
title: 'Pin Two',
unreadCount: 1,
isSelected: false,
typingContact: {
name: 'Someone There',
color: 'blue',
phoneNumber: '+18005551111',
},
acceptedMessageRequest: true,
},
pin3: {
id: 'pin3',
e164: '+18005551111',
name: 'Pin Three',
timestamp: 30,
inboxPosition: 30,
phoneNumber: 'notused',
isArchived: false,
isPinned: true,
markedUnread: false,
type: 'direct',
isMe: false,
lastUpdated: Date.now(),
title: 'Pin Three',
unreadCount: 1,
isSelected: false,
typingContact: {
name: 'Someone There',
color: 'blue',
phoneNumber: '+18005551111',
},
acceptedMessageRequest: true,
},
pin1: {
id: 'pin1',
e164: '+18005551111',
name: 'Pin One',
timestamp: 30,
inboxPosition: 30,
phoneNumber: 'notused',
isArchived: true,
isPinned: true,
markedUnread: false,
type: 'direct',
isMe: false,
lastUpdated: Date.now(),
title: 'Pin One',
unreadCount: 1,
isSelected: false,
typingContact: {
name: 'Someone There',
color: 'blue',
phoneNumber: '+18005551111',
},
acceptedMessageRequest: true,
},
pin4: {
id: 'pin1',
e164: '+18005551111',
name: 'Pin Four',
timestamp: 30,
inboxPosition: 30,
phoneNumber: 'notused',
activeAt: Date.now(),
isArchived: true,
isPinned: false,
markedUnread: false,
type: 'direct',
isMe: false,
lastUpdated: Date.now(),
title: 'Pin One',
unreadCount: 1,
isSelected: false,
typingContact: {
name: 'Someone There',
color: 'blue',
phoneNumber: '+18005551111',
},
acceptedMessageRequest: true,
},
pin5: {
id: 'pin1',
e164: '+18005551111',
name: 'Pin Five',
timestamp: 30,
inboxPosition: 30,
phoneNumber: 'notused',
isArchived: false,
isPinned: false,
markedUnread: false,
type: 'direct',
isMe: false,
lastUpdated: Date.now(),
title: 'Pin One',
unreadCount: 1,
isSelected: false,
typingContact: {
name: 'Someone There',
color: 'blue',
phoneNumber: '+18005551111',
},
acceptedMessageRequest: true,
},
};
const pinnedConversationIds = ['pin1', 'pin2', 'pin3'];
const comparator = _getConversationComparator();
const {
pinnedConversations,
archivedConversations,
} = _getLeftPaneLists(
data,
comparator,
undefined,
pinnedConversationIds
);
assert.strictEqual(pinnedConversations[0].name, 'Pin One');
assert.strictEqual(pinnedConversations[1].name, 'Pin Two');
assert.strictEqual(pinnedConversations[2].name, 'Pin Three');
assert.strictEqual(pinnedConversations.length, 3);
assert.strictEqual(archivedConversations[0].name, 'Pin Four');
assert.strictEqual(archivedConversations.length, 1);
});
});
});