Don't use KVO in GoBoardPosition to react to changes in GoNodeModel

[herzbube]

GoBoardPosition currently uses KVO to react to changes in GoNodeModel and to update its properties numberOfBoardPositions and currentBoardPosition. This "magic" is convenient, because clients that add or discard nodes to/from GoNodeModel do not need to also update GoBoardPosition. On the negative side, the updating logic triggered by KVO is difficult to understand and requires extensive documentation, because it involves assumptions about the behaviour of other parts of the code (at least GoGame, ChangeAndDiscardCommand and DiscardAndPlayCommand).

After various changes that were required to support game variations and board setup in multiple nodes, one specific error case was now found to exist:

• Since commit 120a9d337df2902eb39a161d0a784f58058940d4 the GoNode method modifyBoard is invoked only after the node is added to GoNodeModel. Before the commit the order was reversed.
  • This change was necessary because Zobrist hashes are now stored in GoNode and calculated by the modifyBoard method. In order for a Zobrist hash to be calculated its predecessor hash must be known. The predecessor hash is stored in a node's parent node, and that parent node is known only after a new node has been added to the game tree.
• The KVO magic in GoBoardPosition is already triggered when the new node is added to GoNodeModel, i.e. before the GoNode method modifyBoard is invoked.
• So this means that when KVO observers of the GoBoardPosition property currentBoardPosition are triggered, they now see Go model objects in the wrong state because the GoNode method modifyBoard has not been invoked yet.
  • If the reaction of such a KVO observer is delayed for some reason (e.g. it waits for a long running action to complete, or it triggers a drawing update which only occurs in the next run loop cycle) the wrong board state at KVO trigger time may have no consequences, but this can only be seen as coincidence.
• However, the specific error case that was found was in GoBoardPosition's own KVO-triggered updating logic: The updating logic tries to set the GoGame property nextMoveColor. For that to work the GoGame property setupFirstMoveColor must be up-to-date. However, setupFirstMoveColor is set by the GoNode method modifyBoard (from within GoNodeSetup), and as we have seen that method has not been invoked yet. This means that GoBoardPosition's updating logic can easily set the GoGame property nextMoveColor with the wrong value.

After this analysis, it seems best to no longer use KVO in GoBoardPosition to react to changes in GoNodeModel. Although this removes some convenience, it also removes a difficult to understand piece of code and therefore improves overall maintainability.

[herzbube]

The following KVO-triggered updating logic in GoBoardPosition must be replaced:

• Node is added
  • numberOfBoardPositions needs to be adjusted
  • currentBoardPosition needs to be changed +1 if before the node was added it was referring to the leaf node in GoNodeModel
• Node is removed
  • numberOfBoardPositions needs to be adjusted

The following actors must be changed accordingly:

• Node is added
  • There is only one method in GoNodeModel that adds a new node: appendNode:.
  • The method is invoked in only two places: The GoGame methods play: and pass.
• Node is removed
  • GoNodeModel has several methods to discard nodes, but all of them ultimately invoke the same primitive method: discardNodesFromIndex:
  • The method is invoked in only two places: ChangeAndDiscardCommand and DiscardAndPlayCommand.

[herzbube]

To avoid similar issues with an inconsistent model state in the future, all KVO observing of GoBoardPosition properties currentBoardPosition and numberOfBoardPositions should be replaced. Instead observers should react to notifications posted to the default notification centre, and the actors outlined in the previous comment should explicitly post those notifications when all Go model objects have received their correct state updates.

New notifications:

• currentBoardPositionDidChange
• numberOfBoardPositionsDidChange