From 7155e82255f68ae9f117144f3c7dd9a91332b0d3 Mon Sep 17 00:00:00 2001 From: iwbnwif Date: Tue, 5 Nov 2019 20:18:11 +0000 Subject: [PATCH 1/5] Show the used font in case of failure in Ellipsization unit test --- tests/graphics/ellipsization.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/graphics/ellipsization.cpp b/tests/graphics/ellipsization.cpp index 52cf2f3add..73ad1a1b10 100644 --- a/tests/graphics/ellipsization.cpp +++ b/tests/graphics/ellipsization.cpp @@ -101,8 +101,9 @@ TEST_CASE_METHOD(EllipsizationTestCase, "Ellipsization::NormalCase", "[ellipsiza WX_ASSERT_MESSAGE ( ( - "Test #(%u,%u.%u): \"%s\" -> \"%s\"; width=%dpx > %dpx", + "Test #(%u,%u.%u): %s\n\"%s\" -> \"%s\"; width=%dpx > %dpx", s, f, m, + dc.GetFont().GetNativeFontInfoUserDesc(), str, ret, width, From 315c5a1d6d9d412981ee4eb7938674994f5c77ff Mon Sep 17 00:00:00 2001 From: iwbnwif Date: Wed, 6 Nov 2019 22:10:51 +0000 Subject: [PATCH 2/5] Fix ellipsization of strings containing mnemonics The old code didn't work correctly in the presence of TABs in the string (without wxELLIPSIZE_FLAGS_EXPAND_TABS being used), as their width is elastic and so simply subtracting the width of the ampersands didn't work in this case and could result in ellipsized string being longer than the maximum available width. Fix this by using a different approach and computing the widths of the actually shown string and then just inserting extra entries into the array of widths to match the invisible ampersands positions. --- src/common/ctrlcmn.cpp | 53 ++++++++++++++++++++++++++++-------------- 1 file changed, 35 insertions(+), 18 deletions(-) diff --git a/src/common/ctrlcmn.cpp b/src/common/ctrlcmn.cpp index 8aabe0bfa7..23d66e75db 100644 --- a/src/common/ctrlcmn.cpp +++ b/src/common/ctrlcmn.cpp @@ -293,33 +293,50 @@ struct EllipsizeCalculator m_maxFinalWidthPx(maxFinalWidthPx), m_replacementWidthPx(replacementWidthPx) { - m_isOk = dc.GetPartialTextExtents(s, m_charOffsetsPx); - wxASSERT( m_charOffsetsPx.GetCount() == s.length() ); - + // Where ampersands are used as mnemonic indicator they should not + // affect the overall width of the string and must be removed from the + // measurement. Nonetheless, we need to keep them in the string and + // have a corresponding entry in m_charOffsetsPx. if ( flags & wxELLIPSIZE_FLAGS_PROCESS_MNEMONICS ) { - // The ampersand itself shouldn't count for the width calculation - // as it won't be displayed: either it's an ampersand before some - // other character in which case it indicates a mnemonic, or it's - // an escaped ampersand, in which case only the second one of the - // pair of ampersands will be displayed. But we can't just remove - // the ampersand as we still need it in the final label, in order - // not to lose the mnemonics. Hence we use this hack, and pretend - // that ampersands simply have zero width. Of course, it could be - // not completely precise, but this is the best we can do without - // completely changing this code structure. + // Create a copy of the string with the ampersands removed to get + // the correct widths. + const wxString cpy = wxControl::RemoveMnemonics(s); + + m_isOk = dc.GetPartialTextExtents(cpy, m_charOffsetsPx); + + // Iterate through the original string inserting a cumulative width + // value for each ampersand that is the same as the following + // character's cumulative width value. Except this is only done + // for the first ampersand in a pair (see RemoveMnemonics). size_t n = 0; - int delta = 0; + bool lastWasMnemonic = false; for ( wxString::const_iterator it = s.begin(); it != s.end(); ++it, ++n ) { - if ( *it == '&' ) - delta += dc.GetTextExtent(wxS('&')).GetWidth(); - - m_charOffsetsPx[n] -= delta; + if ( *it == '&' && !lastWasMnemonic && (it + 1) != s.end() ) + { + int w = m_charOffsetsPx[n]; + m_charOffsetsPx.Insert(w, n); + lastWasMnemonic = true; + } + else + { + lastWasMnemonic = false; + } } } + else + { + m_isOk = dc.GetPartialTextExtents(s, m_charOffsetsPx); + } + + // Either way, we should end up with the same number of offsets as + // characters in the original string, except where there is a trailing + // ampersand which is ignored. + wxASSERT( m_charOffsetsPx.GetCount() == s.length() || + m_charOffsetsPx.GetCount() == s.length() - 1); } bool IsOk() const { return m_isOk; } From aa1891e524ac3099a6272d6fc050c2c6a92ca5a8 Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Wed, 13 Nov 2019 18:25:42 +0100 Subject: [PATCH 3/5] Remove unnecessary and unused EllipsizationTestCase class No real changes, just don't use a text fixture which does nothing, this is useless and confusing. --- tests/graphics/ellipsization.cpp | 15 ++++----------- 1 file changed, 4 insertions(+), 11 deletions(-) diff --git a/tests/graphics/ellipsization.cpp b/tests/graphics/ellipsization.cpp index 73ad1a1b10..c3b118520b 100644 --- a/tests/graphics/ellipsization.cpp +++ b/tests/graphics/ellipsization.cpp @@ -23,14 +23,7 @@ // test class // ---------------------------------------------------------------------------- -class EllipsizationTestCase -{ -public: - EllipsizationTestCase() { } -}; - - -TEST_CASE_METHOD(EllipsizationTestCase, "Ellipsization::NormalCase", "[ellipsization]") +TEST_CASE("Ellipsization::NormalCase", "[ellipsization]") { wxMemoryDC dc; @@ -118,7 +111,7 @@ TEST_CASE_METHOD(EllipsizationTestCase, "Ellipsization::NormalCase", "[ellipsiza } -TEST_CASE_METHOD(EllipsizationTestCase, "Ellipsization::EnoughSpace", "[ellipsization]") +TEST_CASE("Ellipsization::EnoughSpace", "[ellipsization]") { // No ellipsization should occur if there's plenty of space. @@ -133,7 +126,7 @@ TEST_CASE_METHOD(EllipsizationTestCase, "Ellipsization::EnoughSpace", "[ellipsiz } -TEST_CASE_METHOD(EllipsizationTestCase, "Ellipsization::VeryLittleSpace", "[ellipsization]") +TEST_CASE("Ellipsization::VeryLittleSpace", "[ellipsization]") { // If there's not enough space, the shortened label should still contain "..." and one character @@ -148,7 +141,7 @@ TEST_CASE_METHOD(EllipsizationTestCase, "Ellipsization::VeryLittleSpace", "[elli } -TEST_CASE_METHOD(EllipsizationTestCase, "Ellipsization::HasThreeDots", "[ellipsization]") +TEST_CASE("Ellipsization::HasThreeDots", "[ellipsization]") { wxMemoryDC dc; From cbda47ff151fd6dc7349670a7b987de248190db3 Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Wed, 13 Nov 2019 18:38:05 +0100 Subject: [PATCH 4/5] Tighten assert check in EllipsizeCalculator Verify that we have exactly as many offsets as expected, both in the case when the string terminates with a (lone) ampersand and when it doesn't, instead of checking that it's one or the other without caring in which case we're. No real changes, but this seems slightly cleaner and avoids questions about the expression "s.length() - 1" when the string is empty. --- src/common/ctrlcmn.cpp | 30 +++++++++++++++++++++--------- 1 file changed, 21 insertions(+), 9 deletions(-) diff --git a/src/common/ctrlcmn.cpp b/src/common/ctrlcmn.cpp index 23d66e75db..f0081b479e 100644 --- a/src/common/ctrlcmn.cpp +++ b/src/common/ctrlcmn.cpp @@ -293,6 +293,8 @@ struct EllipsizeCalculator m_maxFinalWidthPx(maxFinalWidthPx), m_replacementWidthPx(replacementWidthPx) { + size_t expectedOffsetsCount = s.length(); + // Where ampersands are used as mnemonic indicator they should not // affect the overall width of the string and must be removed from the // measurement. Nonetheless, we need to keep them in the string and @@ -315,13 +317,25 @@ struct EllipsizeCalculator it != s.end(); ++it, ++n ) { - if ( *it == '&' && !lastWasMnemonic && (it + 1) != s.end() ) + if ( *it == '&' && !lastWasMnemonic ) { - int w = m_charOffsetsPx[n]; - m_charOffsetsPx.Insert(w, n); - lastWasMnemonic = true; + if ( (it + 1) != s.end() ) + { + int w = m_charOffsetsPx[n]; + m_charOffsetsPx.Insert(w, n); + lastWasMnemonic = true; + } + else // Last character is an ampersand. + { + // This ampersand is removed by RemoveMnemonics() and + // won't be displayed when this string is drawn + // neither, so we intentionally don't use it for our + // calculations neither -- just account for this in the + // assert below. + expectedOffsetsCount--; + } } - else + else // Not an ampersand used to introduce a mnemonic. { lastWasMnemonic = false; } @@ -333,10 +347,8 @@ struct EllipsizeCalculator } // Either way, we should end up with the same number of offsets as - // characters in the original string, except where there is a trailing - // ampersand which is ignored. - wxASSERT( m_charOffsetsPx.GetCount() == s.length() || - m_charOffsetsPx.GetCount() == s.length() - 1); + // characters in the original string. + wxASSERT( m_charOffsetsPx.GetCount() == expectedOffsetsCount ); } bool IsOk() const { return m_isOk; } From 5cf745611b9c17e44882f82ac264679ab6c6a0cb Mon Sep 17 00:00:00 2001 From: iwbnwif Date: Sat, 9 Nov 2019 08:23:15 +0000 Subject: [PATCH 5/5] Don't take trailing whitespace into account when ellipsizing Trailing spaces and tabs are invisible, so the string shouldn't be truncated (possibly losing some printable characters that could have been displayed) just in order to show "..." instead of them. --- src/common/ctrlcmn.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/common/ctrlcmn.cpp b/src/common/ctrlcmn.cpp index f0081b479e..147e613e1d 100644 --- a/src/common/ctrlcmn.cpp +++ b/src/common/ctrlcmn.cpp @@ -596,6 +596,7 @@ wxString wxControlBase::Ellipsize(const wxString& label, const wxDC& dc, { if ( pc == label.end() || *pc == wxS('\n') ) { + curLine.Trim(); curLine = DoEllipsizeSingleLine(curLine, dc, mode, maxFinalWidth, replacementWidth, flags);