Opened 6 years ago

Closed 6 years ago

#14705 closed optimization (fixed)

wxTextMeasure implementation

Reported by: mmarsan Owned by: vadz
Priority: low Milestone:
Component: GUI-all Version: stable-latest
Keywords: TextExtent measures needs-work Cc:
Blocked By: Blocking: #14706, #14706, #14706, #14706, #14706, #14706, #14706, #14706, #14706, #14706, #14706, #14706
Patch: yes


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.

Attachments (2)

textmeasure.patch download (46.0 KB) - added by mmarsan 6 years ago.
textmeasure_v2.patch download (44.3 KB) - added by mmarsan 6 years ago.
Version 2, reworked

Download all attachments as: .zip

Change History (19)

Changed 6 years ago by mmarsan

comment:1 Changed 6 years ago by vadz

  • Keywords needs-work added
  • Priority changed from normal to low
  • Status changed from new to confirmed

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?

comment:2 Changed 6 years ago by vadz

  • Blocking 14706 added

comment:3 Changed 6 years ago by mmarsan

I attach the version 2 (replaces the other)

Changed 6 years ago by mmarsan

Version 2, reworked

comment:4 Changed 6 years ago by mmarsan

  • Blocking

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

comment:5 Changed 6 years ago by vadz

  • Blocking
  • Owner set to vadz
  • Status changed from confirmed to accepted

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

comment:6 Changed 6 years ago by VZ

  • Resolution set to fixed
  • Status changed from accepted to closed

(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.

comment:7 Changed 6 years ago by vadz

  • Blocking

(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!

comment:8 Changed 6 years ago by mmarsan

  • Blocking

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

comment:9 Changed 6 years ago by vadz

  • Blocking

(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!

comment:10 Changed 6 years ago by mmarsan

  • Blocking

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

comment:11 Changed 6 years ago by mmarsan

  • Blocking

(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.

comment:12 Changed 6 years ago by mmarsan

  • Blocking

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

comment:13 Changed 6 years ago by vadz

  • Blocking

(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.

comment:14 Changed 6 years ago by vadz

  • Blocking

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

  • samples/minimal/minimal.cpp

    diff --git a/samples/minimal/minimal.cpp b/samples/minimal/minimal.cpp
    index a78e462..89b4682 100644
    a b bool MyApp::OnInit() 
    172172    CreateStatusBar(2);
    173173    SetStatusText("Welcome to wxWidgets!");
    174174#endif // wxUSE_STATUSBAR
     176    wxString choices[] = { "first", "very very very very long", "last" };
     177    wxSizer* sizer = new wxBoxSizer(wxVERTICAL);
     179    wxChoice* c1 = new wxChoice(this, wxID_ANY);
     180    sizer->Add(c1);
     182    wxChoice* c2 = new wxChoice(this, wxID_ANY);
     183    c2->Append(WXSIZEOF(choices), choices);
     184    sizer->Add(c2);
     186    wxChoice* c3 = new wxChoice(this, wxID_ANY);
     187    c3->SetFont(wxNORMAL_FONT->Scaled(2));
     188    c3->Append(WXSIZEOF(choices), choices);
     189    sizer->Add(c3);
     191    SetSizerAndFit(sizer);

comment:15 Changed 6 years ago by vadz

  • Blocking

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

comment:16 Changed 6 years ago by VZ

  • Blocking

(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.

comment:17 Changed 6 years ago by VZ

  • Blocking

(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.

Note: See TracTickets for help on using tickets.