From f4128208deaeb91427945ea6dd8b00b1eb10ea17 Mon Sep 17 00:00:00 2001 From: wraitii Date: Thu, 2 Jun 2022 12:56:53 +0000 Subject: [PATCH] Fix text alignment handling of spaces around wrapping. Follows f8d2927748. There is an issue with text-wrapping and word separators (aka spaces). Because 0 A.D. collates the space after a word to the same TextCall, we occasionally need to ignore it when considering line-wrapping, because we don't want empty spaces on the right-side of right-aligned text. However, the logic to handle this is currently broken and inconsistent. The method introduced here uses the SFeedback structure to properly report it and generalises the checks. Note that multiples spaces are not collapsed in 0 A.D., and for consistency the word-separator-collapsing behaviour is ignored. Comments by: phosit, vlasdislavbelov Fixes #6551 Differential Revision: https://code.wildfiregames.com/D4662 This was SVN commit r26915. --- .../data/mods/_test.minimal/fonts/console.fnt | 6 +- source/gui/CGUIText.cpp | 57 ++-- source/gui/SettingTypes/CGUIString.cpp | 11 + source/gui/SettingTypes/CGUIString.h | 7 +- source/gui/tests/test_CGUIText.h | 254 ++++++++++++++++++ 5 files changed, 315 insertions(+), 20 deletions(-) create mode 100644 source/gui/tests/test_CGUIText.h diff --git a/binaries/data/mods/_test.minimal/fonts/console.fnt b/binaries/data/mods/_test.minimal/fonts/console.fnt index 1e94a12367..45c09e78e9 100644 --- a/binaries/data/mods/_test.minimal/fonts/console.fnt +++ b/binaries/data/mods/_test.minimal/fonts/console.fnt @@ -1,7 +1,9 @@ -100 +101 256 256 +a 606 -20 +15 +12 32 0 256 0 0 0 0 5 33 250 154 3 11 1 11 4 34 121 18 5 4 0 11 5 diff --git a/source/gui/CGUIText.cpp b/source/gui/CGUIText.cpp index 5438ead317..aa07a58b0d 100644 --- a/source/gui/CGUIText.cpp +++ b/source/gui/CGUIText.cpp @@ -79,7 +79,14 @@ CGUIText::CGUIText(const CGUI& pGUI, const CGUIString& string, const CStrW& font int posLastImage = -1; // Position in the string where last img (either left or right) were encountered. // in order to avoid duplicate processing. - // Go through string word by word + // The calculated width of each word includes the space between the current + // word and the next. When we're wrapping, we need subtract the width of the + // space after the last word on the line before the wrap. + CFontMetrics currentFont(font); + float spaceWidth = currentFont.GetCharacterWidth(L' '); + + // Go through string word by word. + // a word is defined as [start, end[ in string.m_Words so we skip the last item. for (int i = 0; i < static_cast(string.m_Words.size()) - 1; ++i) { // Pre-process each line one time, so we know which floating images @@ -102,8 +109,10 @@ CGUIText::CGUIText(const CGUI& pGUI, const CGUIString& string, const CStrW& font prelimLineHeight = std::max(prelimLineHeight, feedback.m_Size.Height); + float spaceCorrection = feedback.m_EndsWithSpace ? spaceWidth : 0.f; + // If width is 0, then there's no word-wrapping, disable NewLine. - if ((width != 0 && from != i && (lineWidth + 2 * bufferZone > width || feedback.m_NewLine)) || i == static_cast(string.m_Words.size()) - 2) + if ((width != 0 && from != i && (lineWidth - spaceCorrection + 2 * bufferZone > width || feedback.m_NewLine)) || i == static_cast(string.m_Words.size()) - 2) { if (ProcessLine(pGUI, string, font, pObject, images, align, prelimLineHeight, width, bufferZone, firstLine, y, i, from)) return; @@ -173,6 +182,14 @@ void CGUIText::ComputeLineSize( const int tempFrom, CSize2D& lineSize) const { + // The calculated width of each word includes the space between the current + // word and the next. When we're wrapping, we need subtract the width of the + // space after the last word on the line before the wrap. + CFontMetrics currentFont(font); + float spaceWidth = currentFont.GetCharacterWidth(L' '); + + float spaceCorrection = 0.f; + float x = widthRangeFrom; for (int j = tempFrom; j <= i; ++j) { @@ -187,15 +204,12 @@ void CGUIText::ComputeLineSize( // Append X value. x += feedback2.m_Size.Width; - if (width != 0 && x > widthRangeTo && j != tempFrom && !feedback2.m_NewLine) - { - // The calculated width of each word includes the space between the current - // word and the next. When we're wrapping, we need subtract the width of the - // space after the last word on the line before the wrap. - CFontMetrics currentFont(font); - lineSize.Width -= currentFont.GetCharacterWidth(*L" "); + if (width != 0 && x - spaceCorrection > widthRangeTo && j != tempFrom && !feedback2.m_NewLine) break; - } + + // Update after the line-break detection, because otherwise spaceCorrection above + // will refer to the wrapped word and not the last-word-before-the-line-break. + spaceCorrection = feedback2.m_EndsWithSpace ? spaceWidth : 0.f; // Let lineSize.cy be the maximum m_Height we encounter. lineSize.Height = std::max(lineSize.Height, feedback2.m_Size.Height); @@ -209,6 +223,8 @@ void CGUIText::ComputeLineSize( lineSize.Width += feedback2.m_Size.Width; } + // Remove the space if necessary. + lineSize.Width -= spaceCorrection; } bool CGUIText::ProcessLine( @@ -364,14 +380,16 @@ bool CGUIText::AssembleCalls( tc.m_pSpriteCall->m_Area += tc.m_Pos - CSize2D(0, tc.m_pSpriteCall->m_Area.GetHeight()); } - // Append X value. x += feedback2.m_Size.Width; - // The first word overrides the width limit, what we - // do, in those cases, are just drawing that word even - // though it'll extend the object. if (width != 0) // only if word-wrapping is applicable { + // Check if we need to wrap, using the same algorithm as ComputeLineSize + // This means we must ignore the 'space before the next word' for the purposes of wrapping. + CFontMetrics currentFont(font); + float spaceWidth = currentFont.GetCharacterWidth(L' '); + float spaceCorrection = feedback2.m_EndsWithSpace ? spaceWidth : 0.f; + if (feedback2.m_NewLine) { from = j + 1; @@ -392,12 +410,17 @@ bool CGUIText::AssembleCalls( } break; } - else if (x > widthRangeTo && j == tempFrom) + else if (x - spaceCorrection > widthRangeTo && j == tempFrom) { + // The first word overrides the width limit, what we do, + // in those cases, is just drawing that word even + // though it'll extend the object. + // Ergo: do not break, since we want it to be added to m_TextCalls. from = j+1; - // do not break, since we want it to be added to m_TextCalls + // To avoid doing redundant computations, set up j to exit the loop right away. + j = i + 1; } - else if (x > widthRangeTo) + else if (x - spaceCorrection > widthRangeTo) { from = j; break; diff --git a/source/gui/SettingTypes/CGUIString.cpp b/source/gui/SettingTypes/CGUIString.cpp index 539f23caa3..025e24817c 100644 --- a/source/gui/SettingTypes/CGUIString.cpp +++ b/source/gui/SettingTypes/CGUIString.cpp @@ -229,6 +229,17 @@ void CGUIString::GenerateTextCall(const CGUI& pGUI, SFeedback& Feedback, CStrInt if (!TextCall.m_String.empty() && TextCall.m_String[0] == '\n') Feedback.m_NewLine = true; + // Multiple empty spaces are treated as individual words (one per space), + // and for coherence we'll do the 'ignore space after the word' thing + // only if the word actually has some other text in it, so process this only if size >= 2 + else if (TextCall.m_String.size() >= 2) + { + const wchar_t lastChar = TextCall.m_String.back(); + // If the last word ends with a 'space', we'll ignore it when aligning, so mark it. + Feedback.m_EndsWithSpace = lastChar == ' ' || lastChar == 0x3000; + } + else + Feedback.m_EndsWithSpace = false; // Add text-chunk Feedback.m_TextCalls.emplace_back(std::move(TextCall)); diff --git a/source/gui/SettingTypes/CGUIString.h b/source/gui/SettingTypes/CGUIString.h index 662387b3e6..ccb4d3a6bb 100644 --- a/source/gui/SettingTypes/CGUIString.h +++ b/source/gui/SettingTypes/CGUIString.h @@ -1,4 +1,4 @@ -/* Copyright (C) 2021 Wildfire Games. +/* Copyright (C) 2022 Wildfire Games. * This file is part of 0 A.D. * * 0 A.D. is free software: you can redistribute it and/or modify @@ -156,6 +156,11 @@ public: * If the word inputted was a new line. */ bool m_NewLine; + + /** + * If the word inputted ends with a space that can be collapsed when aligning. + */ + bool m_EndsWithSpace; }; /** diff --git a/source/gui/tests/test_CGUIText.h b/source/gui/tests/test_CGUIText.h new file mode 100644 index 0000000000..2a847e9422 --- /dev/null +++ b/source/gui/tests/test_CGUIText.h @@ -0,0 +1,254 @@ +/* Copyright (C) 2022 Wildfire Games. + * This file is part of 0 A.D. + * + * 0 A.D. is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 2 of the License, or + * (at your option) any later version. + * + * 0 A.D. is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with 0 A.D. If not, see . + */ + +#include "lib/self_test.h" + +#include "graphics/FontManager.h" +#include "gui/CGUI.h" +#include "gui/CGUIText.h" +#include "gui/SettingTypes/CGUIString.h" +#include "ps/CLogger.h" +#include "ps/ConfigDB.h" +#include "ps/ProfileViewer.h" +#include "ps/VideoMode.h" +#include "renderer/Renderer.h" + +class TestCGUIText : public CxxTest::TestSuite +{ + CProfileViewer* m_Viewer = nullptr; + CRenderer* m_Renderer = nullptr; +public: + void setUp() + { + g_VFS = CreateVfs(); + TS_ASSERT_OK(g_VFS->Mount(L"", DataDir() / "mods" / "_test.minimal" / "", VFS_MOUNT_MUST_EXIST)); + TS_ASSERT_OK(g_VFS->Mount(L"cache", DataDir() / "_testcache" / "", 0, VFS_MAX_PRIORITY)); + + CXeromyces::Startup(); + + // The renderer spews messages. + TestLogger logger; + + // We need to initialise the renderer to initialise the font manager. + // TODO: decouple this. + CConfigDB::Initialise(); + CConfigDB::Instance()->SetValueString(CFG_SYSTEM, "rendererbackend", "dummy"); + g_VideoMode.InitNonSDL(); + g_VideoMode.CreateBackendDevice(false); + m_Viewer = new CProfileViewer; + m_Renderer = new CRenderer; + } + + void tearDown() + { + delete m_Renderer; + delete m_Viewer; + g_VideoMode.Shutdown(); + CConfigDB::Shutdown(); + CXeromyces::Terminate(); + g_VFS.reset(); + DeleteDirectory(DataDir()/"_testcache"); + } + + void test_empty() + { + CGUI gui(g_ScriptContext); + CGUIText empty; + } + + void test_wrapping() + { + CGUI gui(g_ScriptContext); + + static CStrW font = L"console"; + // Make sure this matches the value of the file. + // TODO: load dynamically. + static const float lineHeight = 12.f; + static const float lineSpacing = 15.f; + + CGUIString string; + CGUIText text; + float width = 0.f; + float renderedWidth = 0.f; + float padding = 0.f; + EAlign align = EAlign::LEFT; + + // Thing to note: the space before the newline should collapse in right-alignment. + string.SetValue(L"Some long text that will wrap-around. \n New line."); + text = CGUIText(gui, string, font, width, padding, align, nullptr); + + // Width 0 means no wrapping, so we should be getting one render call & one line. + // TODO: is it wanted that \n doesn't wrap in that case? + // We have 11 calls: the 9 words (wrap-around is split in two), the space after the newline, and the newline itself. + TS_ASSERT_EQUALS(text.GetTextCalls().size(), 11); + TS_ASSERT_EQUALS(text.GetSize().Height, lineHeight); + + width = 100.f; + padding = 2.0f; + align = EAlign::LEFT; + text = CGUIText(gui, string, font, width, padding, align, nullptr); + renderedWidth = text.GetSize().Width; + // We have 10 calls: the 9 words (wrap-around is split in two), the space after the newline. + TS_ASSERT_EQUALS(text.GetTextCalls().size(), 10); + TS_ASSERT_LESS_THAN(text.GetSize().Width, width); + TS_ASSERT_EQUALS(text.GetSize().Height, padding * 2 + lineHeight + lineSpacing * 4); + + align = EAlign::RIGHT; + text = CGUIText(gui, string, font, width, padding, align, nullptr); + TS_ASSERT_EQUALS(text.GetTextCalls().size(), 10); + TS_ASSERT_EQUALS(text.GetSize().Width, renderedWidth); // Should be the same width as the left-case. + TS_ASSERT_EQUALS(text.GetSize().Height, padding * 2 + lineHeight + lineSpacing * 4); + + width = 400.f; + padding = 3.0f; + align = EAlign::LEFT; + text = CGUIText(gui, string, font, width, padding, align, nullptr); + TS_ASSERT_EQUALS(text.GetTextCalls().size(), 10); + TS_ASSERT_LESS_THAN(text.GetSize().Width, width); + TS_ASSERT_EQUALS(text.GetSize().Height, padding * 2 + lineHeight + lineSpacing); + + width = 400.f; + padding = 5.0f; + align = EAlign::CENTER; + text = CGUIText(gui, string, font, width, padding, align, nullptr); + renderedWidth = text.GetSize().Width; + TS_ASSERT_LESS_THAN(text.GetSize().Width, width); + TS_ASSERT_EQUALS(text.GetSize().Height, padding * 2 + lineHeight + lineSpacing); + + align = EAlign::RIGHT; + text = CGUIText(gui, string, font, width, padding, align, nullptr); + TS_ASSERT_EQUALS(text.GetSize().Width, renderedWidth); // Should be the same width as the center-case. + TS_ASSERT_EQUALS(text.GetSize().Height, padding * 2 + lineHeight + lineSpacing); + + align = EAlign::LEFT; + text = CGUIText(gui, string, font, width, padding, align, nullptr); + TS_ASSERT_EQUALS(text.GetSize().Width, renderedWidth); // Should be the same width as the center-case. + TS_ASSERT_EQUALS(text.GetSize().Height, padding * 2 + lineHeight + lineSpacing); + + width = 400.f; + padding = 100.0f; + align = EAlign::LEFT; + text = CGUIText(gui, string, font, width, padding, align, nullptr); + renderedWidth = text.GetSize().Width; + TS_ASSERT_LESS_THAN(text.GetSize().Width, width); + TS_ASSERT_EQUALS(text.GetSize().Height, padding * 2 + lineHeight + lineSpacing * 2); + + align = EAlign::RIGHT; + text = CGUIText(gui, string, font, width, padding, align, nullptr); + TS_ASSERT_EQUALS(text.GetSize().Width, renderedWidth); // Should be the same width as the left-case. + TS_ASSERT_EQUALS(text.GetSize().Height, padding * 2 + lineHeight + lineSpacing * 2); + } + + void test_overflow() + { + CGUI gui(g_ScriptContext); + + static CStrW font = L"console"; + // Make sure this matches the value of the file. + // TODO: load dynamically. + static const float lineHeight = 12.f; + static const float lineSpacing = 15.f; + + float renderedWidth = 0.f; + const float width = 200.f; + const float padding = 20.f; + + CGUIString string; + CGUIText text; + string.SetValue(L"wordthatisverylonganddefinitelywontfitinaline and other words"); + text = CGUIText(gui, string, font, width, padding, EAlign::LEFT, nullptr); + renderedWidth = text.GetSize().Width; + TS_ASSERT_EQUALS(text.GetSize().Height, lineHeight + lineSpacing * 1 + padding * 2); + text = CGUIText(gui, string, font, width, padding, EAlign::CENTER, nullptr); + TS_ASSERT_EQUALS(renderedWidth, text.GetSize().Width); + TS_ASSERT_EQUALS(text.GetSize().Height, lineHeight + lineSpacing * 1 + padding * 2); + text = CGUIText(gui, string, font, width, padding, EAlign::RIGHT, nullptr); + TS_ASSERT_EQUALS(renderedWidth, text.GetSize().Width); + TS_ASSERT_EQUALS(text.GetSize().Height, lineHeight + lineSpacing * 1 + padding * 2); + + string.SetValue(L"other words and wordthatisverylonganddefinitelywontfitinaline"); + text = CGUIText(gui, string, font, width, padding, EAlign::LEFT, nullptr); + renderedWidth = text.GetSize().Width; + TS_ASSERT_EQUALS(text.GetSize().Height, lineHeight + lineSpacing * 1 + padding * 2); + text = CGUIText(gui, string, font, width, padding, EAlign::CENTER, nullptr); + TS_ASSERT_EQUALS(renderedWidth, text.GetSize().Width); + TS_ASSERT_EQUALS(text.GetSize().Height, lineHeight + lineSpacing * 1 + padding * 2); + text = CGUIText(gui, string, font, width, padding, EAlign::RIGHT, nullptr); + TS_ASSERT_EQUALS(renderedWidth, text.GetSize().Width); + TS_ASSERT_EQUALS(text.GetSize().Height, lineHeight + lineSpacing * 1 + padding * 2); + + string.SetValue(L"wordthatisverylonganddefinitelywontfitinaline"); + text = CGUIText(gui, string, font, width, padding, EAlign::LEFT, nullptr); + renderedWidth = text.GetSize().Width; + TS_ASSERT_EQUALS(text.GetSize().Height, lineHeight + padding * 2); + text = CGUIText(gui, string, font, width, padding, EAlign::CENTER, nullptr); + TS_ASSERT_EQUALS(renderedWidth, text.GetSize().Width); + TS_ASSERT_EQUALS(text.GetSize().Height, lineHeight + padding * 2); + text = CGUIText(gui, string, font, width, padding, EAlign::RIGHT, nullptr); + TS_ASSERT_EQUALS(renderedWidth, text.GetSize().Width); + TS_ASSERT_EQUALS(text.GetSize().Height, lineHeight + padding * 2); + } + + void test_regression_rP26522() + { + TS_ASSERT_OK(g_VFS->Mount(L"", DataDir() / "mods" / "mod" / "", VFS_MOUNT_MUST_EXIST)); + + CGUI gui(g_ScriptContext); + + static CStrW font = L"sans-bold-13"; + CGUIString string; + CGUIText text; + + // rP26522 introduced a bug that triggered in rare cases with word-wrapping. + string.SetValue(L"90–120 min"); + text = CGUIText(gui, string, L"sans-bold-13", 53, 8.f, EAlign::LEFT, nullptr); + + TS_ASSERT_EQUALS(text.GetTextCalls().size(), 2); + TS_ASSERT_EQUALS(text.GetSize().Height, 14 + 9 + 8 * 2); + } + + void test_multiple_blank_spaces() + { + CGUI gui(g_ScriptContext); + + static CStrW font = L"console"; + // Make sure this matches the value of the file. + // TODO: load dynamically. + static const float lineHeight = 12.f; + static const float lineSpacing = 15.f; + + CGUIString string; + CGUIText text; + float width = 100.f; + float renderedWidth = 0.f; + float padding = 0.f; + EAlign align = EAlign::LEFT; + + string.SetValue(L" word another \n spaces \n \n word "); + text = CGUIText(gui, string, font, width, padding, align, nullptr); + + // Blank spaces are treated as a word. + TS_ASSERT_EQUALS(text.GetTextCalls().size(), 26); + TS_ASSERT_EQUALS(text.GetSize().Height, lineHeight + lineSpacing * 4); + TS_ASSERT_EQUALS(text.GetSize().Width, 89.f); + renderedWidth = text.GetSize().Width; + + align = EAlign::RIGHT; + text = CGUIText(gui, string, font, width, padding, align, nullptr); + TS_ASSERT_EQUALS(text.GetSize().Width, renderedWidth); + } +};