From 3482c835353533fc4d5d2cccb7261b4e70f2197c Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Mon, 20 Jan 2025 02:37:45 +0100 Subject: [PATCH] Fix logic in wxTextWrapper::Wrap() The existing code behaved completely wrongly whenever a word longer than the maximal width was present as it just gave up and didn't wrap anything at all after it -- when it should have just let this word overflow but still wrap the rest. Fix this and add a unit test checking that this works correctly now (several of the tests would have failed before). See #23339, #25100. (cherry picked from commit 597273020037ed853e0864d5bc2c1478a3a51286) --- docs/changes.txt | 1 + src/common/stattextcmn.cpp | 22 +++-- tests/Makefile.in | 4 + tests/makefile.gcc | 4 + tests/makefile.vc | 4 + tests/misc/textwrap.cpp | 148 +++++++++++++++++++++++++++++++++ tests/test.bkl | 1 + tests/test_gui.vcxproj | 1 + tests/test_gui.vcxproj.filters | 3 + 9 files changed, 181 insertions(+), 7 deletions(-) create mode 100644 tests/misc/textwrap.cpp diff --git a/docs/changes.txt b/docs/changes.txt index cce43337df..126d75e374 100644 --- a/docs/changes.txt +++ b/docs/changes.txt @@ -272,6 +272,7 @@ All (GUI): - Improve wxPropGrid checkboxes in high DPI (Alex Shvartzkop, #24650, #24651). - Don't break AUI layout if Update() called when window is iconized (#24930). - Improve wxInfoBar appearance (#24838, #24902, #25048). +- Fix multiple bugs in wxStaticText::Wrap() (#23339). - Fix WX_GL_COMPAT_PROFILE spelling (#24964). wxGTK: diff --git a/src/common/stattextcmn.cpp b/src/common/stattextcmn.cpp index 74aa8aa9c2..8e23c6e0e2 100644 --- a/src/common/stattextcmn.cpp +++ b/src/common/stattextcmn.cpp @@ -142,19 +142,27 @@ void wxTextWrapper::Wrap(wxWindow *win, const wxString& text, int widthMax) } // Find the last word to chop off. - const size_t lastSpace = line.rfind(' ', posEnd); - if ( lastSpace == wxString::npos ) + size_t posSpace = line.rfind(' ', posEnd); + if ( posSpace == wxString::npos ) { - // No spaces, so can't wrap. - DoOutputLine(line); - break; + // No spaces, so can't wrap, output until the end of the word + // which is defined here as just a sequence of non-space chars. + // + // TODO: Implement real Unicode word break algorithm. + posSpace = line.find(' ', posEnd); + if ( posSpace == wxString::npos ) + { + // No more spaces at all, output the rest of the line. + DoOutputLine(line); + break; + } } // Output the part that fits. - DoOutputLine(line.substr(0, lastSpace)); + DoOutputLine(line.substr(0, posSpace)); // And redo the layout with the rest. - line = line.substr(lastSpace + 1); + line = line.substr(posSpace + 1); } } } diff --git a/tests/Makefile.in b/tests/Makefile.in index a6b9ea02ed..ae1f56772e 100644 --- a/tests/Makefile.in +++ b/tests/Makefile.in @@ -265,6 +265,7 @@ TEST_GUI_OBJECTS = \ test_gui_garbage.o \ test_gui_safearrayconverttest.o \ test_gui_settings.o \ + test_gui_textwrap.o \ test_gui_socket.o \ test_gui_tlw.o \ test_gui_dataview.o \ @@ -1175,6 +1176,9 @@ test_gui_safearrayconverttest.o: $(srcdir)/misc/safearrayconverttest.cpp $(TEST_ test_gui_settings.o: $(srcdir)/misc/settings.cpp $(TEST_GUI_ODEP) $(CXXC) -c -o $@ $(TEST_GUI_CXXFLAGS) $(srcdir)/misc/settings.cpp +test_gui_textwrap.o: $(srcdir)/misc/textwrap.cpp $(TEST_GUI_ODEP) + $(CXXC) -c -o $@ $(TEST_GUI_CXXFLAGS) $(srcdir)/misc/textwrap.cpp + test_gui_socket.o: $(srcdir)/net/socket.cpp $(TEST_GUI_ODEP) $(CXXC) -c -o $@ $(TEST_GUI_CXXFLAGS) $(srcdir)/net/socket.cpp diff --git a/tests/makefile.gcc b/tests/makefile.gcc index 6a0ee0cd7b..a7eae3594f 100644 --- a/tests/makefile.gcc +++ b/tests/makefile.gcc @@ -239,6 +239,7 @@ TEST_GUI_OBJECTS = \ $(OBJS)\test_gui_garbage.o \ $(OBJS)\test_gui_safearrayconverttest.o \ $(OBJS)\test_gui_settings.o \ + $(OBJS)\test_gui_textwrap.o \ $(OBJS)\test_gui_socket.o \ $(OBJS)\test_gui_tlw.o \ $(OBJS)\test_gui_dataview.o \ @@ -1134,6 +1135,9 @@ $(OBJS)\test_gui_safearrayconverttest.o: ./misc/safearrayconverttest.cpp $(OBJS)\test_gui_settings.o: ./misc/settings.cpp $(CXX) -c -o $@ $(TEST_GUI_CXXFLAGS) $(CPPDEPS) $< +$(OBJS)\test_gui_textwrap.o: ./misc/textwrap.cpp + $(CXX) -c -o $@ $(TEST_GUI_CXXFLAGS) $(CPPDEPS) $< + $(OBJS)\test_gui_socket.o: ./net/socket.cpp $(CXX) -c -o $@ $(TEST_GUI_CXXFLAGS) $(CPPDEPS) $< diff --git a/tests/makefile.vc b/tests/makefile.vc index 43ea60cd9b..f75651dcfd 100644 --- a/tests/makefile.vc +++ b/tests/makefile.vc @@ -255,6 +255,7 @@ TEST_GUI_OBJECTS = \ $(OBJS)\test_gui_garbage.obj \ $(OBJS)\test_gui_safearrayconverttest.obj \ $(OBJS)\test_gui_settings.obj \ + $(OBJS)\test_gui_textwrap.obj \ $(OBJS)\test_gui_socket.obj \ $(OBJS)\test_gui_tlw.obj \ $(OBJS)\test_gui_dataview.obj \ @@ -1432,6 +1433,9 @@ $(OBJS)\test_gui_safearrayconverttest.obj: .\misc\safearrayconverttest.cpp $(OBJS)\test_gui_settings.obj: .\misc\settings.cpp $(CXX) /c /nologo /TP /Fo$@ $(TEST_GUI_CXXFLAGS) .\misc\settings.cpp +$(OBJS)\test_gui_textwrap.obj: .\misc\textwrap.cpp + $(CXX) /c /nologo /TP /Fo$@ $(TEST_GUI_CXXFLAGS) .\misc\textwrap.cpp + $(OBJS)\test_gui_socket.obj: .\net\socket.cpp $(CXX) /c /nologo /TP /Fo$@ $(TEST_GUI_CXXFLAGS) .\net\socket.cpp diff --git a/tests/misc/textwrap.cpp b/tests/misc/textwrap.cpp new file mode 100644 index 0000000000..3369b76b3d --- /dev/null +++ b/tests/misc/textwrap.cpp @@ -0,0 +1,148 @@ +/////////////////////////////////////////////////////////////////////////////// +// Name: tests/misc/textwrap.cpp +// Purpose: wxTextWrapper unit test +// Author: Vadim Zeitlin +// Created: 2025-01-19 +// Copyright: (c) 2025 Vadim Zeitlin +/////////////////////////////////////////////////////////////////////////////// + +// ---------------------------------------------------------------------------- +// headers +// ---------------------------------------------------------------------------- + +#include "testprec.h" + +#ifndef WX_PRECOMP + #include "wx/app.h" +#endif // !PCH + +#include "wx/textwrapper.h" + +// ---------------------------------------------------------------------------- +// wrapper implementation for testing +// ---------------------------------------------------------------------------- + +namespace +{ + +class HardBreakWrapper : public wxTextWrapper +{ +public: + HardBreakWrapper() = default; + + // Return the window used for wrapping: for now, use the main frame because + // it doesn't really matter which one we use. + wxWindow* GetWindow() const + { + return wxTheApp->GetTopWindow(); + } + + // Helper function returning the width of the given text. + int GetExtent(const wxString& text) const + { + return GetWindow()->GetTextExtent(text).x; + } + + // Wrap and return the number of lines in the wrapped text. + size_t Do(const wxString& text, int width) + { + Wrap(GetWindow(), text, width); + + return m_lines.size(); + } + + const wxString& GetLine(size_t n) const + { + REQUIRE( n < m_lines.size() ); + + return m_lines[n]; + } + + wxString GetResult() const { return wxJoin(m_lines, '\n'); } + +protected: + void OnOutputLine(const wxString& line) override { m_lines.push_back(line); } + +private: + wxArrayString m_lines; +}; + +} // anonymous namespace + +// ---------------------------------------------------------------------------- +// the tests +// ---------------------------------------------------------------------------- + +TEST_CASE("wxTextWrapper::Wrap", "[text]") +{ + // Note that this text shouldn't contain line breaks. + const wxString text = + "Lorem ipsum dolor sit amet, consectetur adipiscing elit, " + "sed do eiusmod tempor incididunt ut labore et dolore magna aliqua." + ; + + HardBreakWrapper w; + + // Check that not wrapping the text works. + SECTION("None") + { + REQUIRE( w.Do(text, -1) == 1 ); + CHECK( w.GetLine(0) == text ); + } + + SECTION("Wide") + { + REQUIRE( w.Do(text, 200*w.GetExtent("x")) == 1 ); + CHECK( w.GetLine(0) == text ); + } + + // Check that wrapping the text using reasonable width works. + SECTION("Normal") + { + const auto n = w.Do(text, 40*w.GetExtent("x")); + INFO("Wrapped text:\n" << w.GetResult() << "\n"); + CHECK( n >= 3 ); + } + + SECTION("Narrow") + { + const auto n = w.Do(text, 20*w.GetExtent("x")); + INFO("Wrapped text:\n" << w.GetResult() << "\n"); + CHECK( n >= 7 ); + } + + // Make the window too narrow to fit the word "consectetur" and check that + // the text is still wrapped reasonableness well. + SECTION("Thin") + { + const auto n = w.Do(text, w.GetExtent("Lorum")); + INFO("Wrapped text:\n" << w.GetResult() << "\n"); + CHECK( n > 10 ); + CHECK( w.GetLine(0) == "Lorem" ); + + // There should be just one line with the word "consectetur", longer + // than the wrap width. + bool found = false; + for ( size_t i = 1; i < n; ++i ) + { + if ( w.GetLine(i) == "consectetur" ) + { + found = true; + break; + } + } + + CHECK( found ); + } + + // Make the window too narrow to fit even a single character so that + // wrapped text has one word per line. + SECTION("Narrowest") + { + const auto n = w.Do(text, 1); + INFO("Wrapped text:\n" << w.GetResult() << "\n"); + REQUIRE( n == static_cast(text.Freq(' ')) + 1 ); + CHECK( w.GetLine(0) == "Lorem" ); + CHECK( w.GetLine(5) == "consectetur" ); + } +} diff --git a/tests/test.bkl b/tests/test.bkl index 2805f66c5f..b87d057599 100644 --- a/tests/test.bkl +++ b/tests/test.bkl @@ -269,6 +269,7 @@ misc/garbage.cpp misc/safearrayconverttest.cpp misc/settings.cpp + misc/textwrap.cpp