                                                                                                                                                                                                                                                                
Received-SPF: neutral (google.com: 82.238.156.189 is neither permitted nor denied by best guess record for domain of va...@wxwidgets.org) client-ip=82.238.156.189;
Message-Id: <E1TJ3Kw-0005Ut-J8@smtp.tt-solutions.com>
Date: Tue, 2 Oct 2012 16:20:36 +0200
From: Vadim Zeitlin <va...@wxwidgets.org>
Subject: Re: [wx-dev] wxTextMeasure implementation
To: wx-dev@googlegroups.com
MIME-Version: 1.0
Content-Type: MULTIPART/SIGNED; protocol="application/pgp-signature"; micalg=pgp-sha1; BOUNDARY="706996326-41-1349187636=:2696"
References: <5069F672.80908@gmail.com>
In-Reply-To: <5069F672.80908@gmail.com>

--706996326-41-1349187636=:2696
Content-Type: TEXT/PLAIN; CHARSET=US-ASCII
Content-Disposition: INLINE

On Mon, 01 Oct 2012 22:00:50 +0200 Manolo wrote:

M> I'm sure you understand that I have not dedicated such many years to
M> work with wx internals as you did. So, I'm prone to make faults.

 Hi,

 Everybody makes mistakes (myself definitely included), it's just that it's
much simpler to notice them when reviewing the code written by somebody
else than when looking at your own code. I hope you don't take the number
of comments I made about your patch badly.

M> For what I understood from other post, you prefer to have this new
M> class internal. At least in a beginning.

 OK, I'm actually not sure about this, but let's do it like this. If it is
internal, then the headers should go into include/wx/private and
include/wx/$PORT/private directories, you should remove the DLL export
macros and can include windows.h directly from wx/msw/private/textmeasure.h.

M> The 'm_selfCalled' var allows a simple call like
M>     myWindow->GetTextExtent(...)
M> to work (init, do the job, release resources). And because other
M> calls like GetMultiLineTextExtent() or GetLargestStringExtent() make
M> intensive use of GetTextExtent(), avoid "init & release" repetition.

 Yes, I understand this, but I don't like this approach because it relies
on m_selfCalled being set/updated correctly by all the derived classes and
it's too error-prone. Worse, if you forget to do it, the code still works,
just inefficiently. This is not good.

M> Your approach of public GetTextExtent() calls Init(), then calls
M> virtual DoGetTextExtent() and at last calls Restore() IMO makes the
M> code more difficult to understand for a future reader. It becomes a
M> labyrinth of base and derived classes, public and virtual members.

 There is probably some merit in this argument and it could indeed be less
clear for a very novice programmer but this is a perfectly widely used and
well-understood C++ technique which is also already used in many places in
wxWidgets itself (albeit not as many as I'd like). So I really don't think
we should discard it just because of this. A novice will hopefully find it
not very difficult to learn. And everybody else won't have any troubles at
all with (and how many novices exactly go spelunking in wxWidgets sources
anyhow?).

M> For those who don't know the "run path", I expose an example:
M>     wxDC myDC;
M>     myDC->GetTextExtent(text...);
M> Internals: wxDC is generic. It stores a pointer 'm_pimpl' to the
M> abstract class wxDCImpl.
M> In wxGTK, it depends on using Gtk2 or Gtk3. In Gtk2 the inheritance is
M> wxWindowDCImpl child of wxGTKDCImpl child of wxDCImpl. In Gtk3 is
M> wxWindowDCImpl child of wxGTKCairoDCImpl child of wxGCDCImpl child of
M> wxDCImpl.
M> myDC->GetTextExtent() calls m_pimpl->DoGetTextExtent() which is the
M> virtual member of the proper version of wxWindowDCImpl. This does the job.
M> And now, let's add wxTextMeasure.
M> Because of backward compatibility, we keep wxDC::GetTextExtent(). So
M> wxWindowDCImpl creates a wxTextMeasure object and calls the public
M> wxTextMeasure::GetTextExtent() which should call the platform dependant
M> virtual DoGetTextExtent().

 This is indeed rather complicated but:

1. You absolutely don't need to know about this when you just use wxWidgets.
2. Most of the extra steps have nothing to do with wxTextMeasure, e.g. the
   fact that wxDC uses wxDCImpl is really completely orthogonal to it.

M> All of this labyrinth can be avoided just by:
M>     wxDC myDC;
M>     wxTextMeasure myTm;
M>     myTm->GetTextExtent(text,..., myDC...);
M> and providing virtual platform-dependant versions of GetTextExtent().

 I don't like this at all. This makes the public API more confusing and for
what benefit exactly?

M> I suppose anyone who wants to write a version of GetTextExtent() must
M> know the internals. Either must be aware of the m_selfCalled flag or
M> must be aware of overriding DoGetTextExtent() instead of GetTextExtent(),
M> and that both functions will be used in the "run path".

 In the latter case you can't override GetTextExtent() because it's not
virtual. Also, again, it's a very common C++ idiom to have public Foo() and
private virtual DoFoo(). If somebody doesn't know about it yet, it would be
a good occasion to learn about it.

M> I agree using your approach allows overloading public methods. Do we
M> really need them?

 Yes, I think it's much more convenient.

M> Using
M>    wxTextMeasure tm(this, m_font);
M>    return tm.GetPartialTextExtents(text, widths, m_scaleX);
M> is clearer, yes.

 And this is important.

M> But
M>    wxTextMeasure tm(this, m_font);
M>    tm.GetPartialTextExtents(text, widths1, m_scaleX);
M>    SetFont(newfont);
M>    tm.GetPartialTextExtents(text, widths2, m_scaleX);
M> is wrong, because the 'tm' is not aware of new font.

 Well, you really need to make an effort to do this, don't you? When you
construct wxTextMeasure you say "I want to measure the text on this window
(or DC) using this font". Obviously things won't work correctly if you
change the font later.

 I think it's a non-problem, we don't devise API impossible to misuse
(especially private API) but just API that is difficult to misuse
accidentally. And there is nothing accidental in the above code.

 If you really want to allow it, you could have wxTextMeasure::SetFont()
but, frankly, I wouldn't bother.

M> This case is possible if the user does not know internals.
M>    tm.GetPartialTextExtents(text, widths, m_scaleX, font, this);
M> has no doubt and uses always the right font.

 Yes, and the parameter list is perfectly unreadable (you chose a short
form, in your patch there are calls with 7 parameters, 2 of which can be
indistinguishable from each other NULLs).

M> I put the header in wx.h because many basic controls (button, etc)
M> need some text measurement.

 If it's a private header, it's not even a question, it can't appear in
public wx.h.

M> I passed a pointer instead of a const reference because some time
M> ago I read http://www.gotw.ca/gotw/081.htm Since then, if I want
M> to be sure that no copy is done, despite of if the compiler is smart
M> or not, I use a pointer and take care of not modifying the object
M> myself, instead of using 'const'.
M> I may agree with using 'const' for a member that is going to be
M> overridden who knows how many times. Is this the case?

 wxWidgets convention is to use const reference for all parameters and to
only use const pointers for the parameters that can be NULL. I didn't read
the GotW above but I can't believe Herb Sutter would recommend using (raw)
pointers and I don't see any mention of it from a quick glance at the above
URL. Anyhow, there is no copying being done by the compiler there and, as I
mentioned, I think this class should be made non-copyable anyhow to make it
completely impossible.

 Regards,
VZ

--706996326-41-1349187636=:2696
Content-Type: APPLICATION/PGP-SIGNATURE

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (MingW32)

iEYEABECAAYFAlBq+DQACgkQBupB3k9sHobZBgCeJVDnR7xOOW/+rJRnNaweyVpH
1K4An2Q/lZCOEn8Ys9gUEWtwqK0IjIW8
=i9AJ
-----END PGP SIGNATURE-----

--706996326-41-1349187636=:2696--