Fix 2139 captured spell scroll descriptions

client/widgets/CArtifactHolder.cpp

@@ -32,7 +32,7 @@
  */
 
 CArtPlace::CArtPlace(Point position, const CArtifactInstance * Art):
-    locked(false), picked(false), marked(false), ourArt(Art)
+	locked(false), picked(false), marked(false), ourArt(Art)
 {
 	pos += position;
 	pos.w = pos.h = 44;
@@ -180,7 +180,7 @@ void CArtPlace::clickLeft(tribool down, bool previousState)
 						if(srcInBackpack && srcInSameHero)
 						{
 							if(!ourArt								//cannot move from backpack to AFTER backpack -> combined with vstd::amin above it will guarantee that dest is at most the last artifact
-							  || ourOwner->commonInfo->src.slotID < ourOwner->commonInfo->dst.slotID) //rearranging arts in backpack after taking src artifact, the dest id will be shifted
+								|| ourOwner->commonInfo->src.slotID < ourOwner->commonInfo->dst.slotID) //rearranging arts in backpack after taking src artifact, the dest id will be shifted
 								vstd::advance(ourOwner->commonInfo->dst.slotID, -1);
 						}
 						if(srcInSameHero && ourOwner->commonInfo->dst.slotID == ourOwner->commonInfo->src.slotID) //we came to src == dst
@@ -386,70 +386,36 @@ void CArtPlace::setArtifact(const CArtifactInstance *art)
 		image->disable();
 		text = std::string();
 		hoverText = CGI->generaltexth->allTexts[507];
+		return;
 	}
-	else
-	{
-		image->enable();
-		image->setFrame(locked ? ArtifactID::ART_LOCK : art->artType->iconIndex);
 
-		std::string artDesc = ourArt->artType->Description();
-		if (vstd::contains (artDesc, '{'))
-			text = artDesc;
-		else
-			text = '{' + ourArt->artType->Name() + "}\n\n" + artDesc; //workaround for new artifacts with single name, turns it to H3-style
+	image->enable();
+	image->setFrame(locked ? ArtifactID::ART_LOCK : art->artType->iconIndex);
 
-		if(art->artType->id == ArtifactID::SPELL_SCROLL)
-		{
-			// we expect scroll description to be like this: This scroll contains the [spell name] spell which is added into your spell book for as long as you carry the scroll.
-			// so we want to replace text in [...] with a spell name
-			// however other language versions don't have name placeholder at all, so we have to be careful
-			int spellID = art->getGivenSpellID();
-			size_t nameStart = text.find_first_of('[');
-			size_t nameEnd = text.find_first_of(']', nameStart);
-			if(spellID >= 0)
-			{
-				if(nameStart != std::string::npos  &&  nameEnd != std::string::npos)
-					text = text.replace(nameStart, nameEnd - nameStart + 1, CGI->spellh->objects[spellID]->name);
+	text = art->getEffectiveDescription(ourOwner->curHero);
 
-				//add spell component info (used to provide a pic in r-click popup)
-				baseType = CComponent::spell;
-				type = spellID;
-				bonusValue = 0;
-			}
-		}
-		else
+	if(art->artType->id == ArtifactID::SPELL_SCROLL)
+	{
+		int spellID = art->getGivenSpellID();
+		if(spellID >= 0)
 		{
-			baseType = CComponent::artifact;
-			type = art->artType->id;
+			//add spell component info (used to provide a pic in r-click popup)
+			baseType = CComponent::spell;
+			type = spellID;
 			bonusValue = 0;
 		}
-		if (art->artType->constituents) //display info about components of combined artifact
-		{
-			//TODO
-		}
-		else if (art->artType->constituentOf.size()) //display info about set
-		{
-			std::string artList;
-			auto combinedArt = art->artType->constituentOf[0];
-			text += "\n\n";
-			text += "{" + combinedArt->Name() + "}";
-			int wornArtifacts = 0;
-			for (auto a : *combinedArt->constituents) //TODO: can the artifact be a part of more than one set?
-			{
-				artList += "\n" + a->Name();
-				if (ourOwner->curHero->hasArt(a->id, true))
-					wornArtifacts++;
-			}
-			text += " (" + boost::str(boost::format("%d") % wornArtifacts) +  " / " +
-				boost::str(boost::format("%d") % combinedArt->constituents->size()) + ")" + artList;
-			//TODO: fancy colors and fonts for this text
-		}
-
-		if (locked) // Locks should appear as empty.
-			hoverText = CGI->generaltexth->allTexts[507];
-		else
-			hoverText = boost::str(boost::format(CGI->generaltexth->heroscrn[1]) % ourArt->artType->Name());
 	}
+	else
+	{
+		baseType = CComponent::artifact;
+		type = art->artType->id;
+		bonusValue = 0;
+	}
+
+	if (locked) // Locks should appear as empty.
+		hoverText = CGI->generaltexth->allTexts[507];
+	else
+		hoverText = boost::str(boost::format(CGI->generaltexth->heroscrn[1]) % ourArt->artType->Name());
 }
 
 void CArtifactsOfHero::SCommonPart::reset()
@@ -811,7 +777,7 @@ void CArtifactsOfHero::artifactMoved(const ArtifactLocation &src, const Artifact
 	}
 	else if(src.slot >= GameConstants::BACKPACK_START &&
 	        src.slot <  commonInfo->src.slotID &&
-			src.isHolder(commonInfo->src.AOH->curHero)) //artifact taken from before currently picked one
+			    src.isHolder(commonInfo->src.AOH->curHero)) //artifact taken from before currently picked one
 	{
 		//int fixedSlot = src.hero->getArtPos(commonInfo->src.art);
 		vstd::advance(commonInfo->src.slotID, -1);
@@ -825,14 +791,14 @@ void CArtifactsOfHero::artifactMoved(const ArtifactLocation &src, const Artifact
 	}
 
 	updateParentWindow();
- 	int shift = 0;
+	int shift = 0;
 // 	if(dst.slot >= Arts::BACKPACK_START && dst.slot - Arts::BACKPACK_START < backpackPos)
 // 		shift++;
 //
- 	if(src.slot < GameConstants::BACKPACK_START  &&  dst.slot - GameConstants::BACKPACK_START < backpackPos)
+	if(src.slot < GameConstants::BACKPACK_START  &&  dst.slot - GameConstants::BACKPACK_START < backpackPos)
 		shift++;
 	if(dst.slot < GameConstants::BACKPACK_START  &&  src.slot - GameConstants::BACKPACK_START < backpackPos)
- 		shift--;
+		shift--;
 
 	if( (isCurHeroSrc && src.slot >= GameConstants::BACKPACK_START)
 	 || (isCurHeroDst && dst.slot >= GameConstants::BACKPACK_START) )

client/widgets/CComponent.cpp

@@ -7,6 +7,7 @@
 #include "../CMessage.h"
 #include "../CGameInfo.h"
 #include "../widgets/Images.h"
+#include "../widgets/CArtifactHolder.h"
 #include "../windows/CAdvmapInterface.h"
 
 #include "../../lib/CArtHandler.h"
@@ -144,14 +145,26 @@ size_t CComponent::getIndex()
 
 std::string CComponent::getDescription()
 {
-	switch (compType)
+	switch(compType)
 	{
 	case primskill:  return (subtype < 4)? CGI->generaltexth->arraytxt[2+subtype] //Primary skill
 										 : CGI->generaltexth->allTexts[149]; //mana
 	case secskill:   return CGI->generaltexth->skillInfoTexts[subtype][val-1];
 	case resource:   return CGI->generaltexth->allTexts[242];
 	case creature:   return "";
-	case artifact:   return CGI->arth->artifacts[subtype]->Description();
+	case artifact:
+	{
+		std::unique_ptr<CArtifactInstance> art;
+		if (subtype != ArtifactID::SPELL_SCROLL)
+		{
+			art.reset(CArtifactInstance::createNewArtifactInstance(subtype));
+		}
+		else
+		{
+			art.reset(CArtifactInstance::createScroll(static_cast<SpellID>(val)));
+		}
+		return art->getEffectiveDescription();
+	}
 	case experience: return CGI->generaltexth->allTexts[241];
 	case spell:      return CGI->spellh->objects[subtype]->getLevelInfo(val).description;
 	case morale:     return CGI->generaltexth->heroscrn[ 4 - (val>0) + (val<0)];
@@ -166,7 +179,7 @@ std::string CComponent::getDescription()
 
 std::string CComponent::getSubtitle()
 {
-	if (!perDay)
+	if(!perDay)
 		return getSubtitleInternal();
 
 	std::string ret = CGI->generaltexth->allTexts[3];

client/windows/CTradeWindow.cpp

@@ -269,7 +269,7 @@ void CTradeWindow::CTradeableItem::clickRight(tribool down, bool previousState)
 		case ARTIFACT_TYPE:
 		case ARTIFACT_PLACEHOLDER:
 			if(id >= 0)
-				adventureInt->handleRightClick(CGI->arth->artifacts[id]->Description(), down);
+				adventureInt->handleRightClick(hlp->getEffectiveDescription(), down);
 			break;
 		}
 	}
@@ -500,14 +500,14 @@ void CTradeWindow::getPositionsFor(std::vector<Rect> &poss, bool Left, EType typ
 		int h, w, x, y, dx, dy;
 		int leftToRightOffset;
 		getBaseForPositions(type, dx, dy, x, y, h, w, !Left, leftToRightOffset);
-		
-		const std::vector<Rect> tmp = 
+
+		const std::vector<Rect> tmp =
 		{
 			genRect(h, w, x, y), genRect(h, w, x + dx, y), genRect(h, w, x + 2*dx, y),
 			genRect(h, w, x, y + dy), genRect(h, w, x + dx, y + dy), genRect(h, w, x + 2*dx, y + dy),
-			genRect(h, w, x + dx, y + 2*dy)			
+			genRect(h, w, x + dx, y + 2*dy)
 		};
-		
+
 		vstd::concatenate(poss, tmp);
 
 		if(!Left)

lib/CArtHandler.cpp

@@ -738,9 +738,14 @@ std::string CArtifactInstance::nodeName() const
 }
 
 CArtifactInstance * CArtifactInstance::createScroll( const CSpell *s)
+{
+	return createScroll(s->id);
+}
+
+CArtifactInstance *CArtifactInstance::createScroll(SpellID sid)
 {
 	auto ret = new CArtifactInstance(VLC->arth->artifacts[ArtifactID::SPELL_SCROLL]);
-	auto b = new Bonus(Bonus::PERMANENT, Bonus::SPELL, Bonus::ARTIFACT_INSTANCE, -1, ArtifactID::SPELL_SCROLL, s->id);
+	auto b = new Bonus(Bonus::PERMANENT, Bonus::SPELL, Bonus::ARTIFACT_INSTANCE, -1, ArtifactID::SPELL_SCROLL, sid);
 	ret->addNewBonus(b);
 	return ret;
 }
@@ -752,6 +757,48 @@ void CArtifactInstance::init()
 	setNodeType(ARTIFACT_INSTANCE);
 }
 
+std::string CArtifactInstance::getEffectiveDescription(
+	const CGHeroInstance *hero) const
+{
+	std::string text = this->artType->Description();
+	if (!vstd::contains(text, '{'))
+		text = '{' + this->artType->Name() + "}\n\n" + text; //workaround for new artifacts with single name, turns it to H3-style
+
+	if(this->artType->id == ArtifactID::SPELL_SCROLL)
+	{
+		// we expect scroll description to be like this: This scroll contains the [spell name] spell which is added into your spell book for as long as you carry the scroll.
+		// so we want to replace text in [...] with a spell name
+		// however other language versions don't have name placeholder at all, so we have to be careful
+		int spellID = this->getGivenSpellID();
+		size_t nameStart = text.find_first_of('[');
+		size_t nameEnd = text.find_first_of(']', nameStart);
+		if(spellID >= 0)
+		{
+			if(nameStart != std::string::npos  &&  nameEnd != std::string::npos)
+				text = text.replace(nameStart, nameEnd - nameStart + 1, VLC->spellh->objects[spellID]->name);
+		}
+	}
+	else if (hero && this->artType->constituentOf.size()) //display info about set
+	{
+		std::string artList;
+		auto combinedArt = this->artType->constituentOf[0];
+		text += "\n\n";
+		text += "{" + combinedArt->Name() + "}";
+		int wornArtifacts = 0;
+		for (auto a : *combinedArt->constituents) //TODO: can the artifact be a part of more than one set?
+		{
+			artList += "\n" + a->Name();
+			if (hero->hasArt(a->id, true))
+				wornArtifacts++;
+		}
+		text += " (" + boost::str(boost::format("%d") % wornArtifacts) +  " / " +
+			boost::str(boost::format("%d") % combinedArt->constituents->size()) + ")" + artList;
+		//TODO: fancy colors and fonts for this text
+	}
+
+	return text;
+}
+
 ArtifactPosition CArtifactInstance::firstAvailableSlot(const CArtifactSet *h) const
 {
 	for(auto slot : artType->possibleSlots.at(h->bearerType()))
@@ -900,7 +947,7 @@ SpellID CArtifactInstance::getGivenSpellID() const
 	const Bonus * b = getBonusLocalFirst(Selector::type(Bonus::SPELL));
 	if(!b)
 	{
-        logGlobal->warnStream() << "Warning: " << nodeName() << " doesn't bear any spell!";
+		logGlobal->warnStream() << "Warning: " << nodeName() << " doesn't bear any spell!";
 		return SpellID::NONE;
 	}
 	return SpellID(b->subtype);

lib/CArtHandler.h

@@ -119,6 +119,7 @@ public:
 	void deserializationFix();
 	void setType(CArtifact *Art);
 
+	std::string getEffectiveDescription(const CGHeroInstance *hero = nullptr) const;
 	ArtifactPosition firstAvailableSlot(const CArtifactSet *h) const;
 	ArtifactPosition firstBackpackSlot(const CArtifactSet *h) const;
 	SpellID getGivenSpellID() const; //to be used with scrolls (and similar arts), -1 if none
@@ -143,6 +144,7 @@ public:
 	}
 
 	static CArtifactInstance *createScroll(const CSpell *s);
+	static CArtifactInstance *createScroll(SpellID sid);
 	static CArtifactInstance *createNewArtifactInstance(CArtifact *Art);
 	static CArtifactInstance *createNewArtifactInstance(int aid);
 };

