Initial Support for XEP-0447: Stateless File Sharing #450
Conversation
vanitasvitae
force-pushed
the
stateless_file_sharing
branch
from
e3b0fd7
to
0c4fd39
Compare
|
I rebased. Why is |
There was a problem hiding this comment.
Looking good, thanks!
Please also add the XEP entries into the table at smack-java8-full/src/main/java/org/jivesoftware/smackx/package-info.java
Why is
StandardExtensionElementno longer extendingExtensionElementbtw?
Because ExtensionElement has become a marker interface where all implementing classes must have a static final QNAME field. This invariant is checked by Smack's test suite.
I changed the item type of some lists to
NamedElementinstead, which is not ideal, but allows me to putStandardExtensionElementinstances into it during tests.
I think NamedElement is ideal, as the element is not qualified by an different namespace than its enclosing element.
|
|
||
| public class FileSharingElementProvider extends ExtensionElementProvider<FileSharingElement> { | ||
|
|
||
| public static final FileSharingElementProvider INSTANCE = new FileSharingElementProvider(); |
There was a problem hiding this comment.
This is only used by test code, hence it should be declared in the test class.
|
|
||
| if (event == XmlPullParser.TagEvent.START_ELEMENT) { | ||
| if (name.equals(FileMetadataElement.ELEMENT)) { | ||
| fileMetadataElement = new FileMetadataElementProvider().parse(parser, xmlEnvironment); |
There was a problem hiding this comment.
To avoid the allocation cost, we should create FileMetadataElementProvider.INSTANCE.
| List<UrlDataElement> urlDataElements = new ArrayList<>(); | ||
| List<NamedElement> otherSourceElements = new ArrayList<>(); | ||
| do { | ||
| XmlPullParser.TagEvent event = parser.nextTag(); |
There was a problem hiding this comment.
nextTag() may throw if the parser is before CHARACTERS, please implement the provider using the idiomatic pattern for providers.
| @Override | ||
| public int hashCode() { | ||
| return HashCode.builder() | ||
| .append(getElementName()) |
There was a problem hiding this comment.
Do we really need to mix in the constant element name here? This can probably be implemented as return date.hashCode().
| @Override | ||
| public boolean equals(Object obj) { | ||
| return EqualsUtil.equals(this, obj, (equalsBuilder, other) -> | ||
| equalsBuilder.append(getElementName(), other.getElementName()) |
There was a problem hiding this comment.
| equalsBuilder.append(getElementName(), other.getElementName()) |
As above, I believe it is sufficient to compare the dates of the elements
| default: | ||
| // Catch all for incomplete switch (MissingCasesInEnumSwitch) statement. | ||
| break; | ||
| case START_ELEMENT: |
There was a problem hiding this comment.
This "just" seems to fix the indentation, which I would keep for now in its broken state and probably fix at some point later.
| @Override | ||
| public int hashCode() { | ||
| return HashCode.builder() | ||
| .append(getElementName()) |
There was a problem hiding this comment.
| .append(getElementName()) |
| public boolean equals(Object obj) { | ||
| return EqualsUtil.equals(this, obj, (equalsBuilder, other) -> | ||
| equalsBuilder | ||
| .append(getElementName(), other.getElementName()) |
There was a problem hiding this comment.
| .append(getElementName(), other.getElementName()) |
| public int hashCode() { | ||
| return HashCode.builder() | ||
| .append(getElementName()) | ||
| .append(getNamespace()) |
There was a problem hiding this comment.
| .append(getNamespace()) |
| return EqualsUtil.equals(this, obj, (equalsBuilder, other) -> | ||
| equalsBuilder | ||
| .append(getElementName(), other.getElementName()) | ||
| .append(getNamespace(), other.getNamespace()) |
There was a problem hiding this comment.
| .append(getNamespace(), other.getNamespace()) |
This PR adds initial support (elements and providers) for XEP-0447: Stateless File Sharing
Depends on (and contains) #448 and #449