Skip to content

Commit 483a468

Browse files
alexvinsalexvins
committed
Fixes for code review issues
1 parent 784f6b9 commit 483a468

18 files changed

+196
-209
lines changed

‎client/battle/CBattleInterface.cpp

Lines changed: 26 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -563,24 +563,24 @@ void CBattleInterface::deactivate()
563563

564564
void CBattleInterface::keyPressed(const SDL_KeyboardEvent & key)
565565
{
566-
if (key.keysym.sym == SDLK_q && key.state == SDL_PRESSED)
566+
if(key.keysym.sym == SDLK_q && key.state == SDL_PRESSED)
567567
{
568-
if (settings["battle"]["showQueue"].Bool()) //hide queue
568+
if(settings["battle"]["showQueue"].Bool()) //hide queue
569569
hideQueue();
570570
else
571571
showQueue();
572572

573573
}
574-
else if (key.keysym.sym == SDLK_f && key.state == SDL_PRESSED)
574+
else if(key.keysym.sym == SDLK_f && key.state == SDL_PRESSED)
575575
{
576576
enterCreatureCastingMode();
577577
}
578-
else if (key.keysym.sym == SDLK_ESCAPE)
578+
else if(key.keysym.sym == SDLK_ESCAPE)
579579
{
580580
if(!battleActionsStarted)
581581
CCS->soundh->stopSound(battleIntroSoundChannel);
582582
else
583-
endCastingSpell();
583+
endCastingSpell();
584584
}
585585
}
586586
void CBattleInterface::mouseMoved(const SDL_MouseMotionEvent &sEvent)
@@ -788,14 +788,14 @@ void CBattleInterface::bOptionsf()
788788

