[sip-comm-dev] patch for jabber email notification


#1

hello,

we send you a little patch for the jabber email notification. We worked on
the JabberActivator, the OperationSetBasicInstantMessagingJabberImpl and we
created a Mailbox package which can listen new email notifications from the
server and manage new incoming emails.

Regards,
Matthieu Helleringer
Alain Knaebel

JabberMailNotification.patch (16.5 KB)


#2

Hey Alain and Matthieu,

Thanks for sending the patch. This is a very useful feature, and many
users will be enjoying it, so thanks for the effort!

A few notes (mostly about the form):

* Could you please make sure that all opening accolades are on a line of
their own?

* Could you please make sure that all indentation uses spaces and not
tabs? (You may want to try the AnyEdit Tools plugin for Eclipse. It
shows you the whitespaces you use and also automatically convertsa tabs
to spaces)

* I noticed a few class imports like for example:

Mailbox.java

import org.jivesoftware.smack.packet.IQ;

JabberActivator: > +import
net.java.sip.communicator.service.resources.ResourceManagementService;

MailboxProvider.java

+import org.jivesoftware.smack.packet.IQ;
+import org.jivesoftware.smack.provider.IQProvider;
+import org.xmlpull.v1.XmlPullParser;

QueryNotify.java

+import org.jivesoftware.smack.packet.IQ;
+import net.java.sip.communicator.util.*;

I suspect some of them might be intentional in order to avoid conflicts,
but could you please remove those that weren't?

* The javadocs for some of the methods in Mailbox.java (and possibly
elsewhere?) are quite scarce like for example a single @param or @return
tag. I agree that this is better than nothing but could you please add
an extra explanatory sentence?

* Could you please replace javadocs like these:

+ * @param keepAliveEnabled boolean

with something more descriptif? (even though some of the methods could
be really self-explanatory).

* A few of your classes have javadocs comments that look like this:
"A straight forward implementation of XxxYyyy". It would probably be a
good idea to explain things a bit more. The fact that this class is part
of the mail notification system, its role, and how it accomplishes its
purpose are all details that help newcomers to easily understand code.

* Not sure I understand this change:

- private boolean keepAliveEnabled = false;
+ private boolean keepAliveEnabled = true;

was it accidental?

I believe that's it for the time being. I haven't had the time to test
the patch yet so I may have a few extra comments coming after I do
(unless someone else beats me to it).

Cheers
Emil

Alain K wrote:

···

hello,

we send you a little patch for the jabber email notification. We worked
on the JabberActivator, the OperationSetBasicInstantMessagingJabberImpl
and we created a Mailbox package which can listen new email
notifications from the server and manage new incoming emails.

Regards,
Matthieu Helleringer
Alain Knaebel

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


#3

hello,

we send you the new patch for the jabber email notification with the Emil's
comments.

Regards,
Matthieu Helleringer
Alain Knaebel

JabberMailNotification.patch (19.3 KB)

···

2009/5/18 Emil Ivov <emcho@sip-communicator.org>

Hey Alain and Matthieu,

Thanks for sending the patch. This is a very useful feature, and many
users will be enjoying it, so thanks for the effort!

A few notes (mostly about the form):

* Could you please make sure that all opening accolades are on a line of
their own?

* Could you please make sure that all indentation uses spaces and not
tabs? (You may want to try the AnyEdit Tools plugin for Eclipse. It
shows you the whitespaces you use and also automatically convertsa tabs
to spaces)

* I noticed a few class imports like for example:

Mailbox.java
> import org.jivesoftware.smack.packet.IQ;

JabberActivator: > +import
net.java.sip.communicator.service.resources.ResourceManagementService;

MailboxProvider.java
> +import org.jivesoftware.smack.packet.IQ;
> +import org.jivesoftware.smack.provider.IQProvider;
> +import org.xmlpull.v1.XmlPullParser;

QueryNotify.java
> +import org.jivesoftware.smack.packet.IQ;
> +import net.java.sip.communicator.util.*;

I suspect some of them might be intentional in order to avoid conflicts,
but could you please remove those that weren't?

* The javadocs for some of the methods in Mailbox.java (and possibly
elsewhere?) are quite scarce like for example a single @param or @return
tag. I agree that this is better than nothing but could you please add
an extra explanatory sentence?

* Could you please replace javadocs like these:
> + * @param keepAliveEnabled boolean
with something more descriptif? (even though some of the methods could
be really self-explanatory).

* A few of your classes have javadocs comments that look like this:
"A straight forward implementation of XxxYyyy". It would probably be a
good idea to explain things a bit more. The fact that this class is part
of the mail notification system, its role, and how it accomplishes its
purpose are all details that help newcomers to easily understand code.

* Not sure I understand this change:

> - private boolean keepAliveEnabled = false;
> + private boolean keepAliveEnabled = true;

was it accidental?

I believe that's it for the time being. I haven't had the time to test
the patch yet so I may have a few extra comments coming after I do
(unless someone else beats me to it).

Cheers
Emil

Alain K wrote:
> hello,
>
> we send you a little patch for the jabber email notification. We worked
> on the JabberActivator, the OperationSetBasicInstantMessagingJabberImpl
> and we created a Mailbox package which can listen new email
> notifications from the server and manage new incoming emails.
>
> Regards,
> Matthieu Helleringer
> Alain Knaebel

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