Changes according to review

client/CPlayerInterface.cpp

@@ -347,7 +347,7 @@ void CPlayerInterface::heroMoved(const TryMoveHero & details, bool verbose)
 	if (!hero)
 		return;
 
-	movementController->heroMoved(hero, details);
+	movementController->onTryMoveHero(hero, details);
 }
 
 void CPlayerInterface::heroKilled(const CGHeroInstance* hero)
@@ -937,7 +937,7 @@ void CPlayerInterface::showInfoDialog(EInfoWindowMode type, const std::string &t
 		adventureInt->showInfoBoxMessage(components, text, timer);
 
 		// abort movement, if any. Strictly speaking unnecessary, but prevents some edge cases, like movement sound on visiting Magi Hut with "show messages in status window" on
-		movementController->movementAbortRequested();
+		movementController->requestMovementAbort();
 
 		if (makingTurn && GH.windows().count() > 0 && LOCPLINT == this)
 			CCS->soundh->playSound(static_cast<soundBase::soundID>(soundID));
@@ -984,7 +984,7 @@ void CPlayerInterface::showInfoDialog(const std::string &text, const std::vector
 	{
 		CCS->soundh->playSound(static_cast<soundBase::soundID>(soundID));
 		showingDialog->set(true);
-		movementController->movementAbortRequested(); // interrupt movement to show dialog
+		movementController->requestMovementAbort(); // interrupt movement to show dialog
 		GH.windows().pushWindow(temp);
 	}
 	else
@@ -1007,7 +1007,7 @@ void CPlayerInterface::showYesNoDialog(const std::string &text, CFunctionList<vo
 {
 	boost::unique_lock<boost::recursive_mutex> un(*pim);
 
-	movementController->movementAbortRequested();
+	movementController->requestMovementAbort();
 	LOCPLINT->showingDialog->setn(true);
 	CInfoWindow::showYesNoDialog(text, components, onYes, onNo, playerID);
 }
@@ -1017,7 +1017,7 @@ void CPlayerInterface::showBlockingDialog( const std::string &text, const std::v
 	EVENT_HANDLER_CALLED_BY_CLIENT;
 	waitWhileDialog();
 
-	movementController->movementAbortRequested();
+	movementController->requestMovementAbort();
 	CCS->soundh->playSound(static_cast<soundBase::soundID>(soundID));
 
 	if (!selection && cancel) //simple yes/no dialog
@@ -1182,7 +1182,7 @@ void CPlayerInterface::moveHero( const CGHeroInstance *h, const CGPath& path )
 	if (localState->isHeroSleeping(h))
 		localState->setHeroAwaken(h);
 
-	movementController->movementStartRequested(h, path);
+	movementController->requestMovementStart(h, path);
 }
 
 void CPlayerInterface::showGarrisonDialog( const CArmedInstance *up, const CGHeroInstance *down, bool removableUnits, QueryID queryID)

client/HeroMovementController.cpp

@@ -60,11 +60,17 @@ void HeroMovementController::onBattleStarted()
 	// when battle starts, game will send battleStart pack *before* movement confirmation
 	// and since network thread wait for battle intro to play, movement confirmation will only happen after intro
 	// leading to several bugs, such as blocked input during intro
-	movementAbortRequested();
+	requestMovementAbort();
 }
 
 void HeroMovementController::showTeleportDialog(const CGHeroInstance * hero, TeleportChannelID channel, TTeleportExitsList exits, bool impassable, QueryID askID)
 {
+	if (impassable || exits.empty()) //FIXME: why we even have this dialog in such case?
+	{
+		LOCPLINT->cb->selectionMade(-1, askID);
+		return;
+	}
+
 	// Player entered teleporter
 	// Check whether hero that has entered teleporter has paths that goes through teleporter and select appropriate exit
 	// othervice, ask server to select one randomly by sending invalid (-1) value as answer
@@ -139,7 +145,7 @@ void HeroMovementController::updatePath(const CGHeroInstance * hero, const TryMo
 	}
 }
 
-void HeroMovementController::heroMoved(const CGHeroInstance * hero, const TryMoveHero & details)
+void HeroMovementController::onTryMoveHero(const CGHeroInstance * hero, const TryMoveHero & details)
 {
 	// Server initiated movement -> start movement animation
 	// Note that this movement is not necessarily of owned heroes - other players movement will also pass through this method
@@ -167,7 +173,7 @@ void HeroMovementController::heroMoved(const CGHeroInstance * hero, const TryMov
 	if(details.stopMovement())
 	{
 		if(duringMovement)
-			endHeroMove(hero);
+			endMove(hero);
 		return;
 	}
 
@@ -233,27 +239,23 @@ void HeroMovementController::onMoveHeroApplied()
 	bool wantStop = stoppingMovement;
 	bool canStop = !canMove || canHeroStopAtNode(LOCPLINT->localState->getPath(hero).currNode());
 
-	if(!canMove)
-	{
-		endHeroMove(hero);
-	}
-	else if(wantStop && canStop)
+	if(!canMove || (wantStop && canStop))
 	{
-		endHeroMove(hero);
+		endMove(hero);
 	}
 	else
 	{
-		moveHeroOnce(hero, LOCPLINT->localState->getPath(hero));
+		moveOnce(hero, LOCPLINT->localState->getPath(hero));
 	}
 }
 
-void HeroMovementController::movementAbortRequested()
+void HeroMovementController::requestMovementAbort()
 {
 	if(duringMovement)
-		endHeroMove(currentlyMovingHero);
+		endMove(currentlyMovingHero);
 }
 
-void HeroMovementController::endHeroMove(const CGHeroInstance * hero)
+void HeroMovementController::endMove(const CGHeroInstance * hero)
 {
 	assert(duringMovement == true);
 	assert(currentlyMovingHero != nullptr);
@@ -331,17 +333,17 @@ bool HeroMovementController::canHeroStopAtNode(const CGPathNode & node) const
 	return true;
 }
 
-void HeroMovementController::movementStartRequested(const CGHeroInstance * h, const CGPath & path)
+void HeroMovementController::requestMovementStart(const CGHeroInstance * h, const CGPath & path)
 {
 	assert(duringMovement == false);
 	duringMovement = true;
 	currentlyMovingHero = h;
 
 	CCS->curh->hide();
-	moveHeroOnce(h, path);
+	moveOnce(h, path);
 }
 
-void HeroMovementController::moveHeroOnce(const CGHeroInstance * h, const CGPath & path)
+void HeroMovementController::moveOnce(const CGHeroInstance * h, const CGPath & path)
 {
 	// Moves hero once, sends request to server and immediately returns
 	// movement alongside paths will be done on receiving response from server

client/HeroMovementController.h

@@ -43,9 +43,9 @@ class HeroMovementController
 	void updatePath(const CGHeroInstance * hero, const TryMoveHero & details);
 
 	/// Moves hero 1 tile / path node
-	void moveHeroOnce(const CGHeroInstance * h, const CGPath & path);
+	void moveOnce(const CGHeroInstance * h, const CGPath & path);
 
-	void endHeroMove(const CGHeroInstance * h);
+	void endMove(const CGHeroInstance * h);
 
 	AudioPath getMovementSoundFor(const CGHeroInstance * hero, int3 posPrev, int3 posNext, EPathNodeAction moveType);
 	void updateMovementSound(const CGHeroInstance * hero, int3 posPrev, int3 posNext, EPathNodeAction moveType);
@@ -66,9 +66,9 @@ public:
 	void onPlayerTurnStarted();
 	void onBattleStarted();
 	void showTeleportDialog(const CGHeroInstance * hero, TeleportChannelID channel, TTeleportExitsList exits, bool impassable, QueryID askID);
-	void heroMoved(const CGHeroInstance * hero, const TryMoveHero & details);
+	void onTryMoveHero(const CGHeroInstance * hero, const TryMoveHero & details);
 
 	// UI handlers
-	void movementStartRequested(const CGHeroInstance * h, const CGPath & path);
-	void movementAbortRequested();
+	void requestMovementStart(const CGHeroInstance * h, const CGPath & path);
+	void requestMovementAbort();
 };

server/CGameHandler.cpp

@@ -1139,7 +1139,7 @@ bool CGameHandler::moveHero(ObjectInstanceID hid, int3 dst, ui8 teleporting, boo
 	if(h->movementPointsRemaining() < cost && dst != h->pos && !teleporting)
 		complainRet("Hero doesn't have any movement points left!");
 
-	if (transit && !canFly && !CGTeleport::isTeleport(t.topVisitableObj()))
+	if (transit && !canFly && !(canWalkOnSea && t.terType->isWater()))
 		complainRet("Hero cannot transit over this tile!");
 
 	//several generic blocks of code
@@ -1259,7 +1259,7 @@ bool CGameHandler::moveHero(ObjectInstanceID hid, int3 dst, ui8 teleporting, boo
 			if (CGTeleport::isTeleport(t.topVisitableObj()))
 				visitDest = DONT_VISIT_DEST;
 
-			if (canFly)
+			if (canFly || (canWalkOnSea && t.terType->isWater()))
 			{
 				lookForGuards = IGNORE_GUARDS;
 				visitDest = DONT_VISIT_DEST;