789789
void CBattleInterface::bSurrenderf()
790790
{
791-
if (spellDestSelectMode) //we are casting a spell
791+
if(spellDestSelectMode) //we are casting a spell
792792
return;
793793

794794
int cost = curInt->cb->battleGetSurrenderCost();
795-
if (cost >= 0)
795+
if(cost >= 0)
796796
{
797797
std::string enemyHeroName = curInt->cb->battleGetEnemyHero().name;
798-
if (enemyHeroName.empty())
798+
if(enemyHeroName.empty())
799799
{
800800
logGlobal->warn("Surrender performed without enemy hero, should not happen!");
801801
enemyHeroName = "#ENEMY#";
@@ -1462,14 +1462,9 @@ void CBattleInterface::displayEffect(ui32 effect, BattleHex destTile)
14621462
addNewAnim(new CEffectAnimation(this, customAnim, destTile));
14631463
}
14641464

1465-
void CBattleInterface::displaySpellCast(SpellID spellID, BattleHex destinationTile)
1465+
void CBattleInterface::displaySpellAnimationQueue(const CSpell::TAnimationQueue & q, BattleHex destinationTile)
14661466
{
1467-
const CSpell * spell = spellID.toSpell();
1468-
1469-
if(spell == nullptr)
1470-
return;
1471-
1472-
for(const CSpell::TAnimation & animation : spell->animationInfo.cast)
1467+
for(const CSpell::TAnimation & animation : q)
14731468
{
14741469
if(animation.pause > 0)
14751470
addNewAnim(new CDummyAnimation(this, animation.pause));
@@ -1478,37 +1473,28 @@ void CBattleInterface::displaySpellCast(SpellID spellID, BattleHex destinationTi
14781473
}
14791474
}
14801475

1481-
void CBattleInterface::displaySpellEffect(SpellID spellID, BattleHex destinationTile)
1476+
void CBattleInterface::displaySpellCast(SpellID spellID, BattleHex destinationTile)
14821477
{
1483-
const CSpell *spell = spellID.toSpell();
1478+
const CSpell * spell = spellID.toSpell();
14841479

1485-
if(spell == nullptr)
1486-
return;
1480+
if(spell)
1481+
displaySpellAnimationQueue(spell->animationInfo.cast, destinationTile);
1482+
}
14871483

1488-
for(const CSpell::TAnimation & animation : spell->animationInfo.affect)
1489-
{
1490-
if(animation.pause > 0)
1491-
addNewAnim(new CDummyAnimation(this, animation.pause));
1492-
else
1493-
addNewAnim(new CEffectAnimation(this, animation.resourceName, destinationTile, false, animation.verticalPosition == VerticalPosition::BOTTOM));
1484+
void CBattleInterface::displaySpellEffect(SpellID spellID, BattleHex destinationTile)
1485+
{
1486+
const CSpell *spell = spellID.toSpell();
14941487

1495-
}
1488+
if(spell)
1489+
displaySpellAnimationQueue(spell->animationInfo.affect, destinationTile);
14961490
}
14971491

14981492
void CBattleInterface::displaySpellHit(SpellID spellID, BattleHex destinationTile)
14991493
{
15001494
const CSpell * spell = spellID.toSpell();
15011495

1502-
if(spell == nullptr)
1503-
return;
1504-
1505-
for(const CSpell::TAnimation & animation : spell->animationInfo.hit)
1506-
{
1507-
if(animation.pause > 0)
1508-
addNewAnim(new CDummyAnimation(this, animation.pause));
1509-
else
1510-
addNewAnim(new CEffectAnimation(this, animation.resourceName, destinationTile, false, animation.verticalPosition == VerticalPosition::BOTTOM));
1511-
}
1496+
if(spell)
1497+
displaySpellAnimationQueue(spell->animationInfo.hit, destinationTile);
15121498
}
15131499

15141500
void CBattleInterface::battleTriggerEffect(const BattleTriggerEffect & bte)
@@ -1704,13 +1690,12 @@ void CBattleInterface::enterCreatureCastingMode()
17041690
spells::Target target;
17051691
target.emplace_back();
17061692

1707-
17081693
spells::BattleCast cast(curInt->cb.get(), caster, spells::Mode::CREATURE_ACTIVE, spell);
17091694

17101695
auto m = spell->battleMechanics(&cast);
17111696
spells::detail::ProblemImpl ignored;
17121697

1713-
const bool isCastingPossible = m->canBeCastAt(ignored, target);
1698+
const bool isCastingPossible = m->canBeCastAt(target, ignored);
17141699

17151700
if (isCastingPossible)
17161701
{
@@ -1762,7 +1747,7 @@ void CBattleInterface::reorderPossibleActionsPriority(const CStack * stack, Mous
17621747
case PossiblePlayerBattleAction::OBSTACLE:
17631748
if(!stack->hasBonusOfType(Bonus::NO_SPELLCAST_BY_DEFAULT) && context == MouseHoveredHexContext::OCCUPIED_HEX)
17641749
return 1;
1765-
else
1750+
else
17661751
return 100;//bottom priority
17671752
break;
17681753
case PossiblePlayerBattleAction::RANDOM_GENIE_SPELL:
@@ -2543,7 +2528,7 @@ bool CBattleInterface::isCastingPossibleHere(const CStack *sactive, const CStack
25432528
auto m = sp->battleMechanics(&cast);
25442529
spells::detail::ProblemImpl problem; //todo: display problem in status bar
25452530

2546-
isCastingPossible = m->canBeCastAt(problem, target);
2531+
isCastingPossible = m->canBeCastAt(target, problem);
25472532
}
25482533
}
25492534
else
@@ -3022,7 +3007,7 @@ void CBattleInterface::show(SDL_Surface *to)
30223007
showProjectiles(to);
30233008

30243009
if(battleActionsStarted)
3025-
updateBattleAnimations();
3010+
updateBattleAnimations();
30263011

30273012
SDL_SetClipRect(to, &buf); //restoring previous clip_rect
30283013

‎client/battle/CBattleInterface.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -358,6 +358,7 @@ class CBattleInterface : public WindowBase
358358

359359
void displayEffect(ui32 effect, BattleHex destTile); //displays custom effect on the battlefield
360360

361+
void displaySpellAnimationQueue(const CSpell::TAnimationQueue & q, BattleHex destinationTile);
361362
void displaySpellCast(SpellID spellID, BattleHex destinationTile); //displays spell`s cast animation
362363
void displaySpellEffect(SpellID spellID, BattleHex destinationTile); //displays spell`s affected animation
363364
void displaySpellHit(SpellID spellID, BattleHex destinationTile); //displays spell`s affected animation

‎client/widgets/CGarrisonInt.cpp

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -368,11 +368,11 @@ void CGarrisonSlot::update()
368368
}
369369

370370
CGarrisonSlot::CGarrisonSlot(CGarrisonInt * Owner, int x, int y, SlotID IID, CGarrisonSlot::EGarrisonType Upg, const CStackInstance * creature_)
371-
: ID(IID),
372-
owner(Owner),
373-
myStack(creature_),
374-
creature(creature_ ? creature_->type : nullptr),
375-
upg(Upg)
371+
: ID(IID),
372+
owner(Owner),
373+
myStack(creature_),
374+
creature(creature_ ? creature_->type : nullptr),
375+
upg(Upg)
376376
{
377377
OBJECT_CONSTRUCTION_CAPTURING(255-DISPOSE);
378378

@@ -381,8 +381,8 @@ CGarrisonSlot::CGarrisonSlot(CGarrisonInt * Owner, int x, int y, SlotID IID, CGa
381381

382382
std::string imgName = owner->smallIcons ? "cprsmall" : "TWCRPORT";
383383

384-
creatureImage = std::make_shared<CAnimImage>(imgName, 0);
385-
creatureImage->disable();
384+
creatureImage = std::make_shared<CAnimImage>(imgName, 0);
385+
creatureImage->disable();
386386

387387
selectionImage = std::make_shared<CAnimImage>(imgName, 1);
388388
selectionImage->disable();

‎client/windows/CCastleInterface.cpp

Lines changed: 11 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -111,12 +111,14 @@ void CBuildingRect::hover(bool on)
111111

112112
void CBuildingRect::clickLeft(tribool down, bool previousState)
113113
{
114-
if( previousState && getBuilding() && area && !down && (parent->selectedBuilding==this))
115-
if (!CSDL_Ext::isTransparent(area, GH.current->motion.x-pos.x, GH.current->motion.y-pos.y) ) //inside building image
114+
if(previousState && getBuilding() && area && !down && (parent->selectedBuilding==this))
115+
{
116+
if(!CSDL_Ext::isTransparent(area, GH.current->motion.x-pos.x, GH.current->motion.y-pos.y)) //inside building image
116117
{
117118
auto building = getBuilding();
118119
parent->buildingClicked(building->bid, building->subId, building->upgrade);
119-
}
120+
}
121+
}
120122
}
121123

122124
void CBuildingRect::clickRight(tribool down, bool previousState)
@@ -606,8 +608,7 @@ void CCastleBuildings::recreate()
606608

607609
const CStructure * toAdd = *boost::max_element(entry.second, [=](const CStructure * a, const CStructure * b)
608610
{
609-
return build->getDistance(a->building->bid)
610-
< build->getDistance(b->building->bid);
611+
return build->getDistance(a->building->bid) < build->getDistance(b->building->bid);
611612
});
612613

613614
buildings.push_back(std::make_shared<CBuildingRect>(this, town, toAdd));
@@ -866,8 +867,8 @@ void CCastleBuildings::enterFountain(const BuildingID & building, BuildingSubID:
866867
boost::algorithm::replace_first(hasProduced, "%s", buildingName);
867868
}
868869

869-
bool isMysticPondOrItsUpgrade = subID == BuildingSubID::MYSTIC_POND
870-
|| (upgrades != BuildingID::NONE
870+
bool isMysticPondOrItsUpgrade = subID == BuildingSubID::MYSTIC_POND
871+
|| (upgrades != BuildingID::NONE
871872
&& town->town->buildings.find(BuildingID(upgrades))->second->subId == BuildingSubID::MYSTIC_POND);
872873

873874
if(upgrades != BuildingID::NONE)
@@ -880,9 +881,9 @@ void CCastleBuildings::enterFountain(const BuildingID & building, BuildingSubID:
880881
else //Mystic Pond produced something;
881882
{
882883
descr += "\n\n" + hasProduced;
883-
boost::algorithm::replace_first(descr,"%s",CGI->generaltexth->restypes[town->bonusValue.first]);
884-
boost::algorithm::replace_first(descr,"%d",boost::lexical_cast<std::string>(town->bonusValue.second));
885-
}
884+
boost::algorithm::replace_first(descr,"%s",CGI->generaltexth->restypes[town->bonusValue.first]);
885+
boost::algorithm::replace_first(descr,"%d",boost::lexical_cast<std::string>(town->bonusValue.second));
886+
}
886887
}
887888
LOCPLINT->showInfoDialog(descr, comps);
888889
}

‎include/vcmi/events/SubscriptionRegistry.h

Lines changed: 16 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -32,20 +32,20 @@ class SubscriptionRegistry : public boost::noncopyable
3232
using PostHandler = std::function<void(const E &)>;
3333
using BusTag = const void *;
3434

35-
std::unique_ptr<EventSubscription> subscribeBefore(BusTag tag, PreHandler && cb)
35+
std::unique_ptr<EventSubscription> subscribeBefore(BusTag tag, PreHandler && handler)
3636
{
3737
boost::unique_lock<boost::shared_mutex> lock(mutex);
3838

39-
auto storage = std::make_shared<PreHandlerStorage>(std::move(cb));
39+
auto storage = std::make_shared<PreHandlerStorage>(std::move(handler));
4040
preHandlers[tag].push_back(storage);
4141
return make_unique<PreSubscription>(tag, storage);
4242
}
4343

44-
std::unique_ptr<EventSubscription> subscribeAfter(BusTag tag, PostHandler && cb)
44+
std::unique_ptr<EventSubscription> subscribeAfter(BusTag tag, PostHandler && handler)
4545
{
4646
boost::unique_lock<boost::shared_mutex> lock(mutex);
4747

48-
auto storage = std::make_shared<PostHandlerStorage>(std::move(cb));
48+
auto storage = std::make_shared<PostHandlerStorage>(std::move(handler));
4949
postHandlers[tag].push_back(storage);
5050
return make_unique<PostSubscription>(tag, storage);
5151
}
@@ -84,18 +84,18 @@ class SubscriptionRegistry : public boost::noncopyable
8484
class HandlerStorage
8585
{
8686
public:
87-
explicit HandlerStorage(T && cb_)
88-
: cb(cb_)
87+
explicit HandlerStorage(T && handler_)
88+
: handler(handler_)
8989
{
9090
}
9191

9292
STRONG_INLINE
9393
void operator()(E & event)
9494
{
95-
cb(event);
95+
handler(event);
9696
}
9797
private:
98-
T cb;
98+
T handler;
9999
};
100100

101101
using PreHandlerStorage = HandlerStorage<PreHandler>;
@@ -104,39 +104,39 @@ class SubscriptionRegistry : public boost::noncopyable
104104
class PreSubscription : public EventSubscription
105105
{
106106
public:
107-
PreSubscription(BusTag tag_, std::shared_ptr<PreHandlerStorage> cb_)
108-
: cb(cb_),
107+
PreSubscription(BusTag tag_, std::shared_ptr<PreHandlerStorage> handler_)
108+
: handler(handler_),
109109
tag(tag_)
110110
{
111111
}
112112

113113
virtual ~PreSubscription()
114114
{
115115
auto registry = E::getRegistry();
116-
registry->unsubscribe(tag, cb, registry->preHandlers);
116+
registry->unsubscribe(tag, handler, registry->preHandlers);
117117
}
118118
private:
119119
BusTag tag;
120-
std::shared_ptr<PreHandlerStorage> cb;
120+
std::shared_ptr<PreHandlerStorage> handler;
121121
};
122122

123123
class PostSubscription : public EventSubscription
124124
{
125125
public:
126-
PostSubscription(BusTag tag_, std::shared_ptr<PostHandlerStorage> cb_)
127-
: cb(cb_),
126+
PostSubscription(BusTag tag_, std::shared_ptr<PostHandlerStorage> handler_)
127+
: handler(handler_),
128128
tag(tag_)
129129
{
130130
}
131131

132132
virtual ~PostSubscription()
133133
{
134134
auto registry = E::getRegistry();
135-
registry->unsubscribe(tag, cb, registry->postHandlers);
135+
registry->unsubscribe(tag, handler, registry->postHandlers);
136136
}
137137
private:
138138
BusTag tag;
139-
std::shared_ptr<PostHandlerStorage> cb;
139+
std::shared_ptr<PostHandlerStorage> handler;
140140
};
141141

142142
boost::shared_mutex mutex;

‎lib/CModHandler.cpp

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -697,7 +697,7 @@ bool CModHandler::checkDependencies(const std::vector <TModID> & input) const
697697

698698
for(const TModID & dep : mod.dependencies)
699699
{
700-
if (!vstd::contains(input, dep))
700+
if(!vstd::contains(input, dep))
701701
{
702702
logMod->error("Error: Mod %s requires missing %s!", mod.name, dep);
703703
return false;
@@ -706,14 +706,14 @@ bool CModHandler::checkDependencies(const std::vector <TModID> & input) const
706706

707707
for(const TModID & conflicting : mod.conflicts)
708708
{
709-
if (vstd::contains(input, conflicting))
709+
if(vstd::contains(input, conflicting))
710710
{
711711
logMod->error("Error: Mod %s conflicts with %s!", mod.name, allMods.at(conflicting).name);
712712
return false;
713713
}
714714
}
715715

716-
if (hasCircularDependency(id))
716+
if(hasCircularDependency(id))
717717
return false;
718718
}
719719
return true;
@@ -732,7 +732,7 @@ std::vector <TModID> CModHandler::resolveDependencies(std::vector <TModID> modsT
732732
{
733733
logMod->error("Mod '%s' will not work: it depends on mod '%s', which is not installed.", mod.name, dependency);
734734
res = false; //continue iterations, since we should show all errors for the current mod.
735-
}
735+
}
736736
}
737737
return res;
738738
};
@@ -745,19 +745,19 @@ std::vector <TModID> CModHandler::resolveDependencies(std::vector <TModID> modsT
745745
brokenMods.push_back(mod);
746746
}
747747
if(!brokenMods.empty())
748-
{
748+
{
749749
vstd::erase_if(modsToResolve, [&](TModID mid)
750750
{
751751
return brokenMods.end() != std::find(brokenMods.begin(), brokenMods.end(), mid);
752752
});
753753
brokenMods.clear();
754754
continue;
755-
}
756-
break;
757755
}
756+
break;
757+
}
758758
boost::range::sort(modsToResolve);
759759
return modsToResolve;
760-
}
760+
}
761761

762762
std::vector<std::string> CModHandler::getModList(std::string path)
763763
{

0 commit comments

Comments
 (0)