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

Add support for XEP-0382: Spoiler Messages #195

Closed
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@vanitasvitae
Copy link
Contributor

vanitasvitae commented Jan 16, 2018

No description provided.

@Flowdalic
Copy link
Member

Flowdalic left a comment

Created SMACK-795. Please mention the issue key somewhere in the commit message.

import org.jivesoftware.smackx.disco.ServiceDiscoveryManager;
import org.jivesoftware.smackx.spoiler.element.SpoilerElement;

public final class SpoilerManager extends Manager {

This comment has been minimized.

@Flowdalic

Flowdalic Jan 17, 2018

Member

I think the manager should probably get auto initialized (see other managers for the corresponding Smack pattern). I am also wondering if it should provide methods to start and stop announcing support for spoiler messages (e.g. startAnnouncingSupportForSpoilerMessages() and stopAnnouncingSupportForMessages()`). But then again, I can't come up with a good use case for those methods, given that the manager would, once instantiated, start announcing support.

*
* @return empty SpoilerElement
*/
public SpoilerElement createSpoiler() {

This comment has been minimized.

@Flowdalic

Flowdalic Jan 17, 2018

Member

There is no need for those methods to be non-static. It is also common in Smack that those type of methods are part of the SpoilerElement class. I also don't see a reason why the API would need the with-argument variant of the createSpoiler(…) method, the user could simply use the constructor. And for the non-argument variant, I would probably go with a static final instance field, .e.g SpoilerElement.EMPTY_INSTANCE.

String lang = null;
String hint = null;

// TODO: Find a better way to get 'xml:lang' attribute value

This comment has been minimized.

@Flowdalic

Flowdalic Jan 17, 2018

Member

See ParserUtils.getXmlLang() :)

This comment has been minimized.

@vanitasvitae

vanitasvitae Jan 17, 2018

Contributor

Ah, I searched in XmlPullParser for it, since the corresponding setter is in XmlStringBuilder.

assertXMLEqual(xml, empty.toXML().toString());

XmlPullParser parser = TestUtils.getParser(xml);
SpoilerElement parsed = new SpoilerProvider().parse(parser);

This comment has been minimized.

@Flowdalic

Flowdalic Jan 17, 2018

Member

The common Smack idiom is to have a public field with an instance of a provider, usually for testing purposes, e.g. SpoilerProvider.INSTANCE.


public class SpoilerTest extends SmackTestSuite {

private static final XMPPConnection connection = new DummyConnection();

This comment has been minimized.

@Flowdalic

Flowdalic Jan 17, 2018

Member

XEPs like this usually don't need a connection instance for unit testing. And I think with the changes I suggested, this can go away.

import org.jivesoftware.smack.util.XmlStringBuilder;
import org.jivesoftware.smackx.spoiler.SpoilerManager;

public class SpoilerElement implements ExtensionElement {

This comment has been minimized.

@Flowdalic

Flowdalic Jan 17, 2018

Member

The class's API is missing convenient methods to check whether or not an Message contains a spoiler extension element and to retrieve the metadata of an possibly existing spoiler extension element. For example something like
static boolean containsSpoiler(Message)
static Map<String,String> getSpoilers(Message)

I wonder if the API should make it possible to determine by a single getSpoilers() invocation if there was no spoiler, or if there was just a single empty spoiler. So getSpoilers() returns 'null' in case there are no spoilers, and the empty Map if there are just empty spoilers.

This comment has been minimized.

@vanitasvitae

vanitasvitae Jan 18, 2018

Contributor

Done

*/
public SpoilerElement(String language, String hint) {
this.language = language;
this.hint = hint;

This comment has been minimized.

@Flowdalic

Flowdalic Jan 17, 2018

Member

The constructor should check that hint is not null or empty in case language is not null or empty.

@vanitasvitae vanitasvitae force-pushed the vanitasvitae:spoiler branch from d949671 to 10aaae1 Jan 17, 2018

@vanitasvitae

This comment has been minimized.

Copy link
Contributor

vanitasvitae commented Jan 17, 2018

I updated the PR to reflect your requested changes.

@vanitasvitae vanitasvitae force-pushed the vanitasvitae:spoiler branch from 10aaae1 to ce12899 Jan 17, 2018

vanitasvitae added some commits Feb 10, 2018

*/
private SpoilerManager(XMPPConnection connection) {
super(connection);
ServiceDiscoveryManager.getInstanceFor(connection).addFeature(NAMESPACE_0);

This comment has been minimized.

@Flowdalic

Flowdalic Feb 21, 2018

Member

I don't think we want to automatically announce support for spoiler messages. The XEP is pretty clear that if a spoiler message is received the content must only be displayed upon user's request. But this can not, or only with to much effort, be guranteed on the library level.

I think we should remove the automatic announcement of spoiler messages and instead provide a method the user explicitly has to invoke in order to announce support for spoiler messages.

As beneficial result, the whole automatic manager initialization boilerplate code can go away

@Flowdalic

This comment has been minimized.

Copy link
Member

Flowdalic commented Feb 22, 2018

Squashed and commited as ce19ea4

@Flowdalic Flowdalic closed this Feb 22, 2018

@vanitasvitae vanitasvitae deleted the vanitasvitae:spoiler branch Feb 22, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment