aiannacc / Goko-Salvager

Enhance your Dominion Online experience!
13 stars 9 forks source link

Always Stack module with no method overrides #228

Closed michaeljb closed 10 years ago

michaeljb commented 10 years ago

This branch is based off my grunt branch rather than beta, so #227 should be merged before this pull request (after which this pull request should just be one commit, 130ceae4fc368d42810b83dc6aa96c07950e580c). I did this mainly so I could easily develop with grunt watch:chrome.

See also #217.

I imagine that getting access to the active game client instance will be something other modules want, so that should probably be split out of alwaysStack.js into something more generic and reusable in utils.js. I'm not completely sure what the best thing to do for this would be, but I had something like this in mind:


// in utils.js
GS.whenGameClientReady = function (callback) {
    mtgRoom.conn.bind('gameServerHello', function (msg) {
        var gameClient;

        gameClient = _.find(mtgRoom.games, function (game) {
            return game.gameAddress === msg.data.gameServerAddress;
        }, this);

        callback(gameClient);
    });
};

// in alwaysStack.js, instead of the mtgRoom.conn.bind call
GS.whenGameClientReady(function (client) {
    if (GS.get_option('always_stack')) {
        client.clientConnection.bind('moveCards', alwaysStack);
    }
});
michaeljb commented 10 years ago

Didn't realize GS.getGameClient() was a thing earlier, so GS.whenGameClientReady() could be simpler:

// in utils.js
GS.whenGameClientReady = function (callback) {
    mtgRoom.conn.bind('gameServerHello', function (msg) {
        callback(GS.getGameClient());
    });
};
aiannacc commented 10 years ago

No, your implementation is better than the existing getGameClient(). Not only is it more elegant, but it's probably less likely to fail if, say, Goko hasn't yet populated whatever variables mtgRoom.getCurrentTable() needs. Something along those lines appears to be causing the error that theblankman observed.

michaeljb commented 10 years ago

OK, whenGameClientReady uses my implementation in the commit I just added: 31079870d33b2a2df77a2e00fd4be8dd40247679

Since it depends on the gameServerHello message I'm not sure the other calls can be swapped out but I'll look into it tonight

aiannacc commented 10 years ago

Verified that auto-stack option works correctly. Verified that the game client gets correctly passed to the callback method.

There's a small conflict with #214, but it's easy to resolve.

I'm wondering whether getGameClient() should even continue to exist. It's dangerous, and it's hard to imagine why you would want the current game client just this once. Adding a listener for every new gameClient with whenGameClientReady() seems like it would always be the better solution.

You've confused my github algorithms by embedding #227 in here. Maybe I will just wait for you to fix the minor issues with both before doing the merge. I promise to get to them faster than I did this last time around.

michaeljb commented 10 years ago

You've confused my github algorithms by embedding #227 in here. Maybe I will just wait for you to fix the minor issues with both before doing the merge. I promise to get to them faster than I did this last time around.

I'm having trouble with both giving 'assemble' the 'title' argument and getting a setup where I don't have to enter my private key passphrase every time the task runs (kind of defeats the automation of it all), so #227 is a bit held up. I can cherry-pick the relevant commits here and submit a new pull request.

michaeljb commented 10 years ago

Resubmitted in #232 without the grunt stuff since I don't have that working yet.