leaguevine / leaguevine-ultistats

MIT License
18 stars 4 forks source link

Undo button #25

Closed cboulay closed 12 years ago

cboulay commented 12 years ago

These are two basic ways to implement an undo:

  1. Pop the last event and reverse its effect on the gamestate
    • Requires writing code to reverse every event type
  2. Remove the last event and rebuild the gamestate by bootstrapping from the remaining events in the stack.
    • Requires writing code to bootstrap the gamestate from previous events. It is unclear to me at this time how many previous events are needed.

The second method could be reused IF the user will ever use downloaded game events, such as in a play-by-play watching situation. Also, undoing certain events (e.g. pickup) in method 1 is ambiguous since we don't know what the previous state was without looking at the new last event. So, even if we use method 1, we still need a little of method 2 anyway.

I will try to do method 2.

mliu7 commented 12 years ago

Sounds good to me. Is this how you did it for Ultistats for iPhone too?

cboulay commented 12 years ago

No on the iphone version it was a bit simpler. I didn't track who pulled, and I didn't track any of the other team's stats. Reversing events was considerably simpler, but it was still annoying.

Anyway, I failed to mention that I will try to get the WebSQL sync working before doing this.

mliu7 commented 12 years ago

Ah, gotcha. Yeah, having a local DB seems necessary to bootstrap the game state quickly.

mliu7 commented 12 years ago

For anyone who is curious why this issue has not been resolved, it's because to implement this right we'll need a history of all the events that have been created. To do that, Chad is creating a WebSQL plugin that will store this data and allow us to query on the events to bootstrap the game state appropriately. See Issue #6.

Only after Issue #6 is complete can we realistically tackle this issue.

mliu7 commented 12 years ago

Chad - do you have an ETA for this undo button? Almost all the emails I get about Ultistats are about the undo button, and I'd like to tell them approximately how long they'll have to wait.

mliu7 commented 12 years ago

I just thought of an issue with bootstrapping the game state if we're using a WebSQL backend that synchronizes in the background. If two users on two separate devices are entering events for the same game with one user entering events for one team and the other user entering events for the other team (both users are entering "unknown" for all the other teams' players), our backend will automatically merge the events into a single stream of events. For instance, this game was created by two different users on two different devices each working on just their own team's play-by-play.

For instance, the "unknown threw a compelte pass to unknown" events are merged into more specific events that happened at approximately the same time such as "Brandon Malacek threw a complete pass to Michael Miller", and thus that first event is effectively removed. This might cause some problems with editing or removing events, but it's not as big of a problem as bootstrapping the game state. Because this functionality has proven to be very useful for taking AUDL stats, I think this will be pretty commonly used, and we can expect multiple score reporters for a lot of games. Here's an instance where we could run into some problems where multiple score reporters are entering events that don't actually get merged because they are slightly different:

Imagine the real events go something like this:

And imagine the play by play from users 1 and 2 get entered like this:

cboulay commented 12 years ago

The app never downloads events. Events can only be created locally and uploaded. Undoing an event will delete the local version immediately and send a command to delete it from the server.

What you are saying is that the server merges events that it thinks are the same. a) Which event id is kept? User 1's or User 2's? b) What if both users are entering events for both teams?

When we talked about the possibility of multiple users entering data for the same game we decided that each user's remote representation of that game would be stored independently. If you want to merge events from >1 user I think the best way to do that is to create a NEW representation of the game using a NEW user (game_123_merge or something). Then if one of the users decides to destroy an event, they will destroy their event only. You can then re-merge the game using the updated independent representations.

I hope your event_id column is 64-bit.

cboulay commented 12 years ago

I was thinking about the bootstrapping a little more.

I still like it in theory, but I'm worried it'll be too slow. I can't think of a way to make sure the on_field and off_field lists are correct without going through the entirety of the game. Iterating through every event in a game just to undo an event later in the game might not be worth the cleanliness and robustness this type of method affords.

Thus I am simply going to reverse all of the gamestate changes associated with an event when it is undone. Since this method wouldn't necessarily require access to all of the game's events, then we can do this without the WebSQL plugin.

I'm in the middle of something right now, but I'll start working on the undo button on the main branch shortly.

mliu7 commented 12 years ago

Excellent. Good to know about Ultistats not downloading events from the server.

For the merging events, what I said was a bit incorrect... Sorry. Both events are kept, but our server internally keeps track of which events are duplicates, and the duplicate events are not used when calculating stats or showing the play-by-play. If two events are "merged" on the server, both events are still accessible via the API and both keep their original IDs.