server/CGameHandler.cpp

@@ -471,7 +471,7 @@ void CGameHandler::endBattle(int3 tile, const CGHeroInstance *hero1, const CGHer
 	const CArmedInstance *bEndArmy2 = gs->curB->sides.at(1).armyObject;
 	const BattleResult::EResult result = battleResult.get()->result;
 
-	auto findBattleQuery = [this] () -> std::shared_ptr<CBattleQuery>
+	auto findBattleQuery = [this]() -> std::shared_ptr<CBattleQuery>
 	{
 		for(auto &q : queries.allQueries())
 		{
@@ -526,66 +526,70 @@ void CGameHandler::endBattle(int3 tile, const CGHeroInstance *hero1, const CGHer
 	}
 
 
-	std::vector<ui32> arts; //display them in window
+	std::vector<const CArtifactInstance *> arts; //display them in window
 
-	if (result == BattleResult::NORMAL && finishingBattle->winnerHero)
+	if(result == BattleResult::NORMAL && finishingBattle->winnerHero)
 	{
-		if (finishingBattle->loserHero)
+		auto sendMoveArtifact = [&](const CArtifactInstance *art, MoveArtifact *ma)
 		{
-			auto artifactsWorn = finishingBattle->loserHero->artifactsWorn; //TODO: wrap it into a function, somehow (boost::variant -_-)
+			arts.push_back(art);
+			ma->dst = ArtifactLocation(finishingBattle->winnerHero, art->firstAvailableSlot(finishingBattle->winnerHero));
+			sendAndApply(ma);
+		};
+		if(finishingBattle->loserHero)
+		{
+			//TODO: wrap it into a function, somehow (boost::variant -_-)
+			auto artifactsWorn = finishingBattle->loserHero->artifactsWorn;
 			for (auto artSlot : artifactsWorn)
 			{
 				MoveArtifact ma;
-				ma.src = ArtifactLocation (finishingBattle->loserHero, artSlot.first);
+				ma.src = ArtifactLocation(finishingBattle->loserHero, artSlot.first);
 				const CArtifactInstance * art =  ma.src.getArt();
-				if (art && !art->artType->isBig() && art->artType->id != ArtifactID::SPELLBOOK) // don't move war machines or locked arts (spellbook)
+				if(art && !art->artType->isBig() &&
+				    art->artType->id != ArtifactID::SPELLBOOK)
+						// don't move war machines or locked arts (spellbook)
 				{
-					arts.push_back (art->artType->id);
-					ma.dst = ArtifactLocation (finishingBattle->winnerHero, art->firstAvailableSlot(finishingBattle->winnerHero));
-					sendAndApply(&ma);
+					sendMoveArtifact(art, &ma);
 				}
 			}
-			while (!finishingBattle->loserHero->artifactsInBackpack.empty())
+			while(!finishingBattle->loserHero->artifactsInBackpack.empty())
 			{
 				//we assume that no big artifacts can be found
 				MoveArtifact ma;
-				ma.src = ArtifactLocation (finishingBattle->loserHero,
+				ma.src = ArtifactLocation(finishingBattle->loserHero,
 					ArtifactPosition(GameConstants::BACKPACK_START)); //backpack automatically shifts arts to beginning
 				const CArtifactInstance * art =  ma.src.getArt();
-				arts.push_back (art->artType->id);
-				ma.dst = ArtifactLocation (finishingBattle->winnerHero, art->firstAvailableSlot(finishingBattle->winnerHero));
-				sendAndApply(&ma);
+				if(art->artType->id != ArtifactID::GRAIL) //grail may not be won
+				{
+					sendMoveArtifact(art, &ma);
+				}
 			}
-			if (finishingBattle->loserHero->commander) //TODO: what if commanders belong to no hero?
+			if(finishingBattle->loserHero->commander) //TODO: what if commanders belong to no hero?
 			{
 				artifactsWorn = finishingBattle->loserHero->commander->artifactsWorn;
-				for (auto artSlot : artifactsWorn)
+				for(auto artSlot : artifactsWorn)
 				{
 					MoveArtifact ma;
-					ma.src = ArtifactLocation (finishingBattle->loserHero->commander.get(), artSlot.first);
+					ma.src = ArtifactLocation(finishingBattle->loserHero->commander.get(), artSlot.first);
 					const CArtifactInstance * art =  ma.src.getArt();
 					if (art && !art->artType->isBig())
 					{
-						arts.push_back (art->artType->id);
-						ma.dst = ArtifactLocation (finishingBattle->winnerHero, art->firstAvailableSlot(finishingBattle->winnerHero));
-						sendAndApply(&ma);
+						sendMoveArtifact(art, &ma);
 					}
 				}
 			}
 		}
-		for (auto armySlot : gs->curB->battleGetArmyObject(!battleResult.data->winner)->stacks)
+		for(auto armySlot : gs->curB->battleGetArmyObject(!battleResult.data->winner)->stacks)
 		{
 			auto artifactsWorn = armySlot.second->artifactsWorn;
 			for (auto artSlot : artifactsWorn)
 			{
 				MoveArtifact ma;
-				ma.src = ArtifactLocation (armySlot.second, artSlot.first);
+				ma.src = ArtifactLocation(armySlot.second, artSlot.first);
 				const CArtifactInstance * art =  ma.src.getArt();
 				if (art && !art->artType->isBig())
 				{
-					arts.push_back (art->artType->id);
-					ma.dst = ArtifactLocation (finishingBattle->winnerHero, art->firstAvailableSlot(finishingBattle->winnerHero));
-					sendAndApply(&ma);
+					sendMoveArtifact(art, &ma);
 				}
 			}
 		}
@@ -593,23 +597,25 @@ void CGameHandler::endBattle(int3 tile, const CGHeroInstance *hero1, const CGHer
 
 	sendAndApply(battleResult.data); //after this point casualties objects are destroyed
 
-	if (arts.size()) //display loot
+	if(arts.size()) //display loot
 	{
 		InfoWindow iw;
 		iw.player = finishingBattle->winnerHero->tempOwner;
 
 		iw.text.addTxt (MetaString::GENERAL_TXT, 30); //You have captured enemy artifact
 
-		for (auto id : arts) //TODO; separate function to display loot for various ojects?
+		for(auto art : arts) //TODO; separate function to display loot for various ojects?
 		{
-			iw.components.push_back (Component (Component::ARTIFACT, id, 0, 0));
+			iw.components.push_back(Component(
+				Component::ARTIFACT, art->artType->id,
+				art->artType->id == ArtifactID::SPELL_SCROLL? art->getGivenSpellID() : 0, 0));
 			if(iw.components.size() >= 14)
 			{
 				sendAndApply(&iw);
 				iw.components.clear();
 			}
 		}
-		if (iw.components.size())
+		if(iw.components.size())
 		{
 			sendAndApply(&iw);
 		}