[sip-comm-dev] [patch] a little jumble


#1

Hi all, Yana,

This is a series of little modifications which touch several parts of code. I want be sure there isn't
objections before commiting it. I will explain things in the order they appear in the patch file.

first, I added a method
    public Contact getDefaultContact(Class operationSet);
in the metacontact interface and it's implementation based on
    public Contact getDefaultContact();

The purpose is to obtain a contact for a specific operation. For example, if
we right click on a metacontact and we choose send file, we can simply
send the file to the contact returned by
    metacontact.getDefaultContact(OperationSetFileTransfer.class);

In ImageLoader.java, I added a static field
public static final ImageLoader.ImageID CALL_16x16_ICON
            = new ImageID("SIP_COMMUNICATOR_LOGO");
the image is temporary set to SIP_COMMUNICATOR_LOGO.
It is used later for the right button menu over a metacontact where I added a call menu item.
Yana : I see that there is already some icons which could fit here. Could you
help me to pick one ?

ChatSendPanel.java
   fix a bug discovered long time ago with Emil and Yana which cause
exceptions when there isn't sufficient place in the status label
(where typing notification are shown) to fit the status message.

ChatContactPanel.java
    deactivate the action icon on top of the user name which appears on the right
side of a chat window if action is not supported.

ContactRightButtonMenu.java
    add a call menu item

ContactList.java
    popup the right menu at the right coordinates. Currently, it popups only at the good
height.

CallManager.java
    private Contact getTelephonyContact(MetaContact metaContact)
now returns metaContact.getDefaultContact(OperationSetBasicTelephony.class);
rather than the first found telephony contact.
Yana : I think if you agree with this point, the method could be suppressed in favor of
a direct call to metaContact.getDefaultContact(OperationSetBasicTelephony.class);
wherever it is needed.

added method
    public void createCall(Contact contact)

CallPanel.java
    added a new constructor
    public CallPanel(CallManager callManager, Contact contact)

NightlyBuildID.java : replaced "CVS" by "Sam" as it more kind
    well, will remove it before committing :slight_smile:
    This said, it should be SVN, isn't it ?

OperationSetMultiUserChatJabberImpl.java
        public ChatRoom findRoom(String roomName)
returns createLocalChatRoomInstance if the room searched for
doesn't exist, but, in TestOperationSetMultiUserChatJabberImpl,
findRoom is supposed to return null in case of an inexisting room. I assumed that
the test expectation was the right behaviour. Isn't it ?

OperationSetBasicTelephonyJabberImpl.java
    cleanup, replaced the null stun server with "stun.iptel.org".
The stun server provided here is temporary but, isn't better to have
a valid server (even if the remote machine is down) rather than using
a null one ? And I think anything is better than null for good.

Well that's all. These modifications aren't all related, but as they aren't substancial,
I preferred to put them all in a mail. If someone want to discuss only a particular
point there isn't any obligation to care about others :slight_smile:

Regards
    Sympho.

sip-comm.patch (24.6 KB)


#2

Hi Sympho,

see inline.

Sympho wrote:

Hi all, Yana,

This is a series of little modifications which touch several parts of code. I want be sure there isn't
objections before commiting it. I will explain things in the order they appear in the patch file.

first, I added a method
   public Contact getDefaultContact(Class operationSet);
in the metacontact interface and it's implementation based on
   public Contact getDefaultContact();

The purpose is to obtain a contact for a specific operation. For example, if
we right click on a metacontact and we choose send file, we can simply
send the file to the contact returned by
   metacontact.getDefaultContact(OperationSetFileTransfer.class);

Great.

In ImageLoader.java, I added a static field
public static final ImageLoader.ImageID CALL_16x16_ICON
           = new ImageID("SIP_COMMUNICATOR_LOGO");
the image is temporary set to SIP_COMMUNICATOR_LOGO.
It is used later for the right button menu over a metacontact where I added a call menu item.
Yana : I see that there is already some icons which could fit here. Could you
help me to pick one ?

