wxTextMeasure implementation

[mmarsan]

This patch gathers in a new wxTextMeasure class these methods:
* wxDC / wxWindow GetTextExtent()
* wxDC GetMultiLineTextExtent()
* wxDC GetPartialTextExtents()
* new GetLargestStringExtent()

Where has been possible, an only common method is called.
If there are platform specializations, they are used in their
wxTextMeasure platform implementation.

Most of the code has been reused from dc.cpp, dcbase.cpp, window.cpp.

Apart from the new method GetLargestStringExtent(), notice that now
it is possible to call this class from anywhere, just passing valid
font and valid dc or window.

I've tested it with Ubuntu 12.04 Gtk-2/Gtk-3 and MSW XP, using
combo and richtext samples.

[vadz]

I'm not sure if we decided whether this class ought to be public or not. But if it is supposed to be public (as I think), it should be documented. While if it isn't, then it shouldn't be exported from the DLL, i.e. shouldn't have WXDLLIMPEXP_CORE.

Also, I really dislike m_selfCalled that is supposed to be manually checked in all the derived classes. I'd strongly prefer to have an approach with non-virtual public functions that would call InitCached() and then call a virtual protected (or private) function that wouldn't need to do anything but could rely on the necessary preparation being done. Of course, the same private function could be called from multiple methods, e.g. DoGetTextExtent() could be called from both public GetTextExtent() and public GetLargestStringExtent(). This would be much cleaner IMO and would also have an additional nice side effect of allowing to have overloaded public methods easily.

I also have some doubts concerning the public class API: wouldn't it be better/more convenient to have different ctors specifying either a DC or a window and an optional font instead of having the same 3 trailing defaulted arguments in all the public functions? I.e. I'd prefer writing

wxTextMeasure tm(this, m_font);
return tm.GetPartialTextExtents(text, widths, m_scaleX);

instead of the current, more cryptic, version.

Finally -- as I just realized -- there is a big problem with the other (non-GTK/MSW) platforms where this patch breaks compilation. It uses wxTextMeasure in wxDCImpl::DoGetPartialTextExtents() but this class can't be created (it's abstract!) there. You need to provide a generic implementation of this class forwarding to the (not yet refactored) code in wxDC/wxWindow for these platforms. Or at least (although this would result in a pretty horrible mess and a lot of code duplication so I'm not sure if it's really acceptable) keep the existing code in src/common/dcbase.cpp for these platforms.

Other than that just some minor remarks:

1. We shouldn't include <windows.h> (even via wx/msw/wrapwin.h) from a public header (again, assuming it is public, of course). So you should use WXHDC and WXHFONT in the MSW header instead of the real things.
2. I don't think this header is important enough to be added to wx/wx.h, it's kind of "advanced".
3. These classes should presumably be all made non-copyable using wxDECLARE_NO_COPY_CLASS().
4. There is an extra semicolon in wxTextMeasureMSW() {};.
5. wxTextMeasureBase dtor should be virtual, I wonder how/why didn't you get g++ warnings about this.
6. Forward declarations should use WXDLLIMPEXP_FWD_CORE (I think this is still needed by some MinGW or Cygwin configurations, if this is not true then we should delete them from everywhere but for now it's simpler to just use them consistently).
7. We're not limited by 8.3 names any longer so we could call the file textmeasurecmn.cpp.
8. We should use wxVector<wxString> instead of std::vector<wxString>. Also, why is it passed as a pointer and not a (const) reference?

[mmarsan]

I attach the version 2 (replaces the other)

[mmarsan]

Version 2, reworked

[mmarsan]

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

[vadz]

Just FYI: thanks for the update, I'm working on applying this.

[VZ]

(In [72699]) Factor out text measurement from wxDC and wxWindow into wxTextMeasure.

Add a new private wxTextMeasure class implementing methods for measuring text
and move the often duplicated (but not always identically) code for doing the
same from wxDC and wxWindow into it.

Currently this class is only really implemented in wxMSW and wxGTK.

Also extend the test for text measuring functions and rename it to
MeasuringTextTestCase from MeasuringContextTestCase as it's not wxGC-specific
any more.

Closes #14705.

[vadz]

(In #14706) 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!

[mmarsan]

(In #14706) I've attached the version 3, replaces the others.

[vadz]

(In #14706) 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).
2. Remove CacheBestSize() from there, it's already done by wxWindowBasE::GetBestSize().
3. 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!

[mmarsan]

(In #14706) #if !wxUSE_GENERIC_TEXTMEASURE may not be yet defined for the time wxChoiceBase gets compiled: But __WXMSW__ __WXGTK__ are.

[mmarsan]

(In #14706) Replying to 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.

[mmarsan]

(In #14706) 4th version. I hope it is the last :)

[vadz]

(In #14706) 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.

[vadz]

(In #14706) For the reference, here is the code I used for testing:

samples/minimal/minimal.cpp

@@ -172 +172 @@
     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);
 }
 
 

[vadz]

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

[VZ]

(In #14706) (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.

[VZ]

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

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

Closes #14706.