leaguevine / leaguevine-ultistats

MIT License
18 stars 4 forks source link

Player buttons are not refreshed right away after sub-out #77

Closed mliu7 closed 12 years ago

mliu7 commented 12 years ago

When players are substituted out during a point, the player buttons aren't updated immediately.

To reproduce:

mliu7 commented 12 years ago

Also, I notice that if you click on the button for the player that had been subbed out, the previous play at the top says "unknown".

And looking at the request that gets sent, the removed player's ID is still sent in the event instead of that field being null.

cboulay commented 12 years ago

I'll write out the process here to help me or anyone else identify the problem.

  1. Tapping a player in the roster triggers its click event which calls TrackedGame.Views.RosterItem's "remove_me"
  2. Line 619. remove_me clears the view AND remove's the model from its collection. We must now consider everything bound to the collection's "remove" event.
  3. Lines 483-484 & 495-496, the collections' remove events are bound to trackedgame.add_removed_player_to_other_collection on 270
  4. Line 270: add_removed_player_to_other_collection
    • The removed model is cloned into new_model. I can't remember why I couldn't use the original model.
    • new_model is added to the appropriate collection. *Things bound to roster collections' add: 579, 768
    • The substitution event is created and saved and added to the stack of events (Line 298). *Things bound to gameevents.add: 462, 685
  5. Line 579: collection.add is bound to a re-render of the roster list. This works as advertised.
  6. 768: collection.add is bound to TrackedGame.Views.TeamPlayerArea's render (776)
    • I notice that the render on 776 is a little weird in how it uses this.onf. I should probably get rid of this.onf
  7. (continues from 4) 462 calls "event_added" on 331
  8. event_added (on 331)
    • Changes the trackedgame's "team_in_possession_ix". *Look for things bound to the model or this attribute changing.
    • The game (not trackedgame, but Game) .team_ix_score is updated. *Look for things bound to .game's score/generic change.
    • calls this.state_for_event
  9. state_for_event (on 354)
    • might update "current_state". What is bound to that?
    • might update "player_in_possession_id". What is bound to that?
  10. (continues from 4) 685: renders the play-by-play
    • searches through the onfield players (from both teams) to find the name of the player to be used for the display. This results in the player name being "unknown" if the player has been removed from the onfield collection (i.e. if the event is to sub a player out or if we reach the event through a series of undos where we eventually undo player substitutions.

It seems the source of the problem is in step 6. If it's not an easy fix, I'll have to look into this tomorrow whenever my daughter takes a nap.

mliu7 commented 12 years ago

Thanks for the detailed explanation. I'll assign you to this issue since you clearly have a good idea of how to fix this already.

cboulay commented 12 years ago

Ah, it's a bit stupid. So adding a player back onfield gets rid of this problem. The reason why is that I was only updating the player buttons when the onfield collection was being added to, not when it was being removed from. I never guessed that a player would be removed and not be replaced. It's a sad team when someone gets injured and there's no one left on the bench to replace them. I fixed the bug and I'll commit it in a moment, but I'll leave this issue open because the play-by-play says "unknown stepped off the field". I'm too tired to find that now.

This did make me think of another problem though: what should happen when the player with the disc subs out on an injury? Should the entering player get possession of the disc through a "pick up" event? Philosophically I liked thinking of the "pick up" as a choice; a catch block is both a block and a choice to pick it up thus it merits two events, even if they happen in one motion. Subbing in for the player in possession means that you do not have a choice in picking it up. I guess you have a choice as to whether or not you sub in so that should satisfy me. I still need to check for this situation and add the event.

cboulay commented 12 years ago

I made new issues for the other things I found, so I can close this.

mliu7 commented 12 years ago

Cool, thanks for the fix.

While taking stats, I can see a situation where 7 people are subbed in, but the user realizes one of those 7 is on the sideline, so they sub that person out. After they do that, though, the action may be too fast and they don't have time to figure out which player they are missing, so they just leave it with 6 on the field. I had to do this once or twice when I was tracking a team I didn't know.

When a player with the disc is subbed out, I think we need to go to the "Who picked up?" prompt in order to eliminate all the edge cases. Most of the time, you'll just select the person who was substituted in, but I can think of situations where having done this automatically would result in a problem. For instance, if two players were subbed out and one had the disc, and then two players were subbed in, we don't know which of those two players have the disc.