May be the most appropriate icon is the one used for sip online status, but don't use it from its current location (PROJECT_DIR/resources/images/sip), instead you could copy it in the gui/images/common/ under another more appropriate name. Like this I could change it later.

ChatSendPanel.java
  fix a bug discovered long time ago with Emil and Yana which cause
exceptions when there isn't sufficient place in the status label
(where typing notification are shown) to fit the status message.

Ok.

ChatContactPanel.java
   deactivate the action icon on top of the user name which appears on the right
side of a chat window if action is not supported.

Great.

ContactRightButtonMenu.java
   add a call menu item

Ok.

ContactList.java
   popup the right menu at the right coordinates. Currently, it popups only at the good
height.

Actually this is not a bug, I thought it's looking better like this. Please wait till I give it one more try before committing:)

CallManager.java
   private Contact getTelephonyContact(MetaContact metaContact)
now returns metaContact.getDefaultContact(OperationSetBasicTelephony.class);
rather than the first found telephony contact.
Yana : I think if you agree with this point, the method could be suppressed in favor of
a direct call to metaContact.getDefaultContact(OperationSetBasicTelephony.class);
wherever it is needed.

I agree.

added method
   public void createCall(Contact contact)

Actually there's already a method createCall(Vector contacts), which is meant to be used for selected contacts. The idea is to have the possibility of a multiple selection, which creates a conference call. I think it should be used even if we have only one selected contact. However it could be changed to private and maybe instead of Vector it's better to pass a List.

CallPanel.java
   added a new constructor
   public CallPanel(CallManager callManager, Contact contact)

The same. There's already a constructor, which takes a list of contacts.

NightlyBuildID.java : replaced "CVS" by "Sam" as it more kind
   well, will remove it before committing :slight_smile:
   This said, it should be SVN, isn't it ?

I don't see where is the CVS thing:)

OperationSetMultiUserChatJabberImpl.java
       public ChatRoom findRoom(String roomName)
returns createLocalChatRoomInstance if the room searched for
doesn't exist, but, in TestOperationSetMultiUserChatJabberImpl,
findRoom is supposed to return null in case of an inexisting room. I assumed that
the test expectation was the right behaviour. Isn't it ?

Actually it's not so simple. The findRoom is meant to search for a room on the server and not only in the local rooms cache. You could see that
createLocalChatRoomInstance takes a MultiUserChat instance, so if there's no such room on the server it will throw an OperationFailedException, when trying to create the multi user chat.

OperationSetBasicTelephonyJabberImpl.java
   cleanup, replaced the null stun server with "stun.iptel.org".
The stun server provided here is temporary but, isn't better to have
a valid server (even if the remote machine is down) rather than using
a null one ? And I think anything is better than null for good.

I'm not a specialist in this, but it sounds me right:)

Thanks Sympho!

You have done a really good work!

Yana

···

Well that's all. These modifications aren't all related, but as they aren't substancial,
I preferred to put them all in a mail. If someone want to discuss only a particular
point there isn't any obligation to care about others :slight_smile:

Regards
   Sympho.

------------------------------------------------------------------------

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@sip-communicator.dev.java.net
For additional commands, e-mail: dev-help@sip-communicator.dev.java.net

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@sip-communicator.dev.java.net
For additional commands, e-mail: dev-help@sip-communicator.dev.java.net


#3

Hey Sympho,

When I wrote my mail I didn't see that you've already committed it all. When you are asking for confirmation, which is a very good idea when you're changing someone else's code, you could also wait for a response:)

Could you please have a look at my comments and you should may be turn back to the previous version for some of the changes you've made. (For example the OperationSetMultiUserChatJabberImpl should definitely be turned).

Thanks,
Yana

Yana Stamcheva wrote:

···

Hi Sympho,

see inline.

Sympho wrote:

Hi all, Yana,

This is a series of little modifications which touch several parts of code. I want be sure there isn't
objections before commiting it. I will explain things in the order they appear in the patch file.

first, I added a method
   public Contact getDefaultContact(Class operationSet);
in the metacontact interface and it's implementation based on
   public Contact getDefaultContact();

