> On July 21, 2011, 4:21 a.m., Boroondas Gupte wrote: > > indra/newview/llparticipantlist.cpp, lines 807-808 > > <http://codereview.secondlife.com/r/404/diff/2/?file=6722#file6722line807> > > > > Hmm ... looking at the comment at > > https://bitbucket.org/lindenlab/viewer-development/src/d8c37b383028/indra/newview/llagent.h#cl-705 > > and the implementation of isInGroup at > > https://bitbucket.org/lindenlab/viewer-development/src/d8c37b383028/indra/newview/llagent.cpp#cl-2414 > > , this test actually checks whether the agent in question is a member of > > the group which has an UUID matching the conversation's session ID. > > > > Because the session IDs of group chats will match the UUID of the > > respective groups (and all other chat session types should have session IDs > > not matching any group UUIDs), this also tells us whether the session is a > > group chat. > > > > So maybe the comment should be yet different to reflect this. (Sorry, > > should have looked up this stuff for the first review already.) Maybe: > > // Is session a group chat? (It is if the session ID is the > > UUID of a group of which the agent is a member.) > > > > I wonder whether there is a better way (as in: making less assumptions > > / not using an agent as pivot) to test for this. > > Paul ProductEngine wrote: > I think the comment may be: "Is agent in group call/chat ?"
We don't really care here whether the *agent* is in a group call/chat, but whether the *session* here is that of a *group* call/chat (as opposed to resident-to-resident chat and ad-hoc chat sessions). We just (ab)use the agent to get a list of candidate groups one of which might have the session ID as UUID. At least, that's what the code here seems to do. If the intention is something else, we might have a bug here. Though it would make sense that this be in fact the intention, as the notion of "moderators" does not exist in ad-hoc and resi2resi chat sessions. > I wonder whether there is a better way (as in: making less assumptions / not > using an agent as pivot) to test for this. It looks like there isn't a better way, currently: LLIMModel::LLIMSession keeps track of the session type and offers this information through e.g. bool isGroupSessionType(), but LLParticipantList::LLParticipantListMenu seems neither directly nor indirectly (e.g. via mParent.mSpeakerMgr) aware of the LLIMSession it belongs to, so can't query that. Off course, some refactoring could solve that, but that's not in-scope for this crash fix. - Boroondas ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/404/#review898 ----------------------------------------------------------- On July 21, 2011, 3:27 a.m., Paul ProductEngine wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://codereview.secondlife.com/r/404/ > ----------------------------------------------------------- > > (Updated July 21, 2011, 3:27 a.m.) > > > Review request for Viewer. > > > Summary > ------- > > - Added null checking > > > This addresses bug storm-1458. > http://jira.secondlife.com/browse/storm-1458 > > > Diffs > ----- > > indra/newview/llparticipantlist.cpp d8c37b383028 > > Diff: http://codereview.secondlife.com/r/404/diff > > > Testing > ------- > > > Thanks, > > Paul > >
_______________________________________________ Policies and (un)subscribe information available here: http://wiki.secondlife.com/wiki/OpenSource-Dev Please read the policies before posting to keep unmoderated posting privileges
