#14706: Choice changes for wxTextMeasure

54 views
Skip to first unread message

wxTrac

unread,
Sep 27, 2012, 9:00:54 PM9/27/12
to wx-...@googlegroups.com
Ticket URL: <http://trac.wxwidgets.org/ticket/14706>

#14706: Choice changes for wxTextMeasure
-------------------------------------+--------------------------------------
Reporter: mmarsan | Owner:
Type: optimization | Status: new
Priority: normal | Milestone:
Component: GUI-all | Version: 2.9-svn
Keywords: choice combo textextent | Blockedby:
Patch: 1 | Blocking:
-------------------------------------+--------------------------------------
This patch starts the use of wxTextMeasure::GetLargestStringExtent()
It requires the patch in ticket 14705.

Changes are in GetBestSize() of src/msw/choice.cpp and src/gtk/choice.cpp

This code can be also easily copied to other (i.e. OX) choice and combobox
implementations.


--
Ticket URL: <http://trac.wxwidgets.org/ticket/14706>

wxTrac

unread,
Sep 30, 2012, 6:18:21 PM9/30/12
to wx-...@googlegroups.com
Ticket URL: <http://trac.wxwidgets.org/ticket/14706#comment:1>

#14706: Choice changes for wxTextMeasure
-------------------------------------+--------------------------------------
Reporter: mmarsan | Owner:
Type: optimization | Status: new
Priority: normal | Milestone:
Component: GUI-all | Version: 2.9-svn
Keywords: choice combo textextent | Blockedby: 14705
Patch: 1 | Blocking:
-------------------------------------+--------------------------------------
Changes (by vadz):

* blockedby: => 14705



--
Ticket URL: <http://trac.wxwidgets.org/ticket/14706#comment:1>

wxTrac

unread,
Oct 5, 2012, 7:27:09 PM10/5/12
to wx-...@googlegroups.com
Ticket URL: <http://trac.wxwidgets.org/ticket/14706#comment:3>

#14706: Choice changes for wxTextMeasure
-------------------------------------+--------------------------------------
Reporter: mmarsan | Owner:
Type: optimization | Status: new
Priority: normal | Milestone:
Component: GUI-all | Version: 2.9-svn
Keywords: choice combo textextent | Blockedby: 14705, 14705
Patch: 1 | Blocking:
-------------------------------------+--------------------------------------

Comment(by mmarsan):

I attach the new version (v2) reworked for v2 of patch in ticket 14705


--
Ticket URL: <http://trac.wxwidgets.org/ticket/14706#comment:3>

wxTrac

unread,
Oct 17, 2012, 6:37:38 PM10/17/12
to wx-...@googlegroups.com
Ticket URL: <http://trac.wxwidgets.org/ticket/14706#comment:5>

#14706: Choice changes for wxTextMeasure
-------------------------------------+--------------------------------------
Reporter: mmarsan | Owner:
Type: optimization | Status: new
Priority: low | Milestone:
Component: GUI-all | Version: 2.9-svn
Keywords: choice combo textextent | Blockedby: 14705
Patch: 1 | Blocking:
-------------------------------------+--------------------------------------
Changes (by vadz):

* priority: normal => low
* blockedby: 14705, 14705, 14705, 14705 => 14705


Comment:

The code is so similar in the 2 ports that it would be really better to
put it into `wxChoiceBase`. You could use `#if !wxUSE_GENERIC_TEXTMEASURE`
to do it only for the ports that actually have `wxTextMeasure` or just do
it unconditionally, AFAICS it shouldn't do any lasting harm neither.

Could you please update the patch to do it at the base class level? TIA!


--
Ticket URL: <http://trac.wxwidgets.org/ticket/14706#comment:5>

wxTrac

unread,
Oct 26, 2012, 8:49:17 PM10/26/12
to wx-...@googlegroups.com
Ticket URL: <http://trac.wxwidgets.org/ticket/14706#comment:6>

#14706: Choice changes for wxTextMeasure
-------------------------------------+--------------------------------------
Reporter: mmarsan | Owner:
Type: optimization | Status: new
Priority: low | Milestone:
Component: GUI-all | Version: 2.9-svn
Keywords: choice combo textextent | Blockedby: 14705
Patch: 1 | Blocking:
-------------------------------------+--------------------------------------

Comment(by mmarsan):

I've attached the version 3, replaces the others.


--
Ticket URL: <http://trac.wxwidgets.org/ticket/14706#comment:6>

wxTrac

unread,
Oct 27, 2012, 8:19:06 AM10/27/12
to wx-...@googlegroups.com
Ticket URL: <http://trac.wxwidgets.org/ticket/14706#comment:7>

#14706: Choice changes for wxTextMeasure
-------------------------------------+--------------------------------------
Reporter: mmarsan | Owner:
Type: optimization | Status: new
Priority: low | Milestone:
Component: GUI-all | Version: 2.9-svn
Keywords: choice combo textextent | Blockedby: 14705
Patch: 1 | Blocking:
-------------------------------------+--------------------------------------

