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

t3chguy · 2017-12-29T18:03:59Z

Fixes element-hq/element-web#5792 and probably others
Apologies for tiny other little nit fixes in the same commit

Signed-off-by: Michael Telatynski 7t3chguy@gmail.com

Edit:
now also excludes addresses which are already in the group/room

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

t3chguy · 2017-12-29T23:30:17Z

now that there's a generic thing for excluding addresses, should I remove the includeSelf prop, which doesn't appear to ever actually be used (no instance of this dialog ever has includeSelf:true)

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

lukebarnard1 · 2018-01-05T11:12:18Z

now that there's a generic thing for excluding addresses, should I remove the includeSelf prop, which doesn't appear to ever actually be used (no instance of this dialog ever has includeSelf:true)

Sure, go for it!

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

lukebarnard1

One tiny change but otherwise, this looks really good :)

I continue to be concerned about the AddressPicker API, which mandates use of strings "mx-room-id" and "mx-user-id". But refactoring that could be done another time I think.

lukebarnard1 · 2018-01-05T11:15:07Z

src/RoomInvite.js

@@ -63,10 +63,21 @@ export function showStartChatInviteDialog() {
 }

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


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

lukebarnard1 · 2018-01-05T11:24:17Z

src/components/views/dialogs/AddressPickerDialog.js

-                isKnown: true,
-            });
+            // Don't add it if it is in the excludedAddresses
+            if (this.props.excludedAddresses.find((entry) => {


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

this one could be optimized by changing the format of exludedAddresses to be a map, for constant time access

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

lukebarnard1 · 2018-01-05T11:24:53Z

src/components/views/dialogs/AddressPickerDialog.js

@@ -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) => {


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

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

t3chguy · 2018-01-05T11:29:53Z

I continue to be concerned about the AddressPicker API, which mandates use of strings "mx-room-id" and "mx-user-id". But refactoring that could be done another time I think.

what would you prefer/suggest?
enums/consts?

lukebarnard1 · 2018-01-05T11:36:55Z

Well for a thing that picks "addresses" it seems far to concerned about users and rooms. But I think last time I was worrying about this I concluded that we'd need a UserPicker and RoomPicker, both wrapping AddressPicker. But to be honest, I'm not convinced that this would actually make things easy to digest overall.

lukebarnard1

Just a tiny amount of comment feedback - otherwise LGTM

lukebarnard1 · 2018-01-05T11:57:08Z

src/GroupAddressPicker.js

+        excludedAddresses: groupStore.getGroupMembers().concat(groupStore.getGroupInvitedMembers()).map((member) => ({
+            addressType: 'mx-user-id',
+            address: member.userId,
+        })), // no need to add ourselves as we must already be in the group


"ourselves" is a bit ambiguous, I'd advise that when commenting it's best to talk about "the user" or "the members of the group" etc.

lukebarnard1 · 2018-01-05T11:57:37Z

src/RoomInvite.js

+        }).map((member) => ({
+            addressType: 'mx-user-id',
+            address: member.userId,
+        })), // no need to add ourselves as we must already be in the room


(see above)

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

dbkr · 2018-01-05T12:12:49Z

I'm actually not 100% sure on excluding users/rooms who already in the room/group: it won't be very obvious why they aren't appearing if you don't realise they've already been invited (or are already in the room). @ara4n @lampholder wdyt?

t3chguy · 2018-01-05T12:14:13Z

@dbkr how about graying them out in the dropdown instead then, with hoverover tooltip?

dbkr · 2018-01-08T14:40:36Z

Seems ok from my PoV, although @ara4n / @lampholder should probably weigh in.

lampholder · 2018-04-11T13:55:48Z

Hey @t3chguy could you fix the merge conflict and then I'll run it on lant.uk and test the UX? Ta!

t3chguy · 2018-06-21T08:34:22Z

Splitting this PR out.
#2000 is the first half
second half to follow

t3chguy added 5 commits December 29, 2017 15:07

Don't show addresses that have already been picked

cc05903

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

change check to be explicit such that both address and type must match

df7d545

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

Exclude rooms/users already in group in invite/add dialogs

029d9a5

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

Exclude rooms already in room in invite user to room dialog

ab14c0c

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

Exclude users/rooms already in group in GroupView/featured

01f4801

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

t3chguy added 2 commits December 29, 2017 23:33

fix typo and small layout tweaks

7cc6243

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

fix comment typo

a7f317a

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

t3chguy force-pushed the t3chguy/dedup_address_picker branch from 5b8ca7a to a7f317a Compare December 29, 2017 23:35

remove includeSelf and slight refactor

a73715e

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

lukebarnard1 suggested changes Jan 5, 2018

View reviewed changes

t3chguy added 2 commits January 5, 2018 11:26

now that includeSelf (def=false) is gone, use generic replacement

cae812c

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

remove redundant var cli

7ff2ef3

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

t3chguy assigned lukebarnard1 Jan 5, 2018

lukebarnard1 suggested changes Jan 5, 2018

View reviewed changes

improve comments

a27bafd

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

lukebarnard1 approved these changes Jan 5, 2018

View reviewed changes

lukebarnard1 assigned t3chguy and unassigned lukebarnard1 Apr 23, 2018

t3chguy mentioned this pull request Jun 20, 2018

hide already chosen results from AddressPickerDialog #2000

Merged

t3chguy added the blocked label Jun 21, 2018

t3chguy mentioned this pull request Jun 22, 2018

AddressPickerDialog mark addresses already in the room/group element-hq/element-web#6921

Open

t3chguy closed this Jun 22, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

t3chguy commented Dec 29, 2017 •

edited

Loading

t3chguy commented Dec 29, 2017

lukebarnard1 commented Jan 5, 2018

lukebarnard1 left a comment

lukebarnard1 Jan 5, 2018

lukebarnard1 Jan 5, 2018

t3chguy Jan 5, 2018

lukebarnard1 Jan 5, 2018

lukebarnard1 Jan 5, 2018

t3chguy commented Jan 5, 2018 •

edited

Loading

lukebarnard1 commented Jan 5, 2018

lukebarnard1 left a comment

lukebarnard1 Jan 5, 2018

lukebarnard1 Jan 5, 2018

dbkr commented Jan 5, 2018

t3chguy commented Jan 5, 2018

dbkr commented Jan 8, 2018

lampholder commented Apr 11, 2018

t3chguy commented Jun 21, 2018 •

edited

Loading

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

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

Conversation

t3chguy commented Dec 29, 2017 • edited Loading

t3chguy commented Dec 29, 2017

lukebarnard1 commented Jan 5, 2018

lukebarnard1 left a comment

Choose a reason for hiding this comment

lukebarnard1 Jan 5, 2018

Choose a reason for hiding this comment

lukebarnard1 Jan 5, 2018

Choose a reason for hiding this comment

t3chguy Jan 5, 2018

Choose a reason for hiding this comment

lukebarnard1 Jan 5, 2018

Choose a reason for hiding this comment

lukebarnard1 Jan 5, 2018

Choose a reason for hiding this comment

t3chguy commented Jan 5, 2018 • edited Loading

lukebarnard1 commented Jan 5, 2018

lukebarnard1 left a comment

Choose a reason for hiding this comment

lukebarnard1 Jan 5, 2018

Choose a reason for hiding this comment

lukebarnard1 Jan 5, 2018

Choose a reason for hiding this comment

dbkr commented Jan 5, 2018

t3chguy commented Jan 5, 2018

dbkr commented Jan 8, 2018

lampholder commented Apr 11, 2018

t3chguy commented Jun 21, 2018 • edited Loading

t3chguy commented Dec 29, 2017 •

edited

Loading

t3chguy commented Jan 5, 2018 •

edited

Loading

t3chguy commented Jun 21, 2018 •

edited

Loading