If both users are entering events for both teams, it will be even easier for our server to determine duplicates, and again, both sets of events will still be available through the API.

I am still planning on adding a few attributes to API objects that tell consumers who created the events and when, but I haven't figured out a nice way to do this. A user's ID is not used or accessible anywhere else in the API (or on Leaguevine), so I need to think of a nice way to introduce a user's ID to this API, and then have that ID relate to their player instance in a way that isn't confusing.

Thanks for the update on your plan for reversing the gamestate changes when undo is clicked. I agree that would be a lot faster.

One more thing to keep in mind when downloading events - what about if a user stops using one device and then logs on to a second device? If it's the same account, can we then download those events? While using this app myself, there have been two occasions where my phone battery ran out (I often forget to charge it at night) and then I switched to someone else's phone, but after logging into the new phone it doesn't know who is substituted in or out. This causes the players who were substituted in on the original phone to remain substituted in until I sub them in and then out on the second phone. It's not super important that we fix this, but it's just another thing to think about.

mliu7 commented 12 years ago

Forgot to comment on your 64-bit ID comment. Don't worry, we use 64-bit Bigints so we'll be fine.

cboulay commented 12 years ago

The undo is done in the undo branch. Can you fix up the CSS/HTML for the undo button before merging? Also be sure to restore the appcache that was an individual commit that you can revert

mliu7 commented 12 years ago

Ok, awesome. This looks solid, and I'll play around with it a bit more tonight before merging. It'll be ready for people to use this weekend. Good work!

mliu7 commented 12 years ago

Ok, so in general this undo functionality seems to be working which is great, but there are still a few issues that I think we should resolve before I feel comfortable merging. Here's what I've come across so far:

  1. The players in the substitution events are being entered as "null" instead of the player's ID
  2. After clicking a player button, the previous play area is being updated only after the response is received from the server. Before, it was updated right after the user clicked a button, making the app feel a lot more responsive.
  3. After clicking the undo button, the previous play area is being updated only after the response comes back from the server, just like clicking a player button.
  4. Clicking the undo button a second time before receiving a success response from the first click causes the second click to do nothing.
mliu7 commented 12 years ago

One more issue:

  1. Clicking on an "Unknown" player causes an error (Uncaught TypeError: Cannot call method 'player_tap' of undefined - trackedgame.js:760)
cboulay commented 12 years ago

If the user were to click undo twice, and the app responded immediately, but then the API rejected the first undo (for whatever reason), what state should the app now be in? In any case, 2, 3, and 4 are non-issues once the WebSQL plugin is being used.

1 and 1 are separate issues that I'll fix.

mliu7 commented 12 years ago

Ok, cool, thanks for the clarifications. So I guess I should wait to merge the webSQL plugin before we merge this undo button in order for 2, 3, and 4 to be resolved nicely.

For clicking the undo twice with the API rejecting the first undo, I think the app should be in the state as if the undo was successful. There might be an incorrect pass or something still floating around, but at least the user won't be confused and can continue entering in game data.

One more thing I just thought of. What happens when a user creates an event but then clicks undo before the API returns a success for the event?

cboulay commented 12 years ago

I'll take this opportunity to explain a certain philosophy I have regarding this app. The UI does not echo back what was tapped, it reflects the PERSISTED state of the data. Tapping manipulates the data then the UI reflects the new state of the data. For the sake of argument let me take this to the extreme: if (communication with) the data store (local or remote) has become corrupt, should the UI still tell the user that the passes, undos, substitutions, and scores were all entered successfully even though absolutely nothing was saved?

The eventual evolution of this app will persist everything locally. Since local persistence is much faster, adhering to my philosophy won't impact the user experience. In the mean time, I can patch it so the UI displays the result of the tap even if the result isn't stored anywhere... but this might lead to inconsistencies as you mentioned.

To answer you last question, right now the game state is updated even if the event isn't saved. However, the event doesn't get added to the collection of events until after it has been saved. This causes a problem if the save fails as the game state will reflect that the event was entered but the stack of events will not. As it is now, clicking undo will undo the last event in the collection. I have a TODO there so the gamestate only becomes updated once the event is saved (line 213).

mliu7 commented 12 years ago

I think your philosophy on the UI always reflecting the persisted state is good, and I agree that we should not resort to one-off exceptions of this rule as long as we can avoid it. And for your hypothetical worst case, I agree that we shouldn't falsely tell people things are being saved when they're not.

I think for now, it's better if we don't make patches for the UI to display the result of the tap even if it isn't stored anywhere.

Thanks for helping me see the light. I think I understand this a bit better now.