Fix function wxControlBase::DoEllipsizeSingleLine to really make sure that the ellipsized string takes less pixels than maxFinalWidthPx.

Add comments to explain in more details what the function does and in particular the valid ranges of all internal variables; fix in that regard both the code of both wxELLIPSIZE_START, wxELLIPSIZE_MIDDLE and wxELLIPSIZE_END.
Add more asserts to check the valid ranges and turn a couple of time-expensive checks in level-2 asserts.
Add a test unit for the wxControl::Ellipsize function.
Fix minor details in the docs of wxControl::Ellipsize.
Closes #11567.


git-svn-id: https://svn.wxwidgets.org/svn/wx/wxWidgets/trunk@63660 c3d73ce0-8a6f-49c7-b76d-6d57e0e08775

interface/wx/control.h

@@ -96,6 +96,10 @@ public:
     /**
         Replaces parts of the @a label string with ellipsis, if needed, so
         that it doesn't exceed @a maxWidth.
+        
+        Note that this functions is guaranteed to always returns a string
+        whose rendering on the given DC takes less than @a maxWidth pixels
+        in horizontal.
 
         @param label
             The string to ellipsize
@@ -103,11 +107,15 @@ public:
             The DC used to retrieve the character widths through the
             wxDC::GetPartialTextExtents() function.
         @param mode
-            The ellipsization modes. See ::wxEllipsizeMode.
+            The ellipsization mode. This is the setting which determines
+            which part of the string should be replaced by the ellipsis.
+            See ::wxEllipsizeMode enumeration values for more info.
         @param maxWidth
             The maximum width of the returned string in pixels.
+            This argument determines how much characters of the string need to
+            be removed (and replaced by ellipsis).
         @param flags
-            One or more of the ::wxEllipsize
+            One or more of the ::wxEllipsizeFlags enumeration values combined.
     */
     static wxString Ellipsize(const wxString& label, const wxDC& dc,
                               wxEllipsizeMode mode, int maxWidth,

src/common/ctrlcmn.cpp

@@ -242,8 +242,8 @@ wxString wxControlBase::DoEllipsizeSingleLine(const wxString& curLine, const wxD
 {
     wxASSERT_MSG(replacementWidthPx > 0 && marginWidthPx > 0,
                  "Invalid parameters");
-    wxASSERT_MSG(!curLine.Contains('\n'),
+    wxASSERT_LEVEL_2_MSG(!curLine.Contains('\n'),
-                 "Use Ellipsize() instead!");
+                         "Use Ellipsize() instead!");
 
     wxASSERT_MSG( mode != wxELLIPSIZE_NONE, "shouldn't be called at all then" );
 
@@ -261,92 +261,107 @@ wxString wxControlBase::DoEllipsizeSingleLine(const wxString& curLine, const wxD
 
     wxASSERT(charOffsetsPx.GetCount() == len);
 
+    // NOTE: charOffsetsPx[n] is the width in pixels of the first n characters (with the last one INCLUDED)
+    //       thus charOffsetsPx[len-1] is the total width of the string
     size_t totalWidthPx = charOffsetsPx.Last();
     if ( totalWidthPx <= (size_t)maxFinalWidthPx )
         return curLine;     // we don't need to do any ellipsization!
 
-    int excessPx = totalWidthPx - maxFinalWidthPx +
+    int excessPx = wxMin(totalWidthPx - maxFinalWidthPx +
-                   replacementWidthPx +
+                         replacementWidthPx +
-                   marginWidthPx;     // security margin (NEEDED!)
+                         marginWidthPx,     // security margin
-    wxASSERT(excessPx>0);
+                         totalWidthPx);
+    wxASSERT(excessPx>0);       // excessPx should be in the [1;totalWidthPx] range
 
-    // remove characters in excess
+    // REMEMBER: indexes inside the string have a valid range of [0;len-1] if not otherwise constrained
-    size_t initialCharToRemove,     // index of first char to erase
+    //           lengths/counts of characters (e.g. nCharsToRemove) have a valid range of [0;len] if not otherwise constrained
-           nCharsToRemove;          // how many chars do we need to erase?
+    // NOTE: since this point we know we have for sure a non-empty string from which we need
+    //       to remove _at least_ one character (thus nCharsToRemove below is constrained to be >= 1)
 
+    size_t initialCharToRemove,     // index of first character to erase, valid range is [0;len-1]
+           nCharsToRemove;          // how many chars do we need to erase? valid range is [1;len-initialCharToRemove]
+
+    // let's compute the range of characters to remove depending on the ellipsization mode:
     switch (mode)
     {
         case wxELLIPSIZE_START:
             initialCharToRemove = 0;
-            for ( nCharsToRemove=0;
+            for ( nCharsToRemove = 1;
-                  nCharsToRemove < len && charOffsetsPx[nCharsToRemove] < excessPx;
+                  nCharsToRemove < len && charOffsetsPx[nCharsToRemove-1] < excessPx;
                   nCharsToRemove++ )
                 ;
             break;
 
         case wxELLIPSIZE_MIDDLE:
             {
-                // the start & end of the removed span of chars
+                // NOTE: the following piece of code works also when len == 1
+
+                // start the removal process from the middle of the string
+                // i.e. separe the string in three parts: 
+                // - the first one to preserve, valid range [0;initialCharToRemove-1] or the empty range if initialCharToRemove==0
+                // - the second one to remove, valid range [initialCharToRemove;endCharToRemove]
+                // - the third one to preserve, valid range [endCharToRemove+1;len-1] or the empty range if endCharToRemove==len-1
+                // NOTE: empty range != range [0;0] since the range [0;0] contains 1 character (the zero-th one)!
                 initialCharToRemove = len/2;
-                size_t endChar = len/2;
+                size_t endCharToRemove = len/2;     // index of the last character to remove; valid range is [0;len-1]
 
                 int removedPx = 0;
                 for ( ; removedPx < excessPx; )
                 {
+                    // try to remove the last character of the first part of the string
                     if (initialCharToRemove > 0)
                     {
-                        // widthPx of the initialCharToRemove-th character
+                        // width of the (initialCharToRemove-1)-th character
-                        int widthPx = charOffsetsPx[initialCharToRemove] -
+                        int widthPx;
-                                      charOffsetsPx[initialCharToRemove-1];
+                        if (initialCharToRemove >= 2)
+                            widthPx = charOffsetsPx[initialCharToRemove-1] - charOffsetsPx[initialCharToRemove-2];
+                        else
+                            widthPx = charOffsetsPx[initialCharToRemove-1];     
+                                // the (initialCharToRemove-1)-th character is the first char of the string
+                        
+                        wxASSERT(widthPx >= 0);     // widthPx is zero for e.g. tab characters
 
-                        // remove the initialCharToRemove-th character
+                        // mark the (initialCharToRemove-1)-th character as removable
-                        removedPx += widthPx;
                         initialCharToRemove--;
+                        removedPx += widthPx;
                     }
 
-                    if (endChar < len - 1 &&
+                    // try to remove the first character of the last part of the string
+                    if (endCharToRemove < len - 1 &&
                         removedPx < excessPx)
                     {
-                        // widthPx of the (endChar+1)-th character
+                        // width of the (endCharToRemove+1)-th character
-                        int widthPx = charOffsetsPx[endChar+1] -
+                        int widthPx = charOffsetsPx[endCharToRemove+1] -
-                                      charOffsetsPx[endChar];
+                                      charOffsetsPx[endCharToRemove];
 
-                        // remove the endChar-th character
+                        wxASSERT(widthPx >= 0);     // widthPx is zero for e.g. tab characters
+
+                        // mark the (endCharToRemove+1)-th character as removable
+                        endCharToRemove++;
                         removedPx += widthPx;
-                        endChar++;
                     }
 
-                    if (initialCharToRemove == 0 && endChar == len-1)
+                    if (initialCharToRemove == 0 && endCharToRemove == len-1)
                     {
-                        nCharsToRemove = len+1;
+                        // we need to remove all the characters of the string!
                         break;
                     }
                 }
 
-                initialCharToRemove++;
+                nCharsToRemove = endCharToRemove - initialCharToRemove + 1;
-                nCharsToRemove = endChar - initialCharToRemove + 1;
             }
             break;
 
         case wxELLIPSIZE_END:
             {
-                wxASSERT(len > 0);
-
                 int maxWidthPx = totalWidthPx - excessPx;
-                for ( initialCharToRemove = 0;
-                      initialCharToRemove < len && charOffsetsPx[initialCharToRemove] < maxWidthPx;
-                      initialCharToRemove++ )
-                    ;
 
-                if (initialCharToRemove == 0)
+                // go backward from the end of the string toward the start
-                {
+                for ( initialCharToRemove = len-1;
-                    nCharsToRemove = len;
+                      initialCharToRemove > 0 && charOffsetsPx[initialCharToRemove-1] > maxWidthPx;
-                }
+                      initialCharToRemove-- )
-                else
+                    ;
-                {
+                nCharsToRemove = len - initialCharToRemove;
-                    //initialCharToRemove--;      // go back one character
-                    nCharsToRemove = len - initialCharToRemove;
-                }
             }
             break;
 
@@ -356,25 +371,31 @@ wxString wxControlBase::DoEllipsizeSingleLine(const wxString& curLine, const wxD
             return curLine;
     }
 
-    wxString ret(curLine);
+    wxASSERT(initialCharToRemove >= 0 && initialCharToRemove <= len-1);  // see valid range for initialCharToRemove above
-    if (nCharsToRemove >= len)
+    wxASSERT(nCharsToRemove >= 1 && nCharsToRemove <= len-initialCharToRemove);  // see valid range for nCharsToRemove above
-    {
-        // need to remove the entire row!
-        ret.clear();
-    }
-    else
-    {
-        // erase nCharsToRemove characters after initialCharToRemove (included):
-        ret.erase(initialCharToRemove, nCharsToRemove+1);
 
-        // if there is space for the replacement dots, add them
+    // erase nCharsToRemove characters after initialCharToRemove (included);
-        if (maxFinalWidthPx > replacementWidthPx)
+    // e.g. if we have the string "foobar" (len = 6)
-            ret.insert(initialCharToRemove, wxELLIPSE_REPLACEMENT);
+    //                               ^
-    }
+    //                               \--- initialCharToRemove = 2
+    //      and nCharsToRemove = 2, then we get "foar"
+    wxString ret(curLine);
+    ret.erase(initialCharToRemove, nCharsToRemove);
+
+    int removedPx;
+    if (initialCharToRemove >= 1)
+        removedPx = charOffsetsPx[initialCharToRemove+nCharsToRemove-1] - charOffsetsPx[initialCharToRemove-1];
+    else 
+        removedPx = charOffsetsPx[initialCharToRemove+nCharsToRemove-1];
+    wxASSERT(removedPx >= excessPx);
+
+    // if there is space for the replacement dots, add them
+    if ((int)totalWidthPx-removedPx+replacementWidthPx < maxFinalWidthPx)
+        ret.insert(initialCharToRemove, wxELLIPSE_REPLACEMENT);
 
     // if everything was ok, we should have shortened this line
     // enough to make it fit in maxFinalWidthPx:
-    wxASSERT(dc.GetTextExtent(ret).GetWidth() < maxFinalWidthPx);
+    wxASSERT_LEVEL_2(dc.GetTextExtent(ret).GetWidth() <= maxFinalWidthPx);
 
     return ret;
 }

tests/Makefile.in

@@ -136,6 +136,7 @@ TEST_GUI_OBJECTS =  \
 	test_gui_size.o \
 	test_gui_point.o \
 	test_gui_colour.o \
+	test_gui_ellipsization.o \
 	test_gui_measuring.o \
 	test_gui_config.o \
 	test_gui_comboboxtest.o \
@@ -573,6 +574,9 @@ test_gui_point.o: $(srcdir)/geometry/point.cpp $(TEST_GUI_ODEP)
 test_gui_colour.o: $(srcdir)/graphics/colour.cpp $(TEST_GUI_ODEP)
 	$(CXXC) -c -o $@ $(TEST_GUI_CXXFLAGS) $(srcdir)/graphics/colour.cpp
 
+test_gui_ellipsization.o: $(srcdir)/graphics/ellipsization.cpp $(TEST_GUI_ODEP)
+	$(CXXC) -c -o $@ $(TEST_GUI_CXXFLAGS) $(srcdir)/graphics/ellipsization.cpp
+
 test_gui_measuring.o: $(srcdir)/graphics/measuring.cpp $(TEST_GUI_ODEP)
 	$(CXXC) -c -o $@ $(TEST_GUI_CXXFLAGS) $(srcdir)/graphics/measuring.cpp
 

tests/graphics/ellipsization.cpp

@@ -0,0 +1,78 @@
+///////////////////////////////////////////////////////////////////////////////
+// Name:        tests/graphics/ellipsization.cpp
+// Purpose:     wxControlBase::*Ellipsize* unit test
+// Author:      Francesco Montorsi
+// Created:     2010-03-10
+// RCS-ID:      $Id$
+// Copyright:   (c) 2010 Francesco Montorsi
+///////////////////////////////////////////////////////////////////////////////
+
+// ----------------------------------------------------------------------------
+// headers
+// ----------------------------------------------------------------------------
+
+#include "testprec.h"
+
+#ifdef __BORLANDC__
+    #pragma hdrstop
+#endif
+
+#include "wx/control.h"
+#include "wx/dcmemory.h"
+
+// ----------------------------------------------------------------------------
+// test class
+// ----------------------------------------------------------------------------
+
+class EllipsizationTestCase : public CppUnit::TestCase
+{
+public:
+    EllipsizationTestCase() { }
+
+private:
+    CPPUNIT_TEST_SUITE( EllipsizationTestCase );
+        CPPUNIT_TEST( Ellipsize );
+    CPPUNIT_TEST_SUITE_END();
+
+    void Ellipsize();
+
+    DECLARE_NO_COPY_CLASS(EllipsizationTestCase)
+};
+
+// register in the unnamed registry so that these tests are run by default
+CPPUNIT_TEST_SUITE_REGISTRATION( EllipsizationTestCase );
+
+// also include in it's own registry so that these tests can be run alone
+CPPUNIT_TEST_SUITE_NAMED_REGISTRATION( EllipsizationTestCase, "EllipsizationTestCase" );
+
+void EllipsizationTestCase::Ellipsize()
+{
+    wxMemoryDC dc;
+
+    wxString stringsToTest[] = 
+    { 
+        "N", ".", "x", "foobar", wxS("\u03B1"), "Another test", "a very very very very very very very long string",
+        "\xCE\xB1\xCE\xB2\xCE\xB3\xCE\xB4\xCE\xB5\xCE\xB6\xCE\xB7\xCE\xB8\xCE\xB9", 
+            // alpha+beta+gamma+delta+epsilon+zeta+eta+theta+iota
+        "\t", "\t\t\t\t\t", "a\tstring\twith\ttabs",
+        "\n", "\n\n\n\n\n", "a\nstring\nwith\nnewlines",
+        "&", "&&&&&&&", "a&string&with&newlines",
+        "\t\n&", "a\t\n&string\t\n&with\t\n&many\t\n&chars"
+    };
+    int flagsToTest[] = { 0, wxELLIPSIZE_FLAGS_PROCESS_MNEMONICS, wxELLIPSIZE_FLAGS_EXPAND_TABS, 
+                          wxELLIPSIZE_FLAGS_PROCESS_MNEMONICS|wxELLIPSIZE_FLAGS_EXPAND_TABS };
+    wxEllipsizeMode modesToTest[] = { wxELLIPSIZE_START, wxELLIPSIZE_MIDDLE, wxELLIPSIZE_END };
+    int widthsToTest[] = { 0, 1, 2, 3, 10, 20, 100 };
+
+    for (unsigned int s=0; s<WXSIZEOF(stringsToTest); s++)
+        for (unsigned int f=0; f<WXSIZEOF(flagsToTest); f++)
+            for (unsigned int m=0; m<WXSIZEOF(modesToTest); m++)
+                for (unsigned int w=0; w<WXSIZEOF(widthsToTest); w++)
+                {
+                    wxString ret = wxControlBase::Ellipsize(stringsToTest[s], dc, modesToTest[m], 
+                                                            widthsToTest[w], flagsToTest[f]);
+
+                    CPPUNIT_ASSERT_MESSAGE((std::string)("invalid ellipsization for: " + stringsToTest[s]),
+                                           dc.GetMultiLineTextExtent(ret).GetWidth() <= widthsToTest[w]);
+                }
+}

tests/makefile.bcc

@@ -121,6 +121,7 @@ TEST_GUI_OBJECTS =  \
 	$(OBJS)\test_gui_size.obj \
 	$(OBJS)\test_gui_point.obj \
 	$(OBJS)\test_gui_colour.obj \
+	$(OBJS)\test_gui_ellipsization.obj \
 	$(OBJS)\test_gui_measuring.obj \
 	$(OBJS)\test_gui_config.obj \
 	$(OBJS)\test_gui_comboboxtest.obj \
@@ -627,6 +628,9 @@ $(OBJS)\test_gui_point.obj: .\geometry\point.cpp
 $(OBJS)\test_gui_colour.obj: .\graphics\colour.cpp
 	$(CXX) -q -c -P -o$@ $(TEST_GUI_CXXFLAGS) .\graphics\colour.cpp
 
+$(OBJS)\test_gui_ellipsization.obj: .\graphics\ellipsization.cpp
+	$(CXX) -q -c -P -o$@ $(TEST_GUI_CXXFLAGS) .\graphics\ellipsization.cpp
+
 $(OBJS)\test_gui_measuring.obj: .\graphics\measuring.cpp
 	$(CXX) -q -c -P -o$@ $(TEST_GUI_CXXFLAGS) .\graphics\measuring.cpp
 

tests/makefile.gcc

@@ -115,6 +115,7 @@ TEST_GUI_OBJECTS =  \
 	$(OBJS)\test_gui_size.o \
 	$(OBJS)\test_gui_point.o \
 	$(OBJS)\test_gui_colour.o \
+	$(OBJS)\test_gui_ellipsization.o \
 	$(OBJS)\test_gui_measuring.o \
 	$(OBJS)\test_gui_config.o \
 	$(OBJS)\test_gui_comboboxtest.o \
@@ -609,6 +610,9 @@ $(OBJS)\test_gui_point.o: ./geometry/point.cpp
 $(OBJS)\test_gui_colour.o: ./graphics/colour.cpp
 	$(CXX) -c -o $@ $(TEST_GUI_CXXFLAGS) $(CPPDEPS) $<
 
+$(OBJS)\test_gui_ellipsization.o: ./graphics/ellipsization.cpp
+	$(CXX) -c -o $@ $(TEST_GUI_CXXFLAGS) $(CPPDEPS) $<
+
 $(OBJS)\test_gui_measuring.o: ./graphics/measuring.cpp
 	$(CXX) -c -o $@ $(TEST_GUI_CXXFLAGS) $(CPPDEPS) $<
 

tests/makefile.vc

@@ -116,6 +116,7 @@ TEST_GUI_OBJECTS =  \
 	$(OBJS)\test_gui_size.obj \
 	$(OBJS)\test_gui_point.obj \
 	$(OBJS)\test_gui_colour.obj \
+	$(OBJS)\test_gui_ellipsization.obj \
 	$(OBJS)\test_gui_measuring.obj \
 	$(OBJS)\test_gui_config.obj \
 	$(OBJS)\test_gui_comboboxtest.obj \
@@ -753,6 +754,9 @@ $(OBJS)\test_gui_point.obj: .\geometry\point.cpp
 $(OBJS)\test_gui_colour.obj: .\graphics\colour.cpp
 	$(CXX) /c /nologo /TP /Fo$@ $(TEST_GUI_CXXFLAGS) .\graphics\colour.cpp
 
+$(OBJS)\test_gui_ellipsization.obj: .\graphics\ellipsization.cpp
+	$(CXX) /c /nologo /TP /Fo$@ $(TEST_GUI_CXXFLAGS) .\graphics\ellipsization.cpp
+
 $(OBJS)\test_gui_measuring.obj: .\graphics\measuring.cpp
 	$(CXX) /c /nologo /TP /Fo$@ $(TEST_GUI_CXXFLAGS) .\graphics\measuring.cpp
 

tests/makefile.wat

@@ -359,6 +359,7 @@ TEST_GUI_OBJECTS =  &
 	$(OBJS)\test_gui_size.obj &
 	$(OBJS)\test_gui_point.obj &
 	$(OBJS)\test_gui_colour.obj &
+	$(OBJS)\test_gui_ellipsization.obj &
 	$(OBJS)\test_gui_measuring.obj &
 	$(OBJS)\test_gui_config.obj &
 	$(OBJS)\test_gui_comboboxtest.obj &
@@ -664,6 +665,9 @@ $(OBJS)\test_gui_point.obj :  .AUTODEPEND .\geometry\point.cpp
 $(OBJS)\test_gui_colour.obj :  .AUTODEPEND .\graphics\colour.cpp
 	$(CXX) -bt=nt -zq -fo=$^@ $(TEST_GUI_CXXFLAGS) $<
 
+$(OBJS)\test_gui_ellipsization.obj :  .AUTODEPEND .\graphics\ellipsization.cpp
+	$(CXX) -bt=nt -zq -fo=$^@ $(TEST_GUI_CXXFLAGS) $<
+
 $(OBJS)\test_gui_measuring.obj :  .AUTODEPEND .\graphics\measuring.cpp
 	$(CXX) -bt=nt -zq -fo=$^@ $(TEST_GUI_CXXFLAGS) $<
 

tests/test.bkl

@@ -117,6 +117,7 @@
             geometry/size.cpp
             geometry/point.cpp
             graphics/colour.cpp
+            graphics/ellipsization.cpp
             graphics/measuring.cpp
             config/config.cpp
             controls/comboboxtest.cpp

tests/test_test_gui.dsp

@@ -261,6 +261,10 @@ SOURCE=.\dummy.cpp
 # End Source File
 # Begin Source File
 
+SOURCE=.\graphics\ellipsization.cpp
+# End Source File
+# Begin Source File
+
 SOURCE=.\font\fonttest.cpp
 # End Source File
 # Begin Source File

tests/test_vc7_test_gui.vcproj

@@ -631,6 +631,9 @@
 						UsePrecompiledHeader="1"/>
 				</FileConfiguration>
 			</File>
+			<File
+				RelativePath=".\graphics\ellipsization.cpp">
+			</File>
 			<File
 				RelativePath=".\font\fonttest.cpp">
 			</File>

tests/test_vc8_test_gui.vcproj

@@ -915,6 +915,10 @@
 					/>
 				</FileConfiguration>
 			</File>
+			<File
+				RelativePath=".\graphics\ellipsization.cpp"
+				>
+			</File>
 			<File
 				RelativePath=".\font\fonttest.cpp"
 				>

tests/test_vc9_test_gui.vcproj

@@ -887,6 +887,10 @@
 					/>
 				</FileConfiguration>
 			</File>
+			<File
+				RelativePath=".\graphics\ellipsization.cpp"
+				>
+			</File>
 			<File
 				RelativePath=".\font\fonttest.cpp"
 				>