[jitsi-dev] [jitsi] Fix isPrivate check in case only one user is in the room. (#21)


#1

I believe some return values might be swapped, but as always, correct me if I'm wrong. CC'ed Hristo, I believe he might be familiar with this due to the recent MUC UI changes.

I am loading a chatroom with 1 user (other than myself) and it shows up without a contact list. Tracing through the code I reach a method 'ConferenceChatSession: boolean isContactListSupported()'. I believe
this is the point where is being determined whether or not to show this column with chat room members.

MUCService.isPrivate(chatroom) is used to determine whether or not the chatroom is "private".
The current implementation of this claims that:
- if there is one chat room member and the local user is it, then it is not a private room,
- while if there's one user and it's someone else, the room is private.

So ... the return values are swapped(?)
You can merge this Pull Request by running:

  git pull https://github.com/cobratbq/jitsi fix-private-chat

Or you can view, comment on it, or merge it online at:

  https://github.com/jitsi/jitsi/pull/21

-- Commit Summary --

  * Fix isPrivate check in case only one user is in the room.

-- File Changes --

    M src/net/java/sip/communicator/service/muc/MUCService.java (4)

-- Patch Links --

https://github.com/jitsi/jitsi/pull/21.patch
https://github.com/jitsi/jitsi/pull/21.diff

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi/pull/21


#2

@@ -229,8 +229,8 @@ public static boolean isPrivate(ChatRoom chatRoom)
             {
                 for (ChatRoomMember member : chatRoom.getMembers())
                     if (nickname.equals(member.getName()))
- return false;

Hmm, I don't know whether the heuristic makes sense, but the implementation adheres to the documentation: If the user's nickname is equal to the name of the only member in the room, then the room is NOT private.

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi/pull/21/files#r11572460


#3

@@ -229,8 +229,8 @@ public static boolean isPrivate(ChatRoom chatRoom)
             {
                 for (ChatRoomMember member : chatRoom.getMembers())
                     if (nickname.equals(member.getName()))
- return false;

That'd be Hristo - and it was the reason I initially hoped he'd answer this.

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi/pull/21/files#r11576116


#4

Danny, I might be missing something, but given that this change solves your problem, then following the code, for your chatroom with one other member:
1. getMembersCount() returns 1
2. getMembers() includes a member with name equal to getUserNickname()

Is that intentional?

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi/pull/21#issuecomment-40347391


#5

Hi Danny, Ingo,

As I remember I just moved this method from ConferenceChatManager class and I'm not the author of it. I followed the code and I think that for the xmpp chat rooms this method will always return false because in the members list we include the local user. I'm not sure why we need that method at all. I'm not aware of any use case that we want to have a chat room without member list panel.

Is there anybody that knows why we are using this method?

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi/pull/21#issuecomment-40350979


#6

Danny, unless I get this wrong now, why not also add yourself to the room? Would make it consistent with XMPP (and other products, AFAIK).

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi/pull/21#issuecomment-40882558


#7

Well, I'll do that. I initially didn't
      do that, because the UI shows my own user name just above the
      contact list. Additionally, there are separate event triggers for
      local user events / changes. It seems like its redundant to also
      add myself as to the contact list.
      But other than "it seemed wrong" I don't have any objections, so
      if that's the way it works and the solution, then I'll happily add
      myself to the list.
      So "add myself to the list" it is. Will make the changes shortly.
      Thanks

···

On 04/20/2014 12:38 AM, Ingo Bauersachs wrote:
    
      Danny, unless I get this wrong now, why not also add yourself
        to the room? Would make it consistent with XMPP (and other
        products, AFAIK).
      —
        Reply to this email directly or view
          it on GitHub.

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi/pull/21#issuecomment-40883940


#8

@@ -229,8 +229,8 @@ public static boolean isPrivate(ChatRoom chatRoom)
             {
                 for (ChatRoomMember member : chatRoom.getMembers())
                     if (nickname.equals(member.getName()))
- return false;

    Hi Ingo,
    
      In src/net/java/sip/communicator/service/muc/MUCService.java:
      > @@ -229,8 +229,8 @@ public static boolean isPrivate(ChatRoom chatRoom)

             {
                 for (ChatRoomMember member : chatRoom.getMembers())
                     if (nickname.equals(member.getName()))
- return false;

      Hmm, I don't know whether the heuristic makes sense, but the
        implementation adheres to the documentation: If the user's
        nickname is equal to the name of the only member in the room,
        then the room is NOT private.

···

On 04/14/2014 08:39 AM, Ingo Bauersachs wrote:
      —
        Reply to this email directly or view
          it on GitHub.
    
    I'm currently modifying the implementation, so in that case we don't
    need the patch - as you pointed out.
    I am a bit confused though. If the normal way is to add yourself to
    the chat room, then the isPrivate check doesn't make sense either.
    1. The documentation says "... is private, i.e. represents a
    one-to-one conversation ...".
    2. The documentation continues to say "... and (2) a ChatRoom is
    private if it has only one ChatRoomMember who is not the local
    user." (I probably read this wrongly the first time.)
    This now implies that there are at least and at most 2 members: the
    local user + 1 other person (i.e. person that is not the local
    user). The implementation, however, starts by checking that the chat
    room member count = 1, which doesn't make sense if the local user
    should be added/counted.
    So, it looks like there's some inconsistency in the MUC
    implementation ...

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi/pull/21/files#r11816331


#9

@@ -229,8 +229,8 @@ public static boolean isPrivate(ChatRoom chatRoom)
             {
                 for (ChatRoomMember member : chatRoom.getMembers())
                     if (nickname.equals(member.getName()))
- return false;

I can't really follow anymore, but that's probably because I don't really know IRC (nor MUC in general). You initially said you open a chat room with one user (not yourself). Can you do that without joining, e.g. just to "watch" the channel? If you can, then, yes, isPrivate() is a problem. Otherwise I don't get it. There can be as many users as you like - as soon as there is more than one member, the room is no longer private.

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi/pull/21/files#r11819569


#10

Closed #21.

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi/pull/21#event-129172497


#11

I understand that it’s a bit vague if you are not familiar with the
code. What I mean is:
The service only ever considers a chat room as “private” if there is
exactly 1 member and that member is not me. However, you cannot have
one-to-one chats (isPrivate == true) when you are obligated to add
yourself and your counterpart chat room member.
Anyways, you’re right. Since none of us know exactly how it was
supposed to work, I’m just going to take the approach you suggested,
i.e. add myself as a chat room member to the list, and then we
shouldn’t need to modify isPrivate. I’ll take a look at isPrivate
when I stumble upon it again.

···

On 04/21/2014 08:36 PM, Ingo Bauersachs
wrote:

In src/net/java/sip/communicator/service/muc/MUCService.java:

> @@ -229,8 +229,8 @@ public static boolean isPrivate(ChatRoom chatRoom)
> {
> for (ChatRoomMember member : chatRoom.getMembers())
> if (nickname.equals(member.getName()))
> - return false;
    I can't really follow anymore, but that's probably because I

don’t really know IRC (nor MUC in general). You initially said
you open a chat room with one user (not yourself). Can you do
that without joining, e.g. just to “watch” the channel? If you
can, then, yes, isPrivate() is a problem. Otherwise I don’t get
it. There can be as many users as you like - as soon as there is
more than one member, the room is no longer private.

    Reply to this email directly or [          view

it on GitHub](https://github.com/jitsi/jitsi/pull/21/files#r11819569).


_______________________________________________
dev mailing list
Unsubscribe instructions and other list options:

dev@jitsi.orghttp://lists.jitsi.org/mailman/listinfo/dev