Comment(by vadz):

Thanks, but it's still unfortunately not perfect. Here is what I'd like to
see improved:

1. Provide `wxChoiceBase::DoGetBestSize()` for all ports. As
`wxTextMeasure` can now be used everywhere (by just forwarding back to
`wxDC` if not implemented natively), I don't see any reason to explicitly
test for `__WXMSW__ || __WXGTK__` here (and if such reason does exist, it
would still be better to use `#if !wxUSE_GENERIC_TEXTMEASURE` instead of
explicitly hardcoding the platforms currently implementing it).
1. Remove `CacheBestSize()` from there, it's already done by
`wxWindowBasE::GetBestSize()`.
1. Important: avoid platform-specific code in `wxChoiceBase`. The whole
idea of having it is to avoid these ugly `#ifdefs`. Instead, the base
class version should contain only the code ''really'' common to all
platforms and wxMSW should still continue to override it and handle
`wxCB_SIMPLE` by adding the extra margin there (as for wxGTK, I don't
think it's special case is needed at all but I could be wrong).

Could you please update the patch like this? TIA!


--
Ticket URL: <http://trac.wxwidgets.org/ticket/14706#comment:7>

wxTrac

unread,
Oct 28, 2012, 8:59:25 PM10/28/12
to wx-...@googlegroups.com
Ticket URL: <http://trac.wxwidgets.org/ticket/14706#comment:8>

#14706: Choice changes for wxTextMeasure
-------------------------------------+--------------------------------------
Reporter: mmarsan | Owner:
Type: optimization | Status: new
Priority: low | Milestone:
Component: GUI-all | Version: 2.9-svn
Keywords: choice combo textextent | Blockedby: 14705
Patch: 1 | Blocking:
-------------------------------------+--------------------------------------

Comment(by mmarsan):

{{{#if !wxUSE_GENERIC_TEXTMEASURE}}} may not be yet defined for the time
wxChoiceBase gets compiled: But {{{__WXMSW__ __WXGTK__}}} are.


--
Ticket URL: <http://trac.wxwidgets.org/ticket/14706#comment:8>

wxTrac

unread,
Oct 28, 2012, 9:15:42 PM10/28/12
to wx-...@googlegroups.com
Ticket URL: <http://trac.wxwidgets.org/ticket/14706#comment:9>

#14706: Choice changes for wxTextMeasure
-------------------------------------+--------------------------------------
Reporter: mmarsan | Owner:
Type: optimization | Status: new
Priority: low | Milestone:
Component: GUI-all | Version: 2.9-svn
Keywords: choice combo textextent | Blockedby: 14705
Patch: 1 | Blocking:
-------------------------------------+--------------------------------------

Comment(by mmarsan):

Replying to [comment:8 mmarsan]:
> {{{#if !wxUSE_GENERIC_TEXTMEASURE}}} may not be yet defined for the time
wxChoiceBase gets compiled: But {{{__WXMSW__ __WXGTK__}}} are.

Forget this comment. wx/private/textmeasure.h is included so
wxUSE_GENERIC_TEXTMEASURE is defined.


--
Ticket URL: <http://trac.wxwidgets.org/ticket/14706#comment:9>

wxTrac

unread,
Oct 29, 2012, 6:20:08 PM10/29/12
to wx-...@googlegroups.com
Ticket URL: <http://trac.wxwidgets.org/ticket/14706#comment:10>

#14706: Choice changes for wxTextMeasure
-------------------------------------+--------------------------------------
Reporter: mmarsan | Owner:
Type: optimization | Status: new
Priority: low | Milestone:
Component: GUI-all | Version: 2.9-svn
Keywords: choice combo textextent | Blockedby: 14705
Patch: 1 | Blocking:
-------------------------------------+--------------------------------------

Comment(by mmarsan):

4th version. I hope it is the last :)


--
Ticket URL: <http://trac.wxwidgets.org/ticket/14706#comment:10>

wxTrac

unread,
Nov 1, 2012, 8:51:48 AM11/1/12
to wx-...@googlegroups.com
Ticket URL: <http://trac.wxwidgets.org/ticket/14706#comment:11>

#14706: Choice changes for wxTextMeasure
-------------------------------------+--------------------------------------
Reporter: mmarsan | Owner:
Type: optimization | Status: confirmed
Priority: low | Milestone:
Component: GUI-all | Version: 2.9-svn
Keywords: choice combo textextent | Blockedby: 14705
Patch: 1 | Blocking:
-------------------------------------+--------------------------------------
Changes (by vadz):

* status: new => confirmed


Comment:

Thanks, much better but I don't really understand why do we need to test
for `wxUSE_GENERIC_TEXTMEASURE`, `wxTextMeasure` is supposed to work in
this case too, so I've just removed it. Please let me know if I broke
something.

I also don't see any need for the code dealing with `wxFont` explicitly,
using the window font should be the default behaviour anyhow and if it
isn't, then it's a problem in `wxTextMeasure` itself and should be fixed
there.


--
Ticket URL: <http://trac.wxwidgets.org/ticket/14706#comment:11>

wxTrac

unread,
Nov 1, 2012, 9:15:56 AM11/1/12
to wx-...@googlegroups.com
Ticket URL: <http://trac.wxwidgets.org/ticket/14706#comment:12>

#14706: Choice changes for wxTextMeasure
-------------------------------------+--------------------------------------
Reporter: mmarsan | Owner:
Type: optimization | Status: confirmed
Priority: low | Milestone:
Component: GUI-all | Version: 2.9-svn
Keywords: choice combo textextent | Blockedby: 14705
Patch: 1 | Blocking:
-------------------------------------+--------------------------------------

Comment(by vadz):

For the reference, here is the code I used for testing:
{{{
#!diff
diff --git a/samples/minimal/minimal.cpp b/samples/minimal/minimal.cpp
index a78e462..89b4682 100644
--- a/samples/minimal/minimal.cpp
+++ b/samples/minimal/minimal.cpp
@@ -172,6 +172,23 @@ bool MyApp::OnInit()
CreateStatusBar(2);
SetStatusText("Welcome to wxWidgets!");
#endif // wxUSE_STATUSBAR
+
+ wxString choices[] = { "first", "very very very very long", "last" };
+ wxSizer* sizer = new wxBoxSizer(wxVERTICAL);
+
+ wxChoice* c1 = new wxChoice(this, wxID_ANY);
+ sizer->Add(c1);
+
+ wxChoice* c2 = new wxChoice(this, wxID_ANY);
+ c2->Append(WXSIZEOF(choices), choices);
+ sizer->Add(c2);
+
+ wxChoice* c3 = new wxChoice(this, wxID_ANY);
+ c3->SetFont(wxNORMAL_FONT->Scaled(2));
+ c3->Append(WXSIZEOF(choices), choices);
+ sizer->Add(c3);
+
+ SetSizerAndFit(sizer);
}


}}}


--
Ticket URL: <http://trac.wxwidgets.org/ticket/14706#comment:12>

wxTrac

unread,
Nov 1, 2012, 9:32:56 AM11/1/12
to wx-...@googlegroups.com
Ticket URL: <http://trac.wxwidgets.org/ticket/14706#comment:13>

#14706: Choice changes for wxTextMeasure
-------------------------------------+--------------------------------------
Reporter: mmarsan | Owner:
Type: optimization | Status: confirmed
Priority: low | Milestone:
Component: GUI-all | Version: 2.9-svn
Keywords: choice combo textextent | Blockedby: 14705
Patch: 1 | Blocking:
-------------------------------------+--------------------------------------

Comment(by vadz):

Ugh, testing under MSW shows that the best size is broken, the last choice
above is now truncated... Looking into it...


--
Ticket URL: <http://trac.wxwidgets.org/ticket/14706#comment:13>

wxTrac

unread,
Nov 1, 2012, 1:15:18 PM11/1/12
to wx-...@googlegroups.com
Ticket URL: <http://trac.wxwidgets.org/ticket/14706#comment:15>

#14706: Choice changes for wxTextMeasure
-------------------------------------+--------------------------------------
Reporter: mmarsan | Owner:
Type: optimization | Status: confirmed
Priority: low | Milestone:
Component: GUI-all | Version: 2.9-svn
Keywords: choice combo textextent | Blockedby: 14705
Patch: 1 | Blocking:
-------------------------------------+--------------------------------------

Comment(by VZ):

(In [72844]) Allow constructing wxGTK wxTextMeasure with NULL font.

The font is explicitly documented as being possibly NULL in the base class
and
wxMSW handles NULL font just fine, so also handle it in the GTK version.

See #14706.


--
Ticket URL: <http://trac.wxwidgets.org/ticket/14706#comment:15>

wxTrac

unread,
Nov 1, 2012, 1:15:30 PM11/1/12
to wx-...@googlegroups.com
Ticket URL: <http://trac.wxwidgets.org/ticket/14706#comment:16>

#14706: Choice changes for wxTextMeasure
---------------------------+------------------------------------------------
Reporter: mmarsan | Owner:
Type: optimization | Status: closed
Priority: low | Milestone:
Component: GUI-all | Version: 2.9-svn
Resolution: fixed | Keywords: choice combo textextent
Blockedby: 14705 | Patch: 1
Blocking: |
---------------------------+------------------------------------------------
Changes (by VZ):

* status: confirmed => closed
* resolution: => fixed


Comment:

(In [72848]) Refactor and simplify wxChoice::DoGetBestSize().

Use wxTextMeasure instead of duplicating its code and also reuse the code
between different ports.

Closes #14706.


--
Ticket URL: <http://trac.wxwidgets.org/ticket/14706#comment:16>
Reply all
Reply to author
Forward
0 new messages