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

Archive
#21

-
- 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))

I read up on the bundle convention in the Jitsi projects. So if I understand correctly, references like this need to be isolated to the respective bundle the class itself resides in. In this particular case I'd simply duplicate the constant to *OperationSetContactCapabilitiesJabberImpl*, since there isn't a more appropriate place for this.

···

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

#22

-
- 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))

Yes.

···

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

#23

     {
         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()

I tried obtaining it via *GeneralConfigPluginActivator.getConfigurationService()*. That didn't work. *org.jitsi.service.libjitsi* is listed in *jabber.provider.manifest.mf*. Shouldn't this already comply with the bundle convention?

···

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

#24

I've covered every point on the list. However that one inline comment was a tad problematic.

···

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

#25

     {
         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()

I might have confused the files. Here it's the JabberActivator (or JabberProtocolActivator or similar, I'm on my mobile).
It just always from the activator of the bundle the file lives in.

···

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

#26

     {
         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. Thanks. I'll try to get this done right away.

···

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

#27

     {
         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()

OK, that worked.

···

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

#28

OK. Now everything should be up to code.

···

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

#29

+ *
+ * @param fullJids a list of full JIDs in which to find the resource with
+ * the most capabilities.
+ * @return the <tt>Map</tt> listing the most <tt>OperationSet</tt>s
+ * considered by the associated protocol provider to be supported by the
+ * specified <tt>contact</tt> (i.e. to be possessed as capabilities).
+ * Each supported <tt>OperationSet</tt> capability is represented by a
+ * <tt>Map.Entry</tt> with key equal to the <tt>OperationSet</tt> class
+ * name and value equal to the respective <tt>OperationSet</tt> instance
+ */
+ protected Map<String, OperationSet> getLargestSupportedOperationSet(
+ ArrayList<String> fullJids)
+ {
+ Map<String, OperationSet> supportedOperationSets =
+ new HashMap<String, OperationSet>();
+ if (fullJids!=null){

New line before { please

···

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

#30

+ * Each supported <tt>OperationSet</tt> capability is represented by a
+ * <tt>Map.Entry</tt> with key equal to the <tt>OperationSet</tt> class
+ * name and value equal to the respective <tt>OperationSet</tt> instance
+ */
+ protected Map<String, OperationSet> getLargestSupportedOperationSet(
+ ArrayList<String> fullJids)
+ {
+ Map<String, OperationSet> supportedOperationSets =
+ new HashMap<String, OperationSet>();
+ if (fullJids!=null){
+ for (String fullJid : fullJids)
+ {
+ Map<String, OperationSet> newSupportedOperationSets=
+ getSupportedOperationSets(fullJid, true);
+ if (newSupportedOperationSets.size()>
+ supportedOperationSets.size())

Please use an indent here, it's hard to read like this

···

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

#31

                 }
- else
+ } else {

New lines after } and before {

···

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

#32

+ boolean online = false;
+ Presence presence = parentProvider.getConnection().getRoster()
+ .getPresence(user);
+ if(presence != null)
+ online = presence.isAvailable();
+
+ if(contact != null)
+ {
+ fireContactCapabilitiesEvent(
+ contact,
+ ContactCapabilitiesEvent.
+ SUPPORTED_OPERATION_SETS_CHANGED,
+ getSupportedOperationSets(user, online));
+ }
+ }
+ } else {

Same

···

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

#33

Sorry for missing those. This coding style is very different from my own. The next one should be better :slight_smile:

···

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

#34

Merged #161.

···

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

#35

Merged. Thank you!
Do you want to make an announcement on dev so that people know the difference?

···

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

#36

Unfortunately I've missed one test case. This introduced a bug. The offline bug threw me off track here and I falsely blamed it for this (an unhandled exception looks very similar). If a contact **actually** goes offline, we do not grant him the offline capabilities, which among others is *sending offline messages*. We can't open the chat window, however we could write, if the window is already open.

I fixed it in this commit https://github.com/459below/jitsi/commit/f393b01befb08b2e974f4e741b3b01d5a30051cb
Annoyingly enough I actually did take it into account [here] (https://github.com/459below/jitsi/blob/f393b01befb08b2e974f4e741b3b01d5a30051cb/src/net/java/sip/communicator/impl/protocol/jabber/OperationSetContactCapabilitiesJabberImpl.java#L574), but not [here] (https://github.com/459below/jitsi/blob/f393b01befb08b2e974f4e741b3b01d5a30051cb/src/net/java/sip/communicator/impl/protocol/jabber/OperationSetContactCapabilitiesJabberImpl.java#L493)

···

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

#37

Can you please create a separate PR for that commit?
And please uses a space around operators (and assignments in the future too).

···

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