You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mina.apache.org by Ashish <pa...@gmail.com> on 2009/07/29 06:53:10 UTC
Re: svn commit: r798592 - /mina/sandbox/vysper/trunk/server/core/src/main/java/org/apache/vysper/xmpp/modules/core/im/handler/PresenceAvailabilityHandler.java
Couple of observations [May not be related to this commit]
1. Related to spec compliance use - The current implementation has
same element nested within
@SpecCompliance(compliant = {
@SpecCompliant(spec = "RFC3921bis-05", section = "3.1.5",
status = IN_PROGRESS, comment = "...."),
@SpecCompliant(spec = "RFC3921bis-08", section = "3.1.5",
status = NOT_STARTED, comment = "....")
})
Not sure about the possibility, but if we can specify them
individually (something like @param in javadoc), would be better. I am
yet to explore the possibility
Something like
@SpecCompliant(spec = "RFC3921bis-05", section = "3.1.5", status =
IN_PROGRESS, comment = "...."),
@SpecCompliant(spec = "RFC3921bis-08", section = "3.1.5", status =
NOT_STARTED, comment = "....")
I am not sure if it allowed by JSR 308. If not we can live with this.
2. We should separate static imports from normal imports. Makes it more readable
like
import java....
...// all normal imports
import static ...
...// all static imports
3. Max line length is way higher than normal 80 chars, mostly above
120 chars. For most of MINA code, the limit has been followed.
Let me see if I can dive-in and help out over the weekend.
thanks
ashish
On Tue, Jul 28, 2009 at 9:40 PM, <be...@apache.org> wrote:
> Author: berndf
> Date: Tue Jul 28 16:10:00 2009
> New Revision: 798592
>
> URL: http://svn.apache.org/viewvc?rev=798592&view=rev
> Log:
> fix VYSPER-154: handle presence probes a little bit more correctly
>
> Modified:
> mina/sandbox/vysper/trunk/server/core/src/main/java/org/apache/vysper/xmpp/modules/core/im/handler/PresenceAvailabilityHandler.java
>
> Modified: mina/sandbox/vysper/trunk/server/core/src/main/java/org/apache/vysper/xmpp/modules/core/im/handler/PresenceAvailabilityHandler.java
> URL: http://svn.apache.org/viewvc/mina/sandbox/vysper/trunk/server/core/src/main/java/org/apache/vysper/xmpp/modules/core/im/handler/PresenceAvailabilityHandler.java?rev=798592&r1=798591&r2=798592&view=diff
> ==============================================================================
> --- mina/sandbox/vysper/trunk/server/core/src/main/java/org/apache/vysper/xmpp/modules/core/im/handler/PresenceAvailabilityHandler.java (original)
> +++ mina/sandbox/vysper/trunk/server/core/src/main/java/org/apache/vysper/xmpp/modules/core/im/handler/PresenceAvailabilityHandler.java Tue Jul 28 16:10:00 2009
> @@ -229,6 +229,7 @@
> // send probes to all contacts of the current jid where
> // 'subscription' is either 'to' or 'both'
> // TODO: ...and jid is not blocking inbound presence notification
> + // TODO: optimize: don't send server-local probes when contact's presence is known locally
> List<RosterItem> rosterContacts_TO = new ArrayList<RosterItem>();
> rosterContacts_TO.addAll(item_TO);
> rosterContacts_TO.addAll(item_BOTH);
> @@ -257,7 +258,19 @@
> return presenceStanza;
> }
>
> - @SpecCompliant(spec = "RFC3921bis-04", section = "4.3.2")
> + /**
> + * TODO I don't think this works particulary good.
> + * @param stanza
> + * @param serverRuntimeContext
> + * @param sessionContext
> + * @param registry
> + * @param rosterManager
> + * @return
> + */
> + @SpecCompliance(compliant = {
> + @SpecCompliant(spec = "RFC3921bis-04", section = "4.3.2"),
> + @SpecCompliant(spec = "RFC3921bis-08", section = "4.3.2")
> + })
> private XMPPCoreStanza handleInboundPresenceProbe(PresenceStanza stanza, ServerRuntimeContext serverRuntimeContext, SessionContext sessionContext, ResourceRegistry registry, RosterManager rosterManager) {
> Entity contact = stanza.getFrom();
> Entity user = stanza.getTo();
> @@ -274,21 +287,35 @@
> return null;
> }
>
> - if (user.getResource() == null) {
> + if (contact.getResource() == null) {
> // presence probes must happen on resource level!
> relayStanza(contact, buildPresenceStanza(user, contact, UNSUBSCRIBED, null), sessionContext);
> return null;
> }
>
> - PresenceStanza presenceStanza = retrieveLatestPresence(sessionContext, user);
> - if (presenceStanza == null) {
> + PresenceStanza latestPresenceStanza = null;
> + if (!user.isResourceSet()) {
> + List<String> availableResources = serverRuntimeContext.getResourceRegistry().getAvailableResources(user);
> + for (String availableResource : availableResources) {
> + PresenceStanza presenceStanza = serverRuntimeContext.getPresenceCache().get(new EntityImpl(user, availableResource));
> + // TODO which one to take? which resource has the current presence?
> + if (presenceStanza != null) {
> + latestPresenceStanza = presenceStanza;
> + break;
> + }
> + }
> + } else {
> + latestPresenceStanza = retrieveLatestPresence(sessionContext, user);
> + }
> +
> + if (latestPresenceStanza == null) {
> // we have no current presence info
> relayStanza(contact, buildPresenceStanza(user, contact, UNAVAILABLE, null), sessionContext);
> return null;
> }
>
> // return current presence as probing result
> - relayStanza(contact, buildPresenceStanza(user, contact, null, presenceStanza.getInnerElements()), sessionContext);
> + relayStanza(contact, buildPresenceStanza(user, contact, null, latestPresenceStanza.getInnerElements()), sessionContext);
>
> return null;
> }
>
>
>
Re: svn commit: r798592 - /mina/sandbox/vysper/trunk/server/core/src/main/java/org/apache/vysper/xmpp/modules/core/im/handler/PresenceAvailabilityHandler.java
Posted by Bernd Fondermann <bf...@brainlounge.de>.
Ashish wrote:
> On Wed, Jul 29, 2009 at 2:00 PM, Bernd Fondermann<bf...@brainlounge.de> wrote:
>> Ashish wrote:
>>> Couple of observations [May not be related to this commit]
>>> 1. Related to spec compliance use - The current implementation has
>>> same element nested within
>>>
>>> @SpecCompliance(compliant = {
>>> @SpecCompliant(spec = "RFC3921bis-05", section = "3.1.5",
>>> status = IN_PROGRESS, comment = "...."),
>>> @SpecCompliant(spec = "RFC3921bis-08", section = "3.1.5",
>>> status = NOT_STARTED, comment = "....")
>>> })
>>>
>>> Not sure about the possibility, but if we can specify them
>>> individually (something like @param in javadoc), would be better. I am
>>> yet to explore the possibility
>>>
>>> Something like
>>> @SpecCompliant(spec = "RFC3921bis-05", section = "3.1.5", status =
>>> IN_PROGRESS, comment = "...."),
>>> @SpecCompliant(spec = "RFC3921bis-08", section = "3.1.5", status =
>>> NOT_STARTED, comment = "....")
>>>
>>> I am not sure if it allowed by JSR 308. If not we can live with this.
>> Agreed, but it's not possible. That's the reason why we had to add
>> @SpecCompliance.
>
> :-(
>
>>> 2. We should separate static imports from normal imports. Makes it more readable
>>>
>>> like
>>>
>>> import java....
>>> ...// all normal imports
>>> import static ...
>>> ...// all static imports
>>>
>> Wouldn't it be more straightforward the other way round?
>
> either ways is fine. They just need to be together :-)
I tried to tell it to my IDE, but it seems agnostic about static vs.
non-static imports :-(
>
>> import static ...
>> ...// all static imports
>> import java....
>> ...// all normal imports
>>
>>> 3. Max line length is way higher than normal 80 chars, mostly above
>>> 120 chars. For most of MINA code, the limit has been followed.
>> I committed a fix. Better now?
>>
>> Today, my screen has way more columns than lines.
>> That's why I personally prefer longer lines. 80 chars looks way to
>> narrow for me since sitting in front of those large displays.
>
> Agree, instead of 80, we can have 120, but definitely not 200
> Though, i hate to follow this in a form which actually breaks readability.
> My only concern here is uniformity, if everyone is ok with this we can follow
> this, but we have to do this as a community :-)
Sure, please keep reminding me about it!
Readable code is everything.
Bernd
Re: svn commit: r798592 - /mina/sandbox/vysper/trunk/server/core/src/main/java/org/apache/vysper/xmpp/modules/core/im/handler/PresenceAvailabilityHandler.java
Posted by Ashish <pa...@gmail.com>.
On Wed, Jul 29, 2009 at 2:00 PM, Bernd Fondermann<bf...@brainlounge.de> wrote:
> Ashish wrote:
>> Couple of observations [May not be related to this commit]
>> 1. Related to spec compliance use - The current implementation has
>> same element nested within
>>
>> @SpecCompliance(compliant = {
>> @SpecCompliant(spec = "RFC3921bis-05", section = "3.1.5",
>> status = IN_PROGRESS, comment = "...."),
>> @SpecCompliant(spec = "RFC3921bis-08", section = "3.1.5",
>> status = NOT_STARTED, comment = "....")
>> })
>>
>> Not sure about the possibility, but if we can specify them
>> individually (something like @param in javadoc), would be better. I am
>> yet to explore the possibility
>>
>> Something like
>> @SpecCompliant(spec = "RFC3921bis-05", section = "3.1.5", status =
>> IN_PROGRESS, comment = "...."),
>> @SpecCompliant(spec = "RFC3921bis-08", section = "3.1.5", status =
>> NOT_STARTED, comment = "....")
>>
>> I am not sure if it allowed by JSR 308. If not we can live with this.
>
> Agreed, but it's not possible. That's the reason why we had to add
> @SpecCompliance.
:-(
>>
>> 2. We should separate static imports from normal imports. Makes it more readable
>>
>> like
>>
>> import java....
>> ...// all normal imports
>> import static ...
>> ...// all static imports
>>
>
> Wouldn't it be more straightforward the other way round?
either ways is fine. They just need to be together :-)
>
> import static ...
> ...// all static imports
> import java....
> ...// all normal imports
>
>> 3. Max line length is way higher than normal 80 chars, mostly above
>> 120 chars. For most of MINA code, the limit has been followed.
>
> I committed a fix. Better now?
>
> Today, my screen has way more columns than lines.
> That's why I personally prefer longer lines. 80 chars looks way to
> narrow for me since sitting in front of those large displays.
Agree, instead of 80, we can have 120, but definitely not 200
Though, i hate to follow this in a form which actually breaks readability.
My only concern here is uniformity, if everyone is ok with this we can follow
this, but we have to do this as a community :-)
I have a long way to go before I match your skills :-)
--
thanks
ashish
Blog: http://www.ashishpaliwal.com/blog
My Photo Galleries: http://www.pbase.com/ashishpaliwal
Re: svn commit: r798592 - /mina/sandbox/vysper/trunk/server/core/src/main/java/org/apache/vysper/xmpp/modules/core/im/handler/PresenceAvailabilityHandler.java
Posted by Bernd Fondermann <bf...@brainlounge.de>.
Ashish wrote:
> Couple of observations [May not be related to this commit]
> 1. Related to spec compliance use - The current implementation has
> same element nested within
>
> @SpecCompliance(compliant = {
> @SpecCompliant(spec = "RFC3921bis-05", section = "3.1.5",
> status = IN_PROGRESS, comment = "...."),
> @SpecCompliant(spec = "RFC3921bis-08", section = "3.1.5",
> status = NOT_STARTED, comment = "....")
> })
>
> Not sure about the possibility, but if we can specify them
> individually (something like @param in javadoc), would be better. I am
> yet to explore the possibility
>
> Something like
> @SpecCompliant(spec = "RFC3921bis-05", section = "3.1.5", status =
> IN_PROGRESS, comment = "...."),
> @SpecCompliant(spec = "RFC3921bis-08", section = "3.1.5", status =
> NOT_STARTED, comment = "....")
>
> I am not sure if it allowed by JSR 308. If not we can live with this.
Agreed, but it's not possible. That's the reason why we had to add
@SpecCompliance.
>
> 2. We should separate static imports from normal imports. Makes it more readable
>
> like
>
> import java....
> ...// all normal imports
> import static ...
> ...// all static imports
>
Wouldn't it be more straightforward the other way round?
import static ...
...// all static imports
import java....
...// all normal imports
> 3. Max line length is way higher than normal 80 chars, mostly above
> 120 chars. For most of MINA code, the limit has been followed.
I committed a fix. Better now?
Today, my screen has way more columns than lines.
That's why I personally prefer longer lines. 80 chars looks way to
narrow for me since sitting in front of those large displays.
Bernd
<snip/>