dripton / Slugathon

Clone of the old Avalon Hill fantasy wargame Titan, using Python / PyGTK / Twisted
https://github.com/dripton/Slugathon/wiki
8 stars 2 forks source link

Clarify 47f40b29f826a4dfec1e3165fdc53c71960ea30e #177

Open orbisvicis opened 10 years ago

orbisvicis commented 10 years ago

Commit 47f40b29f826a4dfec1e3165fdc53c71960ea30e. As per the annotation I've added, the else branch seems pointless:

def _add_wfp(self, game):
    wfp = self.wfps.get(game.name)
    if wfp is not None:
        if wfp.has_user_ref_count:
            return
        else:
            # this does absolutely nothing !!
            del wfp
    wfp = WaitingForPlayers.WaitingForPlayers(self.user, self.playername,
                                              game, self.parent_window)
    self.wfps[game.name] = wfp

I don't really follow what this commit tries to prevent, but since has_user_ref_count is gone in gtk3, I'd like to make the following modifications. I don't know how to reproduce the original bug so I'd like your input.

def cb_wfp_destroyed(self, wfp):
    del self.wfps[wfp.game.name]

def _add_wfp(self, game):
    wfp = self.wfps.get(game.name)
    if wfp is None:
        wfp = WaitingForPlayers.WaitingForPlayers(self.user, self.playername,
                                                  game, self.parent_window)
        self.wfps[game.name] = wfp

def _remove_wfp(self, game_name):
    if game_name is self.wfps:
        wfp = self.wfps[game_name]
        wfp.destroy()

p.s. I'd appreciate if you wouldn't merge these changes, so I can avoid any conflicts at my end.

dripton commented 10 years ago

Looking at the change in WaitingForPlayers.py, the bug was that a RemoveGame action on any game would cause WaitingForPlayers dialogs for all games to be dismissed. It should only dismiss the ones for that particular game.

I agree that "del wfp" doesn't seem to accomplish anything. It only destroys the local variable. It needs to be removed from self.wfps instead. Your change looks correct to me, though I haven't tested it.