The purpose is to obtain a contact for a specific operation. For example, if
we right click on a metacontact and we choose send file, we can simply
send the file to the contact returned by
   metacontact.getDefaultContact(OperationSetFileTransfer.class);

Great.

In ImageLoader.java, I added a static field
public static final ImageLoader.ImageID CALL_16x16_ICON
           = new ImageID("SIP_COMMUNICATOR_LOGO");
the image is temporary set to SIP_COMMUNICATOR_LOGO.
It is used later for the right button menu over a metacontact where I added a call menu item.
Yana : I see that there is already some icons which could fit here. Could you
help me to pick one ?

May be the most appropriate icon is the one used for sip online status, but don't use it from its current location (PROJECT_DIR/resources/images/sip), instead you could copy it in the gui/images/common/ under another more appropriate name. Like this I could change it later.

ChatSendPanel.java
  fix a bug discovered long time ago with Emil and Yana which cause
exceptions when there isn't sufficient place in the status label
(where typing notification are shown) to fit the status message.

Ok.

ChatContactPanel.java
   deactivate the action icon on top of the user name which appears on the right
side of a chat window if action is not supported.

Great.

ContactRightButtonMenu.java
   add a call menu item

Ok.

ContactList.java
   popup the right menu at the right coordinates. Currently, it popups only at the good
height.

Actually this is not a bug, I thought it's looking better like this. Please wait till I give it one more try before committing:)

CallManager.java
   private Contact getTelephonyContact(MetaContact metaContact)
now returns metaContact.getDefaultContact(OperationSetBasicTelephony.class);
rather than the first found telephony contact.
Yana : I think if you agree with this point, the method could be suppressed in favor of
a direct call to metaContact.getDefaultContact(OperationSetBasicTelephony.class);
wherever it is needed.

I agree.

added method
   public void createCall(Contact contact)

Actually there's already a method createCall(Vector contacts), which is meant to be used for selected contacts. The idea is to have the possibility of a multiple selection, which creates a conference call. I think it should be used even if we have only one selected contact. However it could be changed to private and maybe instead of Vector it's better to pass a List.

CallPanel.java
   added a new constructor
   public CallPanel(CallManager callManager, Contact contact)

The same. There's already a constructor, which takes a list of contacts.

NightlyBuildID.java : replaced "CVS" by "Sam" as it more kind
   well, will remove it before committing :slight_smile:
   This said, it should be SVN, isn't it ?

I don't see where is the CVS thing:)

OperationSetMultiUserChatJabberImpl.java
       public ChatRoom findRoom(String roomName)
returns createLocalChatRoomInstance if the room searched for
doesn't exist, but, in TestOperationSetMultiUserChatJabberImpl,
findRoom is supposed to return null in case of an inexisting room. I assumed that
the test expectation was the right behaviour. Isn't it ?

Actually it's not so simple. The findRoom is meant to search for a room on the server and not only in the local rooms cache. You could see that
createLocalChatRoomInstance takes a MultiUserChat instance, so if there's no such room on the server it will throw an OperationFailedException, when trying to create the multi user chat.

OperationSetBasicTelephonyJabberImpl.java
   cleanup, replaced the null stun server with "stun.iptel.org".
The stun server provided here is temporary but, isn't better to have
a valid server (even if the remote machine is down) rather than using
a null one ? And I think anything is better than null for good.

I'm not a specialist in this, but it sounds me right:)

Thanks Sympho!

You have done a really good work!

Yana

Well that's all. These modifications aren't all related, but as they aren't substancial,
I preferred to put them all in a mail. If someone want to discuss only a particular
point there isn't any obligation to care about others :slight_smile:

Regards
   Sympho.

------------------------------------------------------------------------

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@sip-communicator.dev.java.net
For additional commands, e-mail: dev-help@sip-communicator.dev.java.net

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@sip-communicator.dev.java.net
For additional commands, e-mail: dev-help@sip-communicator.dev.java.net

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@sip-communicator.dev.java.net
For additional commands, e-mail: dev-help@sip-communicator.dev.java.net


#4

Hi Yana and thanks for your reply,

Yana Stamcheva a �crit :
> Hey Sympho,
>
> When I wrote my mail I didn't see that you've already committed it all. When you are asking for confirmation, which is a very good idea when you're changing someone else's code, you could also wait for a response:)
>

Sorry for the haste :frowning:
I will calm down myself and try to avoid rush next time :slight_smile:

> Could you please have a look at my comments and you should may be turn back to the previous version for some of the changes you've made. (For example the OperationSetMultiUserChatJabberImpl should definitely be turned).

I haven't modified the behaviour here as I saw that opsets of others protocols act in the same way.
only added comments.

see inline for the rest
>
> Thanks,
> Yana
>
> Yana Stamcheva wrote:
>> Hi Sympho,
>>
>> see inline.
>>
>> Sympho wrote:
>>> Hi all, Yana,
>>>
>>> This is a series of little modifications which touch several parts of code. I want be sure there isn't
>>> objections before commiting it. I will explain things in the order they appear in the patch file.
>>>
>>> first, I added a method
>>> public Contact getDefaultContact(Class operationSet);
>>> in the metacontact interface and it's implementation based on
>>> public Contact getDefaultContact();
>>>
>>> The purpose is to obtain a contact for a specific operation. For example, if
>>> we right click on a metacontact and we choose send file, we can simply
>>> send the file to the contact returned by
>>> metacontact.getDefaultContact(OperationSetFileTransfer.class);
>>
>> Great.
>>
>>>
>>> In ImageLoader.java, I added a static field
>>> public static final ImageLoader.ImageID CALL_16x16_ICON
>>> = new ImageID("SIP_COMMUNICATOR_LOGO");
>>> the image is temporary set to SIP_COMMUNICATOR_LOGO.
>>> It is used later for the right button menu over a metacontact where I added a call menu item.
>>> Yana : I see that there is already some icons which could fit here. Could you
>>> help me to pick one ?
>>
>> May be the most appropriate icon is the one used for sip online status, but don't use it from its current location (PROJECT_DIR/resources/images/sip), instead you could copy it in the gui/images/common/ under another more appropriate name. Like this I could change it later.
>>

done.

>>>
>>> ChatSendPanel.java
>>> fix a bug discovered long time ago with Emil and Yana which cause
>>> exceptions when there isn't sufficient place in the status label
>>> (where typing notification are shown) to fit the status message.
>>
>> Ok.
>>
>>> ChatContactPanel.java
>>> deactivate the action icon on top of the user name which appears on the right
>>> side of a chat window if action is not supported.
>>
>> Great.
>>
>>> ContactRightButtonMenu.java
>>> add a call menu item
>>
>> Ok.
>>
>>>
>>> ContactList.java
>>> popup the right menu at the right coordinates. Currently, it popups only at the good
>>> height.
>>
>> Actually this is not a bug, I thought it's looking better like this. Please wait till I give it one more try before committing:)
>>

Ok, it's just that it surprised me a little, when I clicked on the middle of a meta contact label
with the SC main window maximized. The menu popepup really far from where I expected, and
as mouse menus usually opens where the user click, I haven't thought it was intentionally.

>>>
>>> CallManager.java
>>> private Contact getTelephonyContact(MetaContact metaContact)
>>> now returns metaContact.getDefaultContact(OperationSetBasicTelephony.class);
>>> rather than the first found telephony contact.
>>> Yana : I think if you agree with this point, the method could be suppressed in favor of
>>> a direct call to metaContact.getDefaultContact(OperationSetBasicTelephony.class);
>>> wherever it is needed.
>>
>> I agree.
>>
>>>
>>> added method
>>> public void createCall(Contact contact)
>>
>> Actually there's already a method createCall(Vector contacts), which is meant to be used for selected contacts. The idea is to have the possibility of a multiple selection, which creates a conference call. I think it should be used even if we have only one selected contact. However it could be changed to private and maybe instead of Vector it's better to pass a List.
>>>
>>> CallPanel.java
>>> added a new constructor
>>> public CallPanel(CallManager callManager, Contact contact)
>>
>> The same. There's already a constructor, which takes a list of contacts.
>>

In fact, I thought of adding that method in CallManager and modify
CallPanel accordingly only because the underlying telephony opset interface has also two methods
    createCall(String)
    createCall(Contact)
and as calling a single contact is probably the most common scenario, I felt that it was a little
"cumbersome" to create a vector (or a list) each time we have to do that task.

>>>
>>> NightlyBuildID.java : replaced "CVS" by "Sam" as it more kind
>>> well, will remove it before committing :slight_smile:
>>> This said, it should be SVN, isn't it ?
>>
>> I don't see where is the CVS thing:)
>>
>>>
>>> OperationSetMultiUserChatJabberImpl.java
>>> public ChatRoom findRoom(String roomName)
>>> returns createLocalChatRoomInstance if the room searched for
>>> doesn't exist, but, in TestOperationSetMultiUserChatJabberImpl,
>>> findRoom is supposed to return null in case of an inexisting room. I assumed that
>>> the test expectation was the right behaviour. Isn't it ?
>>
>> Actually it's not so simple. The findRoom is meant to search for a room on the server and not only in the local rooms cache. You could see that
>> createLocalChatRoomInstance takes a MultiUserChat instance, so if there's no such room on the server it will throw an OperationFailedException, when trying to create the multi user chat.
>>

At this point, I reverted modifications which may need more discussions.
And once again, thanks for your replies.

    Sympho

>>>
>>> OperationSetBasicTelephonyJabberImpl.java
>>> cleanup, replaced the null stun server with "stun.iptel.org".
>>> The stun server provided here is temporary but, isn't better to have
>>> a valid server (even if the remote machine is down) rather than using
>>> a null one ? And I think anything is better than null for good.
>>
>> I'm not a specialist in this, but it sounds me right:)
>>
>> Thanks Sympho!
>>
>> You have done a really good work!
>>
>> Yana
>>
>>>
>>> Well that's all. These modifications aren't all related, but as they aren't substancial,
>>> I preferred to put them all in a mail. If someone want to discuss only a particular

···

point there isn't any obligation to care about others :slight_smile:

>>>
>>> Regards
>>> Sympho.
>>>
>>> ------------------------------------------------------------------------
>>>
>>> ---------------------------------------------------------------------
>>> To unsubscribe, e-mail: dev-unsubscribe@sip-communicator.dev.java.net
>>> For additional commands, e-mail: dev-help@sip-communicator.dev.java.net
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: dev-unsubscribe@sip-communicator.dev.java.net
>> For additional commands, e-mail: dev-help@sip-communicator.dev.java.net
>>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@sip-communicator.dev.java.net
> For additional commands, e-mail: dev-help@sip-communicator.dev.java.net
>

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@sip-communicator.dev.java.net
For additional commands, e-mail: dev-help@sip-communicator.dev.java.net


#5

Hi Sympho,

Ok, it's just that it surprised me a little, when I clicked on the middle of a meta contact label
with the SC main window maximized. The menu popepup really far from where I expected, and
as mouse menus usually opens where the user click, I haven't thought it was intentionally.

Actually you're right, it's better as you made it. I just wanted to have
a look.

In fact, I thought of adding that method in CallManager and modify
CallPanel accordingly only because the underlying telephony opset interface has also two methods
   createCall(String)
   createCall(Contact)
and as calling a single contact is probably the most common scenario, I felt that it was a little
"cumbersome" to create a vector (or a list) each time we have to do that task.

You're right that the Vector is useless, but we get an array of selected objects and we could pass it directly to the createCall method. I don't see the difference though, in terms of performance, because we'll always have to consider two cases (single and multiple selection) and this code is executed only when you click on the call button. I think that if we add another method this will only complicate the code.

Regards,
Yana

···

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@sip-communicator.dev.java.net
For additional commands, e-mail: dev-help@sip-communicator.dev.java.net