[jitsi-dev] [jitsi] Basic (experimental) IRC support (#16)


#1

Hi all,

Here's the pull request with support for IRC. The implementation is far from finished, but it should be able to provide decent chat support.

Some features that might be available on a good day:
* Connect to IRC server using plain and secure connection,
* Add, join, leave channels,
* Some support for channel forwarding (depends on channel forwarded to being in the contact list),
* Sending and receiving messages in joined channel,
* Sending and receiving private messages (currently implemented as a chat room with only 2 members, and sending happens with /msg user message),
* Support for channel member dynamics, such as: joins, parts, quits, kicks, role changes,
* Nick changes,
* Support for local user role changes,
* Support topic changes.

I have enabled 'debug'-level logging for the irc package. This also shows the output from the IRC library. During this stage, I believe it is good to keep an eye on the irc library's output too, so we can better pinpoint where/how problems might occur. (This should only be noticable if you actually use the IRC support.)

I have cleaned up the code a bit - as per request from Ingo. Some of the larger changes, such as a correct implementation of private messages, are still to come.

Danny
You can merge this Pull Request by running:

  git pull https://github.com/cobratbq/jitsi master

Or you can view, comment on it, or merge it online at:

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

-- Commit Summary --

  * Intermediate commit to save clean up (not currently working right now)
  * Rewritten IrcAccRegWizz after the IcqAccRegWizz since the original
  * Minimal IRC stack set-up.
  * Working on chatroom interactions.
  * Working on channel join/part operations and management.
  * Working on basic chatroom operations: topic changes, mode changes,
  * Tweaks + basic infrastructure for mode changes.
  * Upstream fix for NPE in IRC-API.
  * Simpler implementation for replacing keywords with the "highlighted"
  * Working on mode change support and listen to kick channel messages.
  * Added a TODO note for later attention.
  * Improved kick handling in chatroom listener.
  * Reduced code nesting.
  * Throw exception after failed attempt to connect to IRC server.
  * Invalid operation: disconnect is not availble at that moment.
  * Maintenance, tweaks & Mode enumerator.
  * Added irc-api libraries to classpath. (Previously also fixed a race
  * Clean up
  * Clean-up and TODOs.
  * Enhanced join operations for multithreading.
  * Updated irc-api library 0014-SNAPSHOT
  * NOTICE with changes described
  * Handling of nick change messages.
  * Renamed 'GenericListener' to 'ServerListener'.
  * Tweaks.
  * Added comments for some important classes.
  * Added basic workaround support for receiving private messages.
  * Added initial support for personal chat rooms.
  * Working on nick change support and improve personal chatrooms.
  * Implementation for channel mode 'l'.
  * Verify nick name before setting.
  * Support mode changes for local user role.
  * Tweaks and documentation for ModeParser.
  * Implementation of getServerChatRoomList.
  * Comments and tweaks to kick and invite implementations.
  * Check for null before firing role change event to chatroom.
  * Switched to LinkedList for channels.
  * Compare command case-insensitive.
  * Added TODO for another join failure case.
  * Initial support for secure connections and Wizard UI tweaks.
  * Added output for error messages.
  * Added dummy ChatRoomMember in case the source is an IRCServer instance.
  * Documentation and additional info in system message for mode changes.
  * Documentation, logging and updated irc-api library.
  * Removed CLASSPATH_ATTR_LIBRARY_PATH_ENTRY from .classpath
  * Tweaks + license comments.
  * Added comment w.r.t. creation of secure connection inside the irc-api
  * New parameter in new ChatRoomLocalUserRoleChangeEvent.
  * Added '(Experimental)' to resource.properties.
  * Moved slf4j-{api,simple} into separate bundle.
  * Fix line length.
  * Fixed line length.
  * Updated irc-api to 1.0-0015 and added support for channel join limit.
  * Cleaned up old pircbot library. Added irc-api sources. Some formatting.
  * Added TODOs for some mailing list remarks.
  * Added TODO for 416 truncated LIST command.
  * Fixed 2 IPv6 issues w.r.t. colons inside user identifier.
  * Improvement in syncing disconnect + utilizing map for registering joined
  * Tweaked the order of removal.
  * Initial support for channel forwarding.
  * tweaks to the configuration
  * Make members silent by default.
  * Tweaking the join process: throw OFE when there's trouble.
  * Removed System.out in favor of logging + added logging config for IRC.

-- File Changes --

    M .classpath (2)
    M build.xml (17)
    M lib/felix.client.run.properties (6)
    A lib/installer-exclude/irc-api-1.0-NOTICE (4)
    A lib/installer-exclude/irc-api-1.0-sources.jar (0)
    A lib/installer-exclude/irc-api-1.0.jar (0)
    D lib/installer-exclude/pircbot.jar (0)
    A lib/installer-exclude/slf4j-api-1.7.5.jar (0)
    A lib/installer-exclude/slf4j-simple-1.7.5.jar (0)
    M lib/logging.properties (3)
    M resources/languages/resources.properties (3)
    M src/net/java/sip/communicator/impl/gui/main/chat/ChatConversationPanel.java (45)
    M src/net/java/sip/communicator/impl/gui/main/chat/conference/ConferenceChatSession.java (3)
    M src/net/java/sip/communicator/impl/protocol/irc/ChatRoomIrcImpl.java (167)
    M src/net/java/sip/communicator/impl/protocol/irc/ChatRoomMemberIrcImpl.java (18)
    M src/net/java/sip/communicator/impl/protocol/irc/IrcAccountID.java (4)
    M src/net/java/sip/communicator/impl/protocol/irc/IrcStack.java (2933)
    A src/net/java/sip/communicator/impl/protocol/irc/Mode.java (85)
    A src/net/java/sip/communicator/impl/protocol/irc/ModeParser.java (190)
    M src/net/java/sip/communicator/impl/protocol/irc/OperationSetMultiUserChatIrcImpl.java (47)
    M src/net/java/sip/communicator/impl/protocol/irc/ProtocolProviderServiceIrcImpl.java (40)
    M src/net/java/sip/communicator/impl/protocol/irc/irc.provider.manifest.mf (7)
    M src/net/java/sip/communicator/plugin/ircaccregwizz/FirstWizardPage.java (60)
    M src/net/java/sip/communicator/plugin/ircaccregwizz/IrcAccRegWizzActivator.java (89)
    M src/net/java/sip/communicator/plugin/ircaccregwizz/IrcAccountRegistration.java (23)
    M src/net/java/sip/communicator/plugin/ircaccregwizz/IrcAccountRegistrationWizard.java (14)

-- Patch Links --

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

···

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


#2

Hey Danny
I really don't want to be picky again, so sorry in advance! But some stuff I found:
- There are still a few lines that are too long (and if @emcho reads this, he probably wonders why I insist on this, I really hate this limit myself, but since all other code is like that...)
- boolean comparisons with "expression == false" or "expression == true" are redundant, please just use "expression" or "!expression"
- The class IrcStack.ServerListener has a lof of redundant casts
- Logging: if you construct messages (i.e. passing not just a simple string), surround it with if(logger.isLevelEnabled()) to avoid the runtime cost of constructing an eventually unecessary string
- Some javadoc would look good on IrcStack.ChatRoomListener
- System.err.println in IrcStack.ChatRoomListener
- With regards to translators, I'm not keen on adding (Experimental) to the protocol name. It might stand there for years if someone translates this now and then doesn't translate it back.
- bundle-slf4j has tabs insted of spaces
- The source jars of all the used libraries should go to the repo jitsi/libsrc. Can you make a pull request there as well?
- What happens currently when the certificate of a secure connection fails to validate?
- Then there is very common programming failure around object.wait() with the InterruptedException: a Thread that wait can wake up spuriously (generally known as spurious wakeup). Thus the pattern should always be:

while (!conditionToWaitOnIsFulfilled)
{
    try
    {
        doSomething();
        object.wait();
    }
    catch (InterruptedException ex)
    {
        // usually just do nothing
    }
}

Thanks!

···

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


#3

Hi Ingo,

···

On 01/22/2014 10:24 PM, Ingo Bauersachs wrote:
    
      Hey Danny
        I really don't want to be picky again, so sorry in advance! But
        some stuff I found:
      There are still a few lines that are too long (and if @emcho reads this, he probably
          wonders why I insist on this, I really hate this limit myself,
          but since all other code is like that...)
      
    Is there a quick way to find all of these?
      boolean comparisons with "expression == false" or
          "expression == true" are redundant, please just use
          "expression" or "!expression"
      
    Okay, no prob, I actually prefer the bla == false over !bla, since I
    find '!bla' easier to overlook. Will change it though, ofcourse.
      The class IrcStack.ServerListener has a lof of redundant
          casts
      
    Hmm... you're right ... don't know how those got there. It's
    certainly not something I do for fun ... or profit ...
      Logging: if you construct messages (i.e. passing not just a
          simple string), surround it with if(logger.isLevelEnabled())
          to avoid the runtime cost of constructing an eventually
          unecessary string
      
    Right, so that's the reason.
      Some javadoc would look good on IrcStack.ChatRoomListener
        System.err.println in IrcStack.ChatRoomListener
        With regards to translators, I'm not keen on adding
          (Experimental) to the protocol name. It might stand there for
          years if someone translates this now and then doesn't
          translate it back.
      
    Okay, well it was a suggestion from someone on the list.
      bundle-slf4j has tabs insted of spaces
        The source jars of all the used libraries should go to the
          repo jitsi/libsrc. Can you make a pull request there as well?
      
    Will have a look.
      What happens currently when the certificate of a secure
          connection fails to validate?
      
    I don't have a clue. Would need to try that. Also, the irc-api
    library does not use your suggested way of obtaining an secure
    socket connection, so there definitely room for improvement there.
      Then there is very common programming failure around
          object.wait() with the InterruptedException: a Thread that
          wait can wake up spuriously (generally known as spurious
          wakeup). Thus the pattern should always be:
      while (!conditionToWaitOnIsFulfilled)
{
    try
    {
        doSomething();
        object.wait();
    }
    catch (InterruptedException ex)
    {
        // usually just do nothing
    }
}
    
    Yeah, I did read something about that. So this is actually really
    necessary. I hadn't taken it into account because I didn't know how
    extreme it would be. It sounded like some kind of a last chance
    remedy against deadlocks and such. Thanks for pointing this out!
    So I guess there will be a new pull request then. If anyone else has
    feedback, please don't hesitate to let me know.
    Danny

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


#4

- Finding line length: I don't know. Eclipse has an option to show a marker line (Preferences: General/Editors/TextEditors/Show print margin: 80)
- Experimental: Hmm, yes. I wonder if we should just add this directly in the code instead of translating it. But this isn't important at all.
- Secure connections: The point is that I don't want that users get a wrong impressions of security, even if IRC support will be marked as experimental. Why don't we hide the wizard option for secure connections until the CertificateService handles the SSLContext or TrustManager? Advanced users could still edit the account properties manually. Adding a note that a "secure connection" is actually not yet secure is another option.
- Sprious wakeups: They are rare, but can happen, so it's best to handle them by the book (but I also know for sure that existing code has lots of the same mistake)

···

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


#5

Hi Ingo,

Then there is very common programming failure around

object.wait() with the InterruptedException: a Thread that
wait can wake up spuriously (generally known as spurious
wakeup). Thus the pattern should always be:

while (!conditionToWaitOnIsFulfilled)
{
    try
    {
        doSomething();
        object.wait();
    }
    catch (InterruptedException ex)
    {
        // usually just do nothing
    }
}

You've mentioned that there are lots of those mistakes in code. But I do not agree 100% that throwing InterruptedException is a mistake. Spurious wakeup is when the thread wakes up from wait() without notify() being called on monitor object. It's not the situation when InterruptedException is being thrown. It's thrown when interrupt() method is called on waiting thread. So it doesn't hurt when we throw an exception which will cause the thread to interrputs it's further processing. I'd like to have it clear as I do throw InterruptedException wrapped in RuntimeException all the time.

Regards,
Pawel

···

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


#6

Spurious wakeup is when the thread wakes up from wait() without notify() being called on monitor object.

Yes, I was referring to exactly these wakeups.

It's not the situation when InterruptedException is being thrown. It's thrown when interrupt() method is called on waiting thread. So it doesn't hurt when we throw an exception which will cause the thread to interrputs it's further processing. I'd like to have it clear as I do throw InterruptedException wrapped in RuntimeException all the time.

Exceptions always hurt. They are expensive and although they are an integral part of Java, they shouldn't be abused. I don't know in which cases you need to .interrupt() a thread, but it sounds seriously wrong.

···

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


#7

Actually I do not interrupt those threads, but in case something will interrupt that thread at least we will now about it(because of an exception) and we can fix it. Saves us time on debugging strange issues.

···

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


#8

Hi Pawel,
I did some "research" and concluded myself that the InterruptedException should not be handled inside the loop, so I didn't. Currently when an InterruptedException happens, I propagate an error or handle appropriately. For a spurious wakeup I simply loop and simply continue waiting for the next signal.

···

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


#9

I did some "research"

I'd be curious how you came to that conclusion.

For a spurious wakeup I simply loop and simply continue waiting for the next signal.

Isn't that a contradiction to what you just said before?

···

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


#10

Some [sources] say that interrupting current thread is a good way for handling InterruptedException:

}
catch(InterruptedException e)
{
    Thread.currentThread().interrupt();
}

But I'm not sure if we always want to care about this. The worst thing we can do is to swallow it. This shouldn't be propagated as a good practice.

[sources]: http://www.ibm.com/developerworks/java/library/j-jtp05236/index.html?ca=drs-

···

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


#11

Ingo, about your remarks...

A spurious wakeup does not cause an 'InterruptedException'. The nature of the spurious wakeup is in the attempt for an efficient implementation and cannot be distinguished from a real notify()/notifyAll(). (At least according to some explanations on SO.) Any (non-exceptional) wake up may or may not be spurious. So if the thread wakes up without any kind of exception, we might or might not be finished. So I just check the conditions (defined in the while-loop) and if they are not yet met, then it must have been a spurious wakeup and we go back to waiting.

The InterruptedException on the other hand - at least the way I understand it - is thrown as a consequence of one thread "asking" another thread to interrupt (blocking) operations and "finish" early, i.e. interrupt. Therefore I consider an interruption to be initiated by an intelligent operation requiring the current operation to be interrupted, hence I will not loop and continue waiting.

Hence, I take an InterruptedException serious and respect the request, while a double-check and continue waiting in case of a (possibly spurious) wakeup.

···

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


#12

Or I should say: "... Therefore I consider an interruption to be initiated by an informed thread that requires the current thread to be interrupted, ..."

···

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


#13

By the way, I have implemented support for Jitsi's CertificateService in 20f5e76 and maybe some small fixes/changes in a later commit. I'm not sure if I have implemented everything, though. It seemed too easy. I appreciate the effort Jitsi devs put in security, so I agree with your earlier remark about not doing security half way.
I currently create a TrustManager from the CertificateService and pass the TM on when creating the SSLContext. After that I have been able to confirm its workings (I think) by connecting to a secure IRC server at which point I got a dialog asking me to verify/confirm connecting to the IRC server.

···

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


#14

You could use ServiceUtils.getService to obtain the CertificateService a bit more easily. But in general this is exactly how it should be done. And it actually is mostly that easy - that was the idea of the CertificateService.
Make sure though that if the user says "no" on an invalid certificate that you gracefully abort the connection process in the provider and that the user might need time to make his decision and you might need to restart the connection process due to timeouts.
On that server you tested, did you expect an invalid certificate?

Oh and:
- Please add the license header also to the unit-test files
- There are quite a couple of unnecessarily introduced newlines with whitespaces

···

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


#15

No, it doesn't. But the most prominent thing I've noticed is that it doesn't place the equals sign of assignments to the next line if doesn't fit on one line. But AFAIK there's no option for that. Whats wrong otherwise?
Applying it to existing code isn't a good idea anyway.

···

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


#16

@@ -162,7 +198,7 @@ public String getName()
     {
         return chatRoomName;
     }
-
+

If this is an additional whitespace, please remove it.

···

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


#17

@@ -173,6 +209,29 @@ public String getIdentifier()
     {
         return chatRoomName;
     }
+
+ /**
+ * Adds a <tt>ChatRoomMember</tt> to the list of members of this chat room.
+ *
+ * @param memberID the identifier of the member
+ * @param member the <tt>ChatRoomMember</tt> to add.
+ */
+

extra wl

···

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


#18

@@ -867,6 +904,7 @@ public void fireMessageReceivedEvent( Message message,
      */
     public void firePropertyChangeEvent(PropertyChangeEvent evt)
     {
+

extra wl

···

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


#19

@@ -983,17 +1031,28 @@ public void fireMemberRoleEvent( ChatRoomMember member,
         for (ChatRoomMemberRoleListener listener : listeners)
             listener.memberRoleChanged(evt);
     }
-
+

extra spaces

···

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


#20

      */
     @Override
- public void updatePrivateContactPresenceStatus(String nickname) { }
+ public void updatePrivateContactPresenceStatus(String nickname)
+ {
+ // IRC does not provide continuous presence status updates.

Please put this comment into the javadoc (instead of inheritdoc)

···

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