#14705: wxTextMeasure implementation

[wxTrac]

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

#14705: wxTextMeasure implementation
---------------------------------+------------------------------------------
Reporter: mmarsan | Owner:
Type: optimization | Status: new
Priority: normal | Milestone:
Component: GUI-all | Version: 2.9-svn
Keywords: TextExtent measures | Blockedby:
Patch: 1 | Blocking:
---------------------------------+------------------------------------------
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.


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

[wxTrac]

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

#14705: wxTextMeasure implementation
--------------------------------------------+-------------------------------
Reporter: mmarsan | Owner:
Type: optimization | Status: confirmed
Priority: low | Milestone:
Component: GUI-all | Version: 2.9-svn
Keywords: TextExtent measures needs-work | Blockedby:
Patch: 1 | Blocking:
--------------------------------------------+-------------------------------
Changes (by vadz):

* keywords: TextExtent measures => TextExtent measures needs-work
* priority: normal => low
* status: new => confirmed


Comment:

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
{{{
#!cpp
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.
1. I don't think this header is important enough to be added to
`wx/wx.h`, it's kind of "advanced".
1. These classes should presumably be all made non-copyable using
`wxDECLARE_NO_COPY_CLASS()`.
1. There is an extra semicolon in `wxTextMeasureMSW() {};`.
1. `wxTextMeasureBase` dtor should be virtual, I wonder how/why didn't
you get g++ warnings about this.
1. 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).
1. We're not limited by 8.3 names any longer so we could call the file
`textmeasurecmn.cpp`.
1. We should use `wxVector<wxString>` instead of `std::vector<wxString>`.
Also, why is it passed as a pointer and not a (const) reference?


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

[wxTrac]

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

#14705: wxTextMeasure implementation
--------------------------------------------+-------------------------------
Reporter: mmarsan | Owner:
Type: optimization | Status: confirmed
Priority: low | Milestone:
Component: GUI-all | Version: 2.9-svn

Keywords: TextExtent measures needs-work | Blockedby:

Patch: 1 | Blocking: 14706
--------------------------------------------+-------------------------------

Comment(by mmarsan):

I attach the version 2 (replaces the other)


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

[wxTrac]

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

#14705: wxTextMeasure implementation
--------------------------------------------+-------------------------------
Reporter: mmarsan | Owner: vadz
Type: optimization | Status: accepted
Priority: low | Milestone:
Component: GUI-all | Version: 2.9-svn

Keywords: TextExtent measures needs-work | Blockedby:
Patch: 1 | Blocking: 14706
--------------------------------------------+-------------------------------

Changes (by vadz):

* owner: => vadz
* status: confirmed => accepted
* blocking: 14706, 14706 => 14706


Comment:

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


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

[wxTrac]

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

#14705: wxTextMeasure implementation
---------------------------+------------------------------------------------
Reporter: mmarsan | Owner: vadz
Type: optimization | Status: closed
Priority: low | Milestone:
Component: GUI-all | Version: 2.9-svn
Resolution: fixed | Keywords: TextExtent measures needs-work

Blockedby: | Patch: 1
Blocking: 14706 |

---------------------------+------------------------------------------------
Changes (by VZ):

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


Comment:

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


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