lmi
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

[lmi] Resolving TransferString() TODO comment


From: Vadim Zeitlin
Subject: [lmi] Resolving TransferString() TODO comment
Date: Mon, 27 Jun 2016 00:10:33 +0200

 Hello,

 I've finally returned to this part of your almost month old email, quoting
http://lists.nongnu.org/archive/html/lmi/2016-06/msg00003.html where you
wrote:

> Could I also ask you to opine on the "TODO ??" markers in transferor.cpp',
> because it gladdens my heart to resolve those? This one may be easy for you:
> 
> // TODO ?? Use TransferString() with class wxControlWithItems instead
> // of its derived classes, now that wx has been changed to implement
> // wxControlWithItems::SetSelection(). But consider whether class
> // wxItemContainerImmutable can be used for all those classes and
> // wxRadioBox as well.

 AFAICS it doesn't make much sense to have a function working with
wxControlWithItems because we would still need a separate version for
wxRadioBox and so would still need a template function to avoid duplicating
code for these 2 unrelated classes.

 Going down to the common wxItemContainerImmutable class can work, but
unfortunately deriving from wxItemContainerImmutable is not quite enough
for TransferString() since it also uses GetName() for its diagnostic
message and GetName() is a member of wxWindow only and isn't accessible via
a wxItemContainerImmutable reference. So to make this work I had to double
the argument of TransferString() and pass it the same object as both a
wxItemContainerImmutable and a wxWindow, please see

        https://github.com/vadz/lmi/pull/43/

As the commit message hints, I'm not really sure if it's a net gain. There
are definitely some advantages to not using a template, as mention in the
comment in my PR, but the stutter in the calls to this function looks ugly
and this also allows passing 2 different objects to it which really
shouldn't be possible.

 Unfortunately the only possibility to avoid it that I see involves using
templates anyhow, e.g. the function could take a single WindowWithItems and
we'd have, in pseudo-code, something like this:

        struct WindowWithItems
        {
                virtual wxItemContainerImmutable& GetContainer() const = 0;
                virtual wxWindow& GetWindow() const = 0;
        };

        template <typename T>
        struct WindowWithItemsConcrete {
                explicit WindowWithItemsConcrete(T& control)
                        :container_(control)
                        ,window_   (window)
                {
                }

                virtual wxItemContainerImmutable& GetContainer() const
                {
                        return container_;
                }

                virtual wxWindow& GetWindow() const
                {
                        return window_;
                }

                wxItemContainerImmutable& container_;
                wxWindow&                 window_;
        };

But this would add two more classes and extra complexity for not much gain,
really, so I don't really propose doing it and mention it only for
completeness.

 In fact the only reasonable solution I see would be to add a
GetContainingWindow() accessor to wxItemContainerImmutable itself as I
think it could be useful elsewhere and we do have similar methods in other
similar classes, e.g. wxTextEntry has GetEditableWindow(). Then we could
take just a wxItemContainerImmutable in TransferString() and everything
would work nicely and simply.

 Of course, another solution is to just drop the "TODO ??" comment and live
with the current template function which does bloat the code a little (it's
instantiated 5 times, i.e. 4 times more than really necessary), but
otherwise doesn't present any problems.

 Please let me know what, if anything, would you like to do here,
VZ


reply via email to

[Prev in Thread] Current Thread [Next in Thread]