From abea2a09b9da20b1278915ccf2b183570e0632a5 Mon Sep 17 00:00:00 2001
From: tibetiroka <68112292+tibetiroka@users.noreply.github.com>
Date: Tue, 3 Dec 2024 01:09:16 +0100
Subject: [PATCH] refactor(ui): Extract magic numbers from Dialog (#10323)

Co-authored-by: Rising Leaf <85687254+RisingLeaf@users.noreply.github.com>
---
 source/Dialog.cpp           | 132 +++++++++++++++++++++++-------------
 source/Dialog.h             |   4 +-
 source/text/WrappedText.cpp |   4 +-
 3 files changed, 91 insertions(+), 49 deletions(-)

diff --git a/source/Dialog.cpp b/source/Dialog.cpp
index dd50d386332a..119cd9385658 100644
--- a/source/Dialog.cpp
+++ b/source/Dialog.cpp
@@ -40,9 +40,6 @@ this program. If not, see <https://www.gnu.org/licenses/>.
 using namespace std;
 
 namespace {
-	const int WIDTH = 250;
-	const int WIDE_WIDTH = 450;
-
 	// Map any conceivable numeric keypad keys to their ASCII values. Most of
 	// these will presumably only exist on special programming keyboards.
 	const map<SDL_Keycode, char> KEY_MAP = {
@@ -85,6 +82,40 @@ namespace {
 		{SDLK_KP_SPACE, ' '},
 		{SDLK_KP_VERTICALBAR, '|'}
 	};
+
+	// The width of the margin on the right/left sides of the dialog. This area is part of the sprite,
+	// but shouldn't have any text or other graphics rendered over it. (It's mostly transparent.)
+	constexpr double LEFT_MARGIN = 20;
+	constexpr double RIGHT_MARGIN = 20;
+	constexpr double HORIZONTAL_MARGIN = LEFT_MARGIN + RIGHT_MARGIN;
+	// The margin on the right/left sides of the button sprite. The bottom segment also includes a button
+	// that uses the same value.
+	constexpr double BUTTON_LEFT_MARGIN = 10;
+	constexpr double BUTTON_RIGHT_MARGIN = 10;
+	constexpr double BUTTON_HORIZONTAL_MARGIN = BUTTON_LEFT_MARGIN + BUTTON_RIGHT_MARGIN;
+	// The margin on the top/bottom sides of the button sprite. The bottom segment also includes a button
+	// that uses the same value.
+	constexpr double BUTTON_TOP_MARGIN = 10;
+	constexpr double BUTTON_BOTTOM_MARGIN = 10;
+	constexpr double BUTTON_VERTICAL_MARGIN = BUTTON_TOP_MARGIN + BUTTON_BOTTOM_MARGIN;
+	// The width of the padding used on the left/right sides of each segment, in pixels.
+	constexpr double LEFT_PADDING = 10;
+	constexpr double RIGHT_PADDING = 10;
+	constexpr double HORIZONTAL_PADDING = RIGHT_PADDING + LEFT_PADDING;
+	// The height of the padding used by the top/bottom segment, in pixels.
+	constexpr double TOP_PADDING = 10;
+	constexpr double BOTTOM_PADDING = 10;
+	constexpr double VERTICAL_PADDING = TOP_PADDING + BOTTOM_PADDING;
+	// The width of the padding at the beginning/end of an input field.
+	constexpr double INPUT_LEFT_PADDING = 5;
+	constexpr double INPUT_RIGHT_PADDING = 5;
+	constexpr double INPUT_HORIZONTAL_PADDING = INPUT_LEFT_PADDING + INPUT_RIGHT_PADDING;
+	// The height of the padding at the top/bottom of an input field.
+	constexpr double INPUT_TOP_PADDING = 2;
+	constexpr double INPUT_BOTTOM_PADDING = 2;
+	constexpr double INPUT_VERTICAL_PADDING = INPUT_TOP_PADDING + INPUT_BOTTOM_PADDING;
+	// The height of an input field in pixels.
+	constexpr double INPUT_HEIGHT = 20;
 }
 
 
@@ -129,10 +160,9 @@ void Dialog::Draw()
 	const Sprite *bottom = SpriteSet::Get(isWide ? "ui/dialog bottom wide" : "ui/dialog bottom");
 	const Sprite *cancel = SpriteSet::Get("ui/dialog cancel");
 
-	// Get the position of the top of this dialog, and of the text and input.
-	Point pos(0., (top->Height() + height * middle->Height() + bottom->Height()) * -.5f);
-	Point textPos(Width() * -.5 + 10., pos.Y() + 20.);
-	Point inputPos = Point(0., -70.) - pos;
+	// Get the position of the top of this dialog, and of the input.
+	Point pos(0., (top->Height() + extensionCount * middle->Height() + bottom->Height()) * -.5);
+	Point inputPos = Point(0., -(cancel->Height() + INPUT_HEIGHT)) - pos;
 
 	// Draw the top section of the dialog box.
 	pos.Y() += top->Height() * .5;
@@ -140,7 +170,7 @@ void Dialog::Draw()
 	pos.Y() += top->Height() * .5;
 
 	// The middle section is duplicated depending on how long the text is.
-	for(int i = 0; i < height; ++i)
+	for(int i = 0; i < extensionCount; ++i)
 	{
 		pos.Y() += middle->Height() * .5;
 		SpriteShader::Draw(middle, pos);
@@ -151,43 +181,44 @@ void Dialog::Draw()
 	const Font &font = FontSet::Get(14);
 	pos.Y() += bottom->Height() * .5;
 	SpriteShader::Draw(bottom, pos);
-	pos.Y() += bottom->Height() * .5 - 25.;
+	pos.Y() += (bottom->Height() - cancel->Height()) * .5;
 
 	// Draw the buttons, including optionally the cancel button.
 	const Color &bright = *GameData::Colors().Get("bright");
 	const Color &dim = *GameData::Colors().Get("medium");
 	const Color &back = *GameData::Colors().Get("faint");
 	const Color &inactive = *GameData::Colors().Get("inactive");
+	const string okText = isMission ? "Accept" : "OK";
+	okPos = pos + Point((top->Width() - RIGHT_MARGIN - cancel->Width()) * .5, 0.);
+	Point labelPos(
+		okPos.X() - .5 * font.Width(okText),
+		okPos.Y() - .5 * font.Height());
+	font.Draw(okText, labelPos, isOkDisabled ? inactive : (okIsActive ? bright : dim));
 	if(canCancel)
 	{
 		string cancelText = isMission ? "Decline" : "Cancel";
-		cancelPos = pos + Point(isWide ? 110. : 10., 0.);
+		cancelPos = pos + Point(okPos.X() - cancel->Width() + BUTTON_RIGHT_MARGIN, 0.);
 		SpriteShader::Draw(cancel, cancelPos);
-		Point labelPos(
-			cancelPos.X() - .5 * font.Width(cancelText),
-			cancelPos.Y() - .5 * font.Height());
+		labelPos = {
+				cancelPos.X() - .5 * font.Width(cancelText),
+				cancelPos.Y() - .5 * font.Height()};
 		font.Draw(cancelText, labelPos, !okIsActive ? bright : dim);
 	}
-	string okText = isMission ? "Accept" : "OK";
-	okPos = pos + Point(isWide ? 190. : 90., 0.);
-	Point labelPos(
-		okPos.X() - .5 * font.Width(okText),
-		okPos.Y() - .5 * font.Height());
-	font.Draw(okText, labelPos, isOkDisabled ? inactive : (okIsActive ? bright : dim));
 
 	// Draw the input, if any.
 	if(!isMission && (intFun || stringFun))
 	{
-		FillShader::Fill(inputPos, Point(Width() - 20., 20.), back);
+		FillShader::Fill(inputPos, Point(Width() - HORIZONTAL_PADDING, INPUT_HEIGHT), back);
 
 		Point stringPos(
-			inputPos.X() - (Width() - 20) * .5 + 5.,
+			inputPos.X() - (Width() - HORIZONTAL_PADDING) * .5 + INPUT_LEFT_PADDING,
 			inputPos.Y() - .5 * font.Height());
-		const auto inputText = DisplayText(input, {Width() - 30, Truncate::FRONT});
+		const auto inputText = DisplayText(input, {static_cast<int>(Width() - HORIZONTAL_PADDING - INPUT_HORIZONTAL_PADDING),
+				Truncate::FRONT});
 		font.Draw(inputText, stringPos, bright);
 
-		Point barPos(stringPos.X() + font.FormattedWidth(inputText) + 2., inputPos.Y());
-		FillShader::Fill(barPos, Point(1., 16.), dim);
+		Point barPos(stringPos.X() + font.FormattedWidth(inputText) + INPUT_TOP_PADDING, inputPos.Y());
+		FillShader::Fill(barPos, Point(1., INPUT_HEIGHT - INPUT_VERTICAL_PADDING), dim);
 	}
 }
 
@@ -295,8 +326,12 @@ bool Dialog::Click(int x, int y, int clicks)
 {
 	Point clickPos(x, y);
 
+	const Sprite *sprite = SpriteSet::Get("ui/dialog cancel");
+	const double toleranceX = (sprite->Width() - BUTTON_HORIZONTAL_MARGIN) / 2.;
+	const double toleranceY = (sprite->Height() - BUTTON_VERTICAL_MARGIN) / 2.;
+
 	Point ok = clickPos - okPos;
-	if(fabs(ok.X()) < 40. && fabs(ok.Y()) < 20.)
+	if(fabs(ok.X()) < toleranceX && fabs(ok.Y()) < toleranceY)
 	{
 		okIsActive = true;
 		return DoKey(SDLK_RETURN);
@@ -305,7 +340,7 @@ bool Dialog::Click(int x, int y, int clicks)
 	if(canCancel)
 	{
 		Point cancel = clickPos - cancelPos;
-		if(fabs(cancel.X()) < 40. && fabs(cancel.Y()) < 20.)
+		if(fabs(cancel.X()) < toleranceX && fabs(cancel.Y()) < toleranceY)
 		{
 			okIsActive = false;
 			return DoKey(SDLK_RETURN);
@@ -327,7 +362,7 @@ void Dialog::Init(const string &message, Truncate truncate, bool canCancel, bool
 	okIsActive = true;
 	isWide = false;
 
-	Point textRectSize(Width() - 20, 0);
+	Point textRectSize(Width() - HORIZONTAL_PADDING, 0);
 	text = std::make_shared<TextArea>();
 	text->SetAlignment(Alignment::JUSTIFIED);
 	text->SetRect(Rectangle(Point(), textRectSize));
@@ -336,6 +371,7 @@ void Dialog::Init(const string &message, Truncate truncate, bool canCancel, bool
 	text->SetText(message);
 	AddChild(text);
 
+	const Sprite *top = SpriteSet::Get("ui/dialog top");
 	// If the dialog is too tall, then switch to wide mode.
 	int maxHeight = Screen::Height() * 3 / 4;
 	if(text->GetTextHeight() > maxHeight)
@@ -343,46 +379,47 @@ void Dialog::Init(const string &message, Truncate truncate, bool canCancel, bool
 		textRectSize.Y() = maxHeight;
 		isWide = true;
 		// Re-wrap with the new width
-		textRectSize.X() = Width() - 20;
+		textRectSize.X() = Width() - HORIZONTAL_PADDING;
 		text->SetRect(Rectangle(Point{}, textRectSize));
 
-		if(text->GetLongestLineWidth() <= WIDTH)
+		if(text->GetLongestLineWidth() <= top->Width() - HORIZONTAL_MARGIN - HORIZONTAL_PADDING)
 		{
 			// Formatted text is long and skinny (e.g. scan result dialog). Go back
 			// to using the default width, since the wide width doesn't help.
 			isWide = false;
-			textRectSize.X() = Width() - 20;
+			textRectSize.X() = Width() - HORIZONTAL_PADDING;
 			text->SetRect(Rectangle(Point{}, textRectSize));
 		}
 	}
 	else
 		textRectSize.Y() = text->GetTextHeight();
 
-	// The dialog with no extenders is 80 pixels tall. 10 pixels at the top and
-	// bottom are "padding," but text.Height() over-reports the height by about
-	// 6 pixels because it includes its own padding at the bottom. If there is a
-	// text input, we need another 20 pixels for it and 10 pixels padding.
-	height = 10 + (textRectSize.Y() - 6) + 10 + 30 * (!isMission && (intFun || stringFun));
-	// Determine how many 40-pixel extension panels we need.
-	if(height <= 80)
-		height = 0;
+	top = SpriteSet::Get(isWide ? "ui/dialog top wide" : "ui/dialog top");
+	const Sprite *middle = SpriteSet::Get(isWide ? "ui/dialog middle wide" : "ui/dialog middle");
+	const Sprite *bottom = SpriteSet::Get(isWide ? "ui/dialog bottom wide" : "ui/dialog bottom");
+	const Sprite *cancel = SpriteSet::Get("ui/dialog cancel");
+	// The height of the bottom sprite without the included button's height.
+	const int realBottomHeight = bottom->Height() - cancel->Height();
+
+	int height = TOP_PADDING + textRectSize.Y() + BOTTOM_PADDING +
+			(realBottomHeight - BOTTOM_PADDING) * (!isMission && (intFun || stringFun));
+	// Determine how many extension panels we need.
+	if(height <= realBottomHeight + top->Height())
+		extensionCount = 0;
 	else
-		height = (height - 40) / 40;
+		extensionCount = (height - middle->Height()) / middle->Height();
 
 	// Now that we know how big we want to render the text, position the text
 	// area and add it to the UI.
-	const Sprite *top = SpriteSet::Get(isWide ? "ui/dialog top wide" : "ui/dialog top");
-	const Sprite *middle = SpriteSet::Get(isWide ? "ui/dialog middle wide" : "ui/dialog middle");
-	const Sprite *bottom = SpriteSet::Get(isWide ? "ui/dialog bottom wide" : "ui/dialog bottom");
 
 	// Get the position of the top of this dialog, and of the text and input.
-	Point pos(0., (top->Height() + height * middle->Height() + bottom->Height()) * -.5f);
-	Point textPos(Width() * -.5 + 10., pos.Y() + 20.);
+	Point pos(0., (top->Height() + extensionCount * middle->Height() + bottom->Height()) * -.5f);
+	Point textPos(Width() * -.5 + LEFT_PADDING, pos.Y() + VERTICAL_PADDING);
 	// Resize textRectSize to match the visual height of the dialog, which will
 	// be rounded up from the actual text height by the number of panels that
 	// were added. This helps correctly position the TextArea scroll buttons.
-	// The text height was over-reported by 6 pixels, so we add those pixels back for consistency.
-	textRectSize.Y() = 60 + height * 40 + 6 - 30 * (!isMission && (intFun || stringFun));
+	textRectSize.Y() = (top->Height() + realBottomHeight - VERTICAL_PADDING) + extensionCount * middle->Height() -
+			(realBottomHeight - BOTTOM_PADDING) * (!isMission && (intFun || stringFun));
 
 	Rectangle textRect = Rectangle::FromCorner(textPos, textRectSize);
 	text->SetRect(textRect);
@@ -426,5 +463,6 @@ void Dialog::DoCallback(const bool isOk) const
 
 int Dialog::Width() const
 {
-	return isWide ? WIDE_WIDTH : WIDTH;
+	const Sprite *top = SpriteSet::Get(isWide ? "ui/dialog top wide" : "ui/dialog top");
+	return top->Width() - HORIZONTAL_MARGIN;
 }
diff --git a/source/Dialog.h b/source/Dialog.h
index 7fec7fd360b2..633012479d6a 100644
--- a/source/Dialog.h
+++ b/source/Dialog.h
@@ -102,12 +102,14 @@ class Dialog : public Panel {
 	// Common code from all three constructors:
 	void Init(const std::string &message, Truncate truncate, bool canCancel = true, bool isMission = false);
 	void DoCallback(bool isOk = true) const;
+	// The width of the dialog, excluding margins.
 	int Width() const;
 
 
 protected:
 	std::shared_ptr<TextArea> text;
-	int height;
+	// The number of extra segments in this dialog.
+	int extensionCount;
 
 	std::function<void(int)> intFun;
 	std::function<void(const std::string &)> stringFun;
diff --git a/source/text/WrappedText.cpp b/source/text/WrappedText.cpp
index c5e9e8a53211..3622d8f877ae 100644
--- a/source/text/WrappedText.cpp
+++ b/source/text/WrappedText.cpp
@@ -313,7 +313,9 @@ void WrappedText::Wrap()
 	// Adjust the spacing of words in the final line of text.
 	AdjustLine(lineBegin, lineWidth, true);
 
-	height = word.y;
+	// We have over-calculated the actual height by an extra paragraph break,
+	// so subtract that.
+	height = max(0, word.y - paragraphBreak);
 }