aTalk Jingle implementation for review #520
Conversation
|
The licensing header on these files is not correct, there's a LOT of copy/paste from Jitsi. Apache-2.0 should be mostly fine, but (c) is 8x8. Ping @damencho |
|
Thanks for pinging me. I think that should keep the licenses from the project those are copied from ... |
|
A bit of aTalk development history. aTalk app development started way back in 2014, where the source was first forked from jitsi-android. Later on in 2015 , the Jitsi project is taken over by In all the released sources that are in the pull request, some of the classes are new and developed by aTalk developer; where they have used "Copyright xxxx Jive Software" as header statement, and the @author carries the developer(s) . The other jingle classes in the pull request, that perform similar functions as the original jitsi-android, all have been rewritten as to confirm to smack standard implementation using builder approach i.e. java class that extends AbstractXmlElement. I am no sure whether 8x8 can claim as the rightful owner of these files. The situation may be similar to FMJ and JFM packages that implements same/similar functions. |
|
I have done a local build on the latest pull-in sources. the whole build was OK, except in Checkstyle with errors in the import statements e.g.: The source are code formatted by android studio; user has not control or change settings to remove the above errors. In aTalk code format, it always use "next line" option for braces placement in I found the above code format make the source easier for code review. No sure if Smack will consider this as well. Currently with the above deviations, there are a lot of manual works need to performed when raising a pull request for smack repository. |
I'm not a lawyer, but what I know is that you cannot change the license headers of the code, especially for the LGPL code. So it is up to the Smack developers to decide. |
|
FYI: All the other jingle classes in jingle_rtp package that perform similar functions as the original jitsi-android, have been rewritten to confirm to smack standard implementation using builder approach. The code structure implementation made reference to smack source more than to jitsi. The situation is similar to FMJ and JFM packages that implement same/similar functions. In fact all Java Documents for FMJ refer to JFM; and JFM has not copyright over FMJ. |
|
I see two challenges with getting this into Smack as of now. First, Jingle is a very complex topic, and most Jingle implementations I've seen so far where subpar. They certainly "get the job done" but lack proper error handling and reporting, extensibility, and clean code (not that Smack is much better in some areas here). Ultimately, this leads to them becoming a maintenance burden, if not nightmare. The second issue is that unclear code pedigree. License regulations have to be obeyed and copyright statements have to be retained. Note that I am not saying that there are actual issues with the code in this PR. It is just that concerns have been raised, and I take licenses seriously and have not been able to find time to review the stated issues. In general, I am quite busy for the foreseeable future, and while I am certain that I will be in the future able again to spend some more of my spare time on open source projects, now is not the right time for me to take over, or even thoroughly review, the proposed code. But I think the situation is not so bleak as one may think. It would be perfectly fine, if not even advantageous, if we cultivate the Jingle code in an extra repository, and, in turn, library which is based on Smack. I think we could even get the Jingle Models, i.e., the |
|
I strong agree JIngle implementation is a complex task. I have reservation aTalk pull request will fully accepted by smack team immediately; but hopefully give someone a reference base, and make enhancements when doing the final smack implementation; meantime allowing other xmpp client developers a reference source for his earlier jingle features implementation. I feel the smack team has done an excellent job in providing the improved smack-jingle library even at this stage. aTalk heavily depends on smack libraries including the new Jingle support, these have helped in extending and accelerating aTalk app features implementation. To my best knowledge in interpretation of the license clauses as a layman, I certainly respect copyrighted material and source contributors. If your team found otherwise in my source releases, I would more than happy to make the necessary changes. You mentioned the "Smack Jingle" repository, is this in existence at the moment. Certainly this will help the xmpp community to share the work done; and later get included in the main smack library when it is matured. JIngle plays an importance role for xmpp client implementation. XMPP client implementation without jingle support, does limit its main function to be a text only communicator. |
Thanks, that line of though is certainly sensible.
Thanks again. :)
No. This was a subliminal hint in an attempt to encourage you to create such a repository. |
|
This looks super interesting! Curious as to whether there are plans to progress it further? |
Recipient and initiator interchanged in JingleContentImpl sending stanza
Must close both streams to send <close xmlns='http://jabber.org/protocol/ibb'>
|
What is the progress about it? |
|
The PR now contains all the souces to support rtp for media call, jingle file transfer with JET. The source is exact match 99% with aTalk, and is fully tested on aTalk. |
|
@vanitasvitae, @gkfabs: Have you seen? |
|
@Neustradamus - stop mentioning people in comments just to get their attention. It is incredibly annoying. I believe you've been asked to stop doing that repeatedly. |
|
For what it's worth, I appreciate the enormous efforts that have been put into this implementation by the author. However, I strongly agree with Flowdalic's statement that reads:
While there are concerns regarding this, we should avoid merging these changes. Sadly, reviewing issues like these is a time-consuming effort - one that few of us have much experience with. @Flowdalic if it helps, and as this wades into the territory of 'legalese', we can find out if the members of the board of the Ignite Realtime Foundation can help with this effort. |
|
Copyright controversy is only related to source in jingle_rtp directory. Most of the other sources are actually from the smack team members. aTalk just ported the sources and made enhancements, so they are compatible with smack version 4.4, and work reliability for application integration. May be smack team can consider port in these source first, and leave jingle_rtp out until thing are clear up. |
Make JingleFile extends JingleFileTransferChild Clean up JingleAction class
Thank you for your contribution!
Before creating a Pull Request, please make sure to
gradle checksuccessfully in order to make sure that your code does not break any JUnit tests and is conform to the projects code style.SMACK-XXX) in the body of your commit message (not the header), so that Jira automatically links the PR to the issue.aTalk jingle implementation
jingle classes implementatation in aTalk are sub-classed/based on two defined based classes.
a. AbstractXmlElement
b. DefaultXmlElementProvide
With the above two based classes, all jingle classes implementations are simpler and clearer.
The pull requested classes for review are based on the above implementation topologies;
these classe provide the support of the aTalk media call (video/audio) setup as shown in the logcat below.
Perhaps the smack team would review the aTalk jingle implementation, possibly consider for integration
with the samck future releases.
You may refer to aTalk github (v2.9.0) for full implementation and use the jingle classes.
https://github.com/cmeng-git/atalk-android