[jitsi-dev] [jitsi] Fixes the contacts 'call button' disappearing on multiresource contacts (#161)


#1

This changes the behavior from "last connected resource" or "highest
priority resource" to "most capable resource" regarding the display of
buttons next to this contact. We now check every resource the contact
has connected and we will consider the one with the most capabilities.
This eliminates the very frustrating scenario in which the call
capabilities are disabled on a contact who connected additionally a
non-jingle client, thus resolves #111
You can view, comment on, or merge this pull request online at:

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

-- Commit Summary --

  * Fixes the contacts 'call button' disappearing on multiresource contacts
  * Correcting and completing Javadoc comments

-- File Changes --

    M src/net/java/sip/communicator/impl/protocol/jabber/MobileIndicator.java (8)
    M src/net/java/sip/communicator/impl/protocol/jabber/OperationSetContactCapabilitiesJabberImpl.java (77)
    M src/net/java/sip/communicator/impl/protocol/jabber/extensions/caps/EntityCapsManager.java (34)
    M src/net/java/sip/communicator/impl/protocol/jabber/extensions/caps/UserCapsNodeListener.java (10)

-- Patch Links --

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

···

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


#2

I'm going to send in the contributor agreement in the next few days.

···

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


#3

Thanks!
How does this behave now for two equally capable resources (e.g. chat)
- with the same priority
- with different priorities
- with different online statuses?

I'm a bit concerned about what happens when e.g. a mobile device resource is online and only chat capable and a desktop resource is call-capable, but away. The contact list shows the highest available online status, thus when you call a contact in this scenario it would ring on the desktop while said callee is potentially not even near his machine.

···

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


#4

My intention was to reliably show the call buttons as long as there is such a client connected. This is done by comparing the size of the determined set of capabilities of the resources. The largest one wins. Jitsi/Jitsi-android has 35, while Psi, Adium and ChatSecure have 31. If there are two sets with the exact same size, the first one would win. My assumption here is, that it's most likely the same set. So it shouldn't be affected at all by priorities or statuses, only by resources. The call connects to the highest prioritized call capable resource, even if there is a non-call-capable resource with a higher priority. So the case you described with a lower prioritized call capable resource set on 'away' being 'covered' by a higher prioritized resource set on 'online' is how it would behave. However Jitsi displays the callee's resource which is being called to the caller. And in the extended tool-tips the priority and online status of all resources is listed, so the caller could see
  the away status of the resource they just called. Then they could send a message which would then be delivered to the callee's mobile device. That should be alright, shouldn't it?

Regarding same priorities and online statuses I noticed a different issue a few days back. I've already posted to jitsi-users [here] (http://lists.jitsi.org/pipermail/users/2015-October/010175.html) (2nd issue). I just noticed this seems to occur only if the two resources use the same priority.

While I was testing your proposed scenario I stumbled upon a certain constellation of resources and priorities in which it doesn't show the buttons, but the mobile indicator. I will look into that as well as in the other issue, but in a different thread.

···

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


#5

I can't test and further comment for a couple of days. Please ping me if I don't get back to you by the end of next week. In the meantime, do you think you could make the behavior change optional? I'm not sure yet what would be the default then, but at least it would be configurable.

···

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


#6

I can make this optional. I'll introduce a property for this, or did you mean an option in the GUI? I would like to point out, that Pidgin users [cannot](https://developer.pidgin.im/wiki/Protocol%20Specific%20Questions#HowcanIconfigureresourcepriority) specify their priorities. Also that Pidgin for Linux implements Jingle.

At the moment I'm still getting some weirdness by Psi on Linux, which I want to iron out.

···

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


#7

A GUI option would be ideal. I'm thinking of a checkbox labeled something like "Use all resources when showing a contacts capabilities" in a new advanced configuration section for XMPP (similar to the one for SIP).

···

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


#8

I set it to disabled as default. When it is disabled it uses the untouched code and otherwise it uses the new implementation. How should I proceed regarding translations? At the moment it's only residing in the generic resources.properties.

I'll commit this, after I've doubled checked the code and looked into that Psi issue some more.

![screen shot 2015-10-26 at 14 01 11](https://cloud.githubusercontent.com/assets/11325631/10729540/81c45652-7bea-11e5-8477-1d0a7227f93c.png)

···

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


#9

We use [Pootle](http://translate.jitsi.org) for translations. You don't need to touch the other resource files (they'd get overwritten).

···

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


#10

I doubled checked the code and tested it thoroughly. However the issue regarding the offline presence on same priority resources caused that weirdness I wanted to iron out. That line [here](https://github.com/459below/jitsi/blob/fix-callbutton/src/net/java/sip/communicator/impl/protocol/jabber/OperationSetContactCapabilitiesJabberImpl.java#L538) prevents that all caps nodes get removed if a rogue offline presence update comes in. At first I thought of it as a workaround, however after some thought it looks proper to me.

I'm going to look into that presence issue now.

The CLA has been submitted on 17.10.

···

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


#11

Seems that presence bug lies [here](https://github.com/459below/jitsi/blob/fix-callbutton/src/net/java/sip/communicator/impl/protocol/jabber/OperationSetPersistentPresenceJabberImpl.java#L1527). In case this ends up being exactly 0 again TreeSet will deem them equal and discard one of them.

I will verify this and if true put it into a different pull request.

···

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


#12

     {
         OperationSetPresence opsetPresence
- = parentProvider.getOperationSet(OperationSetPresence.class);
-
- if (opsetPresence != null)
- {
- String jid = StringUtils.parseBareAddress(user);
- Contact contact = opsetPresence.findContactByID(jid);
-
- // If the contact isn't null and is online we try to discover the
- // new set of operation sets and to notify interested parties.
- // Otherwise we ignore the event.
- if (contact != null)
+ = parentProvider.getOperationSet(OperationSetPresence.class);
+ if (opsetPresence != null) {
+ if(LibJitsi.getConfigurationService()

You cannot access the config service like this. It needs to be obtained from GeneralConfigPluginActivator.getConfigurationService()

···

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


#13

+ */
+package net.java.sip.communicator.plugin.generalconfig;
+
+import java.awt.*;
+import java.awt.event.*;
+
+import javax.swing.*;
+
+import org.jitsi.service.configuration.*;
+
+import net.java.sip.communicator.plugin.desktoputil.*;
+
+/**
+ * Implementation of the configuration form.
+ *
+ * @author Damian Minkov

That would be you, not Damian :slight_smile:

···

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


#14

-
- if (opsetPresence != null)
- {
- String jid = StringUtils.parseBareAddress(user);
- Contact contact = opsetPresence.findContactByID(jid);
-
- // If the contact isn't null and is online we try to discover the
- // new set of operation sets and to notify interested parties.
- // Otherwise we ignore the event.
- if (contact != null)
+ = parentProvider.getOperationSet(OperationSetPresence.class);
+ if (opsetPresence != null) {
+ if(LibJitsi.getConfigurationService()
+ .getBoolean(
+ XMPPConfigForm.PROP_XMPP_USE_ALL_RESOURCES_FOR_CAPABILITIES,
+ XMPPConfigForm.USE_ALL_RESOURCES_FOR_CAPABILITIES_DEFAULT))

The XMPPConfigForm is in a different bundle and may not be referenced in the protocol-jabber bundle. (It would e.g. break the Videobridge which doesn't have the generalconfig bundle). This probably only works because the constants are expanded at compile time. And because by convention you cannot reference impl-bundles in the UI, I currently only see the option of duplicating those constants in the protocol and the UI bundle.

···

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


#15

     {
- OperationSetPresence opsetPresence
+ if(!LibJitsi.getConfigurationService()
+ .getBoolean(
+ XMPPConfigForm.PROP_XMPP_USE_ALL_RESOURCES_FOR_CAPABILITIES,
+ XMPPConfigForm.USE_ALL_RESOURCES_FOR_CAPABILITIES_DEFAULT)
+ ||fullJids.isEmpty())

Same as above.

···

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


#16

Okay, thanks, so:
- I need to verify the CLA with Emil as I don't have access to Docusign myself.
- Please address the inline comments.
- Please check your changes for violations of our [code convention](https://jitsi.org/Documentation/CodeConvention). There are some lines that are too long, braces on the same line, some missing spaces between operators. (But please don't reformat existing code, so far this PR is exemplar for that!)
- Enable your behavior by default. I want to get this into the nightlies to gather feedback.
- When you're done, please rebase the PR on master and squash the first three commits into one.
- Looking forward to your other PR, it probably shouldn't be a `TreeSet<>` but rather a `SortedList<>`. We had some issues with that before when the meaning of `Set` wasn't really understood.

···

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


#17

     {
         OperationSetPresence opsetPresence
- = parentProvider.getOperationSet(OperationSetPresence.class);
-
- if (opsetPresence != null)
- {
- String jid = StringUtils.parseBareAddress(user);
- Contact contact = opsetPresence.findContactByID(jid);
-
- // If the contact isn't null and is online we try to discover the
- // new set of operation sets and to notify interested parties.
- // Otherwise we ignore the event.
- if (contact != null)
+ = parentProvider.getOperationSet(OperationSetPresence.class);
+ if (opsetPresence != null) {
+ if(LibJitsi.getConfigurationService()

Alright

···

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


#18

+ */
+package net.java.sip.communicator.plugin.generalconfig;
+
+import java.awt.*;
+import java.awt.event.*;
+
+import javax.swing.*;
+
+import org.jitsi.service.configuration.*;
+
+import net.java.sip.communicator.plugin.desktoputil.*;
+
+/**
+ * Implementation of the configuration form.
+ *
+ * @author Damian Minkov

Yes of course. That slipped through after I duplicated that class. My apologies to Damian.

···

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


#19

I'll do all of that.

Regarding the squashing. I haven't done much collaborative work with git as of yet. The purpose of squashing the first three commits is to clean up the commit history since the second and third commit are tiny or should it even all be squashed into one big commit?

···

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


#20

The first two commits (151e552f4ee8539f0ac0ba88f1e4a4391f871c44 and 13da0947f8a1a8e7ab9918741c051f51eaa2f7ff) logically belong together, it makes no sense to keep them as separate history on how the code became how it is. The third commit (b121edc96f6db8bf7014058cb8e199317ad1fde6) is a merge. Since this branch is not something other will work on, there's no need to not use rebase.
Same goes for the last commit (ad56640087885f0258a7bb0fad153b8194cc6ab6), you can get rid of that with rebasing (instead of merging).

···

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