-
Notifications
You must be signed in to change notification settings - Fork 885
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
XEP-0084: User Avatar implementation #107
Conversation
a35dea3
to
85a82eb
Compare
return getNode(METADATA_NAMESPACE); | ||
} | ||
|
||
private LeafNode getNode(String nodeName) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is why it's a good idea to have PubSubManager.getOrCreateNode(String)
: You are duplicating code that you already wrote for Geo Location here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code so that you can make as many member fields final. And make every member final that you can make final.
*/ | ||
public class MetadataInfo { | ||
|
||
private String id; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should all be final.
*/ | ||
public class MetadataPointer { | ||
|
||
private String namespace; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all final
public static final String ELEMENT = "data"; | ||
public static final String NAMESPACE = UserAvatarManager.DATA_NAMESPACE; | ||
|
||
private byte[] data; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
final
* | ||
* @return the data | ||
*/ | ||
public byte[] getData() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return a copy of the data to keep DataExtension immutable.
public static final String ELEMENT = "metadata"; | ||
public static final String NAMESPACE = UserAvatarManager.METADATA_NAMESPACE; | ||
|
||
private List<MetadataInfo> infos; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all final and unmodifiable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
شماره نشان داده نمیشود
@Flowdalic please check the last fixes :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've just finished the first round of reviews. The OMEMO PR will be merged soon, and after this happened, I'll hope that we can merge your PRs. :)
public class MetadataInfo { | ||
|
||
private final String id; | ||
private final String url; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be of type 'URL'
this.url = url; | ||
this.bytes = bytes; | ||
this.type = type; | ||
this.height = pixelsHeight; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
height and width are xs:unsingedByte, thus the constructor should ensure that it's value is within the range of [1,254] and otherwise throw an IllegalArgumentException.
*/ | ||
public MetadataPointer(String namespace, HashMap<String, Object> fields) { | ||
this.namespace = namespace; | ||
this.fields = fields; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be wrap with Collections.umodifyableList()
*/ | ||
public boolean isSupportedByServer() | ||
throws NoResponseException, XMPPErrorException, NotConnectedException, InterruptedException { | ||
return findNodeItem(DATA_NAMESPACE) && findNodeItem(METADATA_NAMESPACE); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that this is how you determine if xep84 is supported by the server. You likely just need to check if PEP is supported.
* | ||
* @return the data as String | ||
*/ | ||
public String getDataAsString() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be called getDataBase64Encoded()
. To make it clear that the string representation of the data is base64 encoded.
outerloop: while (true) { | ||
int eventType = parser.next(); | ||
|
||
if (eventType == XmlPullParser.START_TAG) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should use switch/case on the eventType int.
|
||
if (eventType == XmlPullParser.START_TAG) { | ||
|
||
if (parser.getName().equals("info")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should use switch/case on the getName() string.
I'll close this in favor of #332 |
Documentation: http://xmpp.org/extensions/xep-0084.html