Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix_function_room.member_#173 #211

Merged
merged 29 commits into from
Jan 24, 2017
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
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
9 changes: 4 additions & 5 deletions src/puppet-web/firer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -207,7 +207,7 @@ async function checkRoomJoin(m: Message): Promise<void> {
const loaded = inviteeContactList[i] instanceof Contact

if (!loaded) {
let c = room.member(inviteeList[i])
let c = room.member({remark: inviteeList[i]}) || room.member({nick: inviteeList[i]})
Copy link
Member

Choose a reason for hiding this comment

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

We can keep room.member(inviteeList[i]) here, after you follow the review for member() parameters suggestion.

Copy link
Member Author

@lijiarui lijiarui Jan 18, 2017

Choose a reason for hiding this comment

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

We can keep room.member(inviteeList[i]) here, after you follow the review for member() parameters suggestion.

I don't agree with this suggestion, because of the following reason:

We keep function room.member() work, but it should work only with the function find member by nickName.
We cannot use it both find remark and nickName, because one can set a remark same as nick, then it would be confused.

So, what do you think about it?

if (!c) {
inviteeListAllDone = false
continue
Expand All @@ -234,7 +234,7 @@ async function checkRoomJoin(m: Message): Promise<void> {
}

if (!inviterContact) {
inviterContact = room.member(inviter)
inviterContact = room.member({remark: inviter}) || room.member({nick: inviter})
Copy link
Member

Choose a reason for hiding this comment

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

We can keep room.member(inviteeList[i]) here, after you follow the review for member() parameters suggestion.

Copy link
Member Author

Choose a reason for hiding this comment

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

We can keep room.member(inviteeList[i]) here, after you follow the review for member() parameters suggestion.

I don't agree with this suggestion, because of the following reason:

We keep function room.member() work, but it should work only with the function find member by nickName.
We cannot use it both find remark and nickName, because one can set a remark same as nick, then it would be confused.

So, what do you think about it?

}

if (inviteeListAllDone && inviterContact) {
Expand Down Expand Up @@ -316,7 +316,7 @@ async function checkRoomLeave(m: Message): Promise<void> {
/**
* FIXME: leaver maybe is a list
*/
let leaverContact = room.member(leaver)
let leaverContact = room.member({remark: leaver}) || room.member({nick: leaver})
Copy link
Member

Choose a reason for hiding this comment

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

We can keep room.member(leaver) here, after you follow the review for member() parameters suggestion.

Copy link
Member Author

Choose a reason for hiding this comment

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

We can keep room.member(inviteeList[i]) here, after you follow the review for member() parameters suggestion.

I don't agree with this suggestion, because of the following reason:

We keep function room.member() work, but it should work only with the function find member by nickName.
We cannot use it both find remark and nickName, because one can set a remark same as nick, then it would be confused.

So, what do you think about it?


if (!leaverContact) {
log.error('PuppetWebFirer', 'fireRoomLeave() leaver %s not found, event `room-leave` & `leave` will not be fired')
Expand Down Expand Up @@ -354,7 +354,6 @@ async function checkRoomTopic(m: Message): Promise<void> {
} catch (e) { // not found
return
}

const room = m.room()
if (!room) {
log.warn('PuppetWebFirer', 'fireRoomLeave() room not found')
Expand All @@ -367,7 +366,7 @@ async function checkRoomTopic(m: Message): Promise<void> {
if (/^You$/.test(changer) || /^你$/.test(changer)) {
changerContact = Contact.load(this.userId)
} else {
changerContact = room.member(changer)
changerContact = room.member({remark: changer}) || room.member({nick: changer})
Copy link
Member

Choose a reason for hiding this comment

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

We can keep room.member(changer) here, after you follow the review for member() parameters suggestion.

Copy link
Member Author

Choose a reason for hiding this comment

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

We can keep room.member(inviteeList[i]) here, after you follow the review for member() parameters suggestion.

I don't agree with this suggestion, because of the following reason:

We keep function room.member() work, but it should work only with the function find member by nickName.
We cannot use it both find remark and nickName, because one can set a remark same as nick, then it would be confused.

So, what do you think about it?

Copy link
Member

@huan huan Jan 18, 2017

Choose a reason for hiding this comment

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

Why you copy/paste the same answer four times here?

I comment more than one times because I want to point out the position for you where needs to change.

What you were arguing about is not wrong, but it's not entirely right. Remember always to KISS(Keep It Simple, Stupid).

The most scenario is we just want to get a member by name(whatever remark/nick/display). I do not agree with you that "it should work only with the function find the member by nickname", instead it should return a contact with name match nick/display/remark. And If "one set a remark same as nick", then it's the time to use the new QueryFilter as the parameter to be more precise.

So just change it to what I said.

Copy link
Member Author

@lijiarui lijiarui Jan 19, 2017

Choose a reason for hiding this comment

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

I have two questions:

1. room.member() should use which order?

I have found the rules of the name display order in wechat group's in #173 as follows:

Display order:
@ Event: displayName > nickName
system message remark > nickName
on-screen Names:remark > displayName > nickName

I suggest choosing on-screen Name, because it contains system message and it is useful in firer.ts

2. Now room.member can just return one contact, should it return a contact list?

}

if (!changerContact) {
Expand Down
107 changes: 87 additions & 20 deletions src/room.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ type RoomObj = {
ownerUin: number
memberList: Contact[]
nickMap: Map<string, string>
remarkMap: Map<string, string>
displayMap: Map<string, string>
}

export type RoomRawMember = {
Expand All @@ -52,6 +54,12 @@ export type RoomQueryFilter = {
topic: string | RegExp
}

export type MemberQueryFilter = {
nick?: string
remark?: string
display?: string
}

export class Room extends EventEmitter implements Sayable {
private static pool = new Map<string, Room>()

Expand Down Expand Up @@ -90,6 +98,15 @@ export class Room extends EventEmitter implements Sayable {
// return this.load(contactGetter)
// }

private async readyAllMembers(memberList: RoomRawMember[]): Promise<void> {
let contact: Contact
Copy link
Member

Choose a reason for hiding this comment

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

get rid of this line

for (let member of memberList) {
contact = Contact.load(member.UserName)
Copy link
Member

@huan huan Jan 18, 2017

Choose a reason for hiding this comment

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

change this line to:
let contact = Contact.load(member.UserName)
because it's only used inside this closure.

await contact.ready()
}
return
}

public async ready(contactGetter?: (id: string) => Promise<any>): Promise<void> {
log.silly('Room', 'ready(%s)', contactGetter ? contactGetter.constructor.name : '')
if (!this.id) {
Expand All @@ -114,6 +131,7 @@ export class Room extends EventEmitter implements Sayable {
const data = await contactGetter(this.id)
log.silly('Room', `contactGetter(${this.id}) resolved`)
this.rawObj = data
await this.readyAllMembers(this.rawObj.MemberList)
Copy link
Member

Choose a reason for hiding this comment

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

good

this.obj = this.parse(data)

if (!this.obj) {
Expand Down Expand Up @@ -190,6 +208,8 @@ export class Room extends EventEmitter implements Sayable {

const memberList = this.parseMemberList(rawObj.MemberList)
const nickMap = this.parseNickMap(rawObj.MemberList)
const remarkMap = this.parseRemarkMap(rawObj.MemberList)
const displayMap = this.parseDisplayName(rawObj.MemberList)
Copy link
Member

Choose a reason for hiding this comment

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

Instead of introduce new functions, use this.parseMap(memberList, 'display') directly at here.


return {
id: rawObj.UserName,
Expand All @@ -199,6 +219,8 @@ export class Room extends EventEmitter implements Sayable {

Copy link
Member

Choose a reason for hiding this comment

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

Please keep the original comments in the code, and add new comments if needed, for keep the history clear.(until someday we decide to clean them)

memberList,
nickMap,
remarkMap,
displayMap,
}
}

Expand All @@ -209,23 +231,47 @@ export class Room extends EventEmitter implements Sayable {
return rawMemberList.map(m => Contact.load(m.UserName))
}

/**
* ISSUE #64 emoji need to be striped
* ISSUE #104 never use remark name because sys group message will never use that
* @rui: cannot use argument NickName because it mix real nick and remark
*/
private parseNickMap(memberList: RoomRawMember[]): Map<string, string> {
const nickMap: Map<string, string> = new Map<string, string>()

let contact: Contact
Copy link
Member

Choose a reason for hiding this comment

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

move let to be inside the closure.

if (memberList && memberList.map) {
memberList.forEach(member => {
/**
* ISSUE #64 emoji need to be striped
* ISSUE #104 never use remark name because sys group message will never use that
*/
nickMap[member.UserName] = UtilLib.stripEmoji(
member.DisplayName || member.NickName
)
contact = Contact.load(member.UserName)
nickMap[member.UserName] = UtilLib.stripEmoji(contact.name())
})
}
return nickMap
}

private parseRemarkMap(memberList: RoomRawMember[]): Map<string, string> {
const remarkMap: Map<string, string> = new Map<string, string>()
let contact: Contact
let remark: string | null
Copy link
Member

Choose a reason for hiding this comment

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

move let to be inside the closure.

if (memberList && memberList.map) {
memberList.forEach(member => {
contact = Contact.load(member.UserName)
remark = contact.remark() || ''
remarkMap[member.UserName] = UtilLib.stripEmoji(remark)
})
}
return remarkMap
}

private parseDisplayName(memberList: RoomRawMember[]): Map<string, string> {
Copy link
Member

Choose a reason for hiding this comment

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

parseRemarkMap & parseDisplayName has many duplicated code.

Please consider to be not duplicated that much.

Copy link
Member Author

Choose a reason for hiding this comment

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

parseRemarkMap & parseDisplayName has many duplicated code.

/**
* ISSUE #64 emoji need to be striped
* ISSUE #104 never use remark name because sys group message will never use that
*/

Actually, parseRemarkMap & parseDisplayName & parseNickMap has many duplicated code. So should I not duplicated all? Then maybe I cannot keep the original comments in the code...

const displayMap: Map<string, string> = new Map<string, string>()
if (memberList && memberList.map) {
memberList.forEach(member => {
displayMap[member.UserName] = UtilLib.stripEmoji(member.DisplayName)
})
}
return displayMap
}

public dumpRaw() {
console.error('======= dump raw Room =======')
Object.keys(this.rawObj).forEach(k => console.error(`${k}: ${this.rawObj[k]}`))
Expand Down Expand Up @@ -354,28 +400,49 @@ export class Room extends EventEmitter implements Sayable {

return null
}

/**
* NickName / DisplayName / RemarkName of member
* find member by `nick`(NickName) / `display`(DisplayName) / `remark`(RemarkName)
* when use member(name:string), find member by nickName by default
Copy link
Member

Choose a reason for hiding this comment

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

Should by all name by default.

Copy link
Member

Choose a reason for hiding this comment

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

you missed this review.

*/
public member(name: string): Contact | null {
log.verbose('Room', 'member(%s)', name)
public member(queryArg: MemberQueryFilter | string): Contact | null {
Copy link
Member

Choose a reason for hiding this comment

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

Add function declarations before function implement like this:

+ public member(filter: MemberQueryFilter): Contact | null
+ public member(name: string): Contact | null
+ 
public member(queryArg: MemberQueryFilter | string): Contact | null {

let query: MemberQueryFilter
if (typeof queryArg === 'string') {
Copy link
Member

Choose a reason for hiding this comment

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

Need to support member(name: string) here, which name could be any of remark/display/nick.

log.error('Room', 'function member should use member(nick:name)')
query = { nick: queryArg }
} else {
Copy link
Member

@huan huan Jan 19, 2017

Choose a reason for hiding this comment

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

Remove else here, and get all the code in this block out.

query = queryArg
}

log.verbose('Room', 'member({ %s })'
Copy link
Member

Choose a reason for hiding this comment

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

use log.silly instead of verbose here.

verbose should be used only once per function, usually at the beginning of the function, to log the function is run and all parameters it has received.

Any other log should be silly

, Object.keys(query)
.map(k => `${k}: ${query[k]}`)
.join(', ')
)

if (Object.keys(query).length !== 1) {
throw new Error('Room member find quaryArg only support one key. multi key support is not availble now.')
}

if (!this.obj || !this.obj.memberList) {
log.warn('Room', 'member() not ready')
return null
}
let filterKey = Object.keys(query)[0]
let filterValue: string = UtilLib.stripEmoji(query[filterKey])

const keyMap = {
nick: 'nickMap',
remark: 'remarkMap',
display: 'displayMap'
}

/**
* ISSUE #64 emoji need to be striped
*/
name = UtilLib.stripEmoji(name)
filterKey = keyMap[filterKey]

const nickMap = this.obj.nickMap
const idList = Object.keys(nickMap)
.filter(k => nickMap[k] === name)
const filterMap = this.obj[filterKey]
const idList = Object.keys(filterMap)
.filter(k => filterMap[k] === filterValue)

log.silly('Room', 'member() check nickMap: %s', JSON.stringify(nickMap))
log.silly('Room', 'member() check %s: %s', filterKey, JSON.stringify(filterKey))

if (idList.length) {
return Contact.load(idList[0])
Expand Down