Skip to content
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

Howdy Jack #3

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Howdy Jack #3

wants to merge 1 commit into from

Conversation

HenrikJoreteg
Copy link
Contributor

I've made a relatively minor, reverse-compatible tweak to how the addHandler() function works. Rather than having to pass positional arguments for 'ns', 'name', 'type', etc. You can now pass just two arguments. The handler function and a simple object literal that defines the other values you wish to filter by.

For example, rather than this:

this.connection.addHandler(
    this.handleMucMessage.bind(this),
    null, "message",
    "groupchat", 
    null, 
    room, 
    {matchBare: true}
);

You can now do this:

this.connection.addHandler(this.handleMucMessage.bind(this), {
    name: 'message',
    type: 'groupchat',
    from: '[email protected]',
    matchBare: true
});

For me, this solves the problem of having to check the docs each time to see which order to pass in the refining arguments. Also, this approach opens up the possibility of really expanding the matching capabilities of that addHandler() function. Potentially, it could be expanded to support more fine-grained matching of a stanza's children, etc.

Not sure if you're interested in messing with this API. But the way I wrote it, it maintains backward compatibility. So all existing code should still work, unmodified.

Anyways, just figured I'd do a pull request in case you liked my change.

Thanks for Strophe and your Pro XMPP book! Cheers @HenrikJoreteg

…h an object rather than passing 'null' for several positional arguments. Reverse compatibility was maintained.
@metajack
Copy link
Owner

I like this a lot. The only thing I am considering is whether to take it even farther.

I think it was a mistake to use the return value of a handle to determine whether it is a one time or permanent handler. I've been thinking about deprecating addHandler and instead using a pair of functions (like addOnetimeHandler/addHandler), but I wasn't sure how to make it backwards compatible. This could solve the problem.

What do you think about making the two arg version use the new semantics? Or do you think that would be too confusing? If so, do you have alternate suggestions for naming these new functions?

@HenrikJoreteg
Copy link
Contributor Author

Hi Jack, glad you liked the concept.

I agree that deprecating the existing addHandler may be the best approach to avoid confusion. I'd suggest calling the new methods addTempHandler() and addPermHandler(). Also, if you're revisiting these APIs anyway, I have one additional idea:

I've seen backbone.js do this. While they don't require jQuery they detect for it and add some convenience methods if you have it. Essentially, if jQuery is present, you could allow jquery selector strings as an option.

Using the echobot example from your post about jquery and strophe you could potentially allow something like this:

this.connection.addPermHandler(this.handleMucMessage.bind(this), {
    selector: 'message[type='chat'][from]:has(body)'
});

This would streamline the process of identifying a muc invitation. As an example, without this option if I'm writing a muc plugin I have to do first add a handler that looks like this (using my object argument syntax):

// handle muc invites
this.connection.addHandler(this.handleMucInvite.bind(this), {
    name: 'message',
    ns: Strophe.NS.MUC_USER
});

Then in my handleMucInvite function I have to do this:

handleMucInvite: function (message) {

    var invite = $(message).find('x[xmlns="' + Strophe.NS.MUC_USER + '"] > invite'),
        result;

    // Here I have to again check whether this is actually a muc invitation
    if (invite.length) {
        result = {
            room: $(message).attr('from'),
            reason: $(message).find('reason').text(),
            body: $(message).find('body').text(),
            from: $(message).find('invite').attr('from'),
            xml: message
        };

        $(document).trigger('mucInviteReceived', result);
    }

    return true;
},

Otherwise I could just do:

// handle muc invites
this.connection.addHandler(this.handleMucInvite.bind(this), {
    selector: 'message[xmlns="' + Strophe.NS.MUC_USER + '"]:has(invite)'
});

Then my handler would only get triggered if it has an <invite> elem. Rather than having to check that again in my handler. This would be relevant for pubsub updates and other stanzas that are more deeply nested. Just a thought.

@metajack
Copy link
Owner

I think this would be good as a plugin perhaps. I've been intending to add XPath support for some time, but the browser's built in XPath won't work and there are no lightweight XPath implementations in JavaScript. Joe Hildebrand and I started work on one, but it is still unfinished.

@HenrikJoreteg
Copy link
Contributor Author

I suppose it could just be a plugin. Anyways, on the original issue. I'm for deprecating addHandler and adding addTempHandler() and addPermHandler()

@zanchin
Copy link

zanchin commented Nov 10, 2010

I would argue against taking advantage of jQuery in Strophe core because of reduced portability of code between applications. Same thing goes for plugins. I prefer to keep my plugins generic and use jQuery selectors in the application layer if necessary.

Built-in XPath support would be great though. I'be been meaning to look at http://coderepos.org/share/wiki/JavaScript-XPath for use in Strophe, but haven't had the time yet.

@metajack
Copy link
Owner

The XPath library we are working on is here: http://bitbucket.org/hildjj/curlypath

lboynton added a commit to lboynton/strophejs that referenced this pull request Aug 16, 2011
…as argument to the AUTHFAIL event.

This caters for servers implementing case metajack#3 in the XMPP spec in the following section: http://xmpp.org/internet-drafts/draft-saintandre-rfc3920bis-08.html#bind-clientsubmit-error-conflict
lboynton added a commit to lboynton/strophejs that referenced this pull request Aug 31, 2011
…as argument to the AUTHFAIL event.

This caters for servers implementing case metajack#3 in the XMPP spec in the following section: http://xmpp.org/internet-drafts/draft-saintandre-rfc3920bis-08.html#bind-clientsubmit-error-conflict
@mweibel
Copy link

mweibel commented Mar 6, 2012

Thumbs up for all three ideas, simplifying how to pass arguments to addHandler, adding two different methods for permanent and one time handlers and built-in xpath.

@piotr-cz
Copy link

What about specifying handler type in it's options?
Anyway, moving away form current state (return true) and options as object is really great.
I'm against jQuery dependency

@goors
Copy link

goors commented Dec 7, 2013

Hi everyone,

I am having problem on message that have invite in it.
I do not know what to put in addHandler because invite message does not have type.

Chat.connection.addHandler(Chat.on_message,null, "message", "groupchat"); //for group message
Chat.connection.addHandler(Chat.on_message,null, "message", "chat"); //for regular message

but what to put for invite message?
Chat.connection.addHandler(Chat.on_message,null, "message",null,null,null,null); fires on invite massage but it also fires on all other messages! Please help.

@Gordin
Copy link

Gordin commented Dec 7, 2013

@goors If you are referring to those invites http://xmpp.org/extensions/xep-0045.html#registrar-querytypes-invite
and you also want invites to be handled by Chat.on_message then you could do something like

var on_invite = function(x) {
    if (x.getElementsByTagName("invite").length) {
        return Chat.on_message(x);
    } else {
        return true;
    };
Chat.connection.addHandler(on_invite, null, "message");

You should use the https://github.com/strophe/strophejs repo btw, this one is obsolete

@goors
Copy link

goors commented Dec 7, 2013

Will this handler Chat.connection.addHandler(on_invite, null, "message"); handle chat and group_chat type also?

@Gordin
Copy link

Gordin commented Dec 8, 2013

It will be called on all messages but only those with an invite tag will reach Chat.on_message

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants