Re: [lmi] Direct use of wx data members

[Vadim Zeitlin]

On Sat, 24 Feb 2018 02:14:50 +0000 Greg Chicares <address@hidden> wrote:

GC> It seems odd that a wx data member is accessed directly here:
GC> 
GC> $grep m_parentAsWindow *.?pp
GC> input_sequence_entry.cpp:        (m_parentAsWindow
GC> rounding_view_editor.cpp:        (m_parentAsWindow
GC> 
GC> when a GetParentAsWindow() const accessor is available (though
GC> the accessor was added in wx-2.9.5, and this code may be older).
GC> 
GC> Is it safe to apply the patch below?

 Yes, of course, the accessor is trivial, after all, so the meaning of the
code is not changed at all by this change.

GC> I hesitate only because the accessor's documentation says:
GC> 
GC>   
http://docs.wxwidgets.org/3.1/classwx_xml_resource_handler.html#a59e6a94940a4d1c781f83f6396760a5a
GC> | After CreateResource has been called this will return the item's
GC> | parent as a wxWindow.

 I.e. it can't be used (or can, but would return nullptr) before
CreateResource() is called. But using m_parentAsWindow directly doesn't
change this.

On Sat, 24 Feb 2018 02:47:21 +0000 Greg Chicares <address@hidden> wrote:

GC> On 2018-02-24 02:14, Greg Chicares wrote:
GC> > It seems odd that a wx data member is accessed directly here:
GC> 
GC> ...and also here:
GC> 
GC> $grep '\<m_' ledger_pdf_generator_wx.cpp
GC> [...]
GC>         m_Width  = wxRound(image.GetWidth () / scale_factor);
GC>         m_Height = wxRound(image.GetHeight() / scale_factor);
GC>         x += m_PosX;
GC>         int pos_y = y + m_PosY;
GC>             m_WParser->GetContainer()->InsertCell
GC>         m_Height = render_or_measure(0, oe_only_measure);
GC>         render_or_measure(y + m_PosY, oe_render);
GC>         m_WParser->GetContainer()->InsertCell(new 
numeric_summary_table_cell());

 I don't think we can avoid using the members entirely here, there is no
SetSize() method that would allow to set m_Width and m_Height without
accessing them directly. We could add such a protected method but, to be
honest, I don't see much advantage of using it, there is no encapsulation
to speak of here anyhow.

 And even though SetPos() does exist, using m_Pos[XY] would seem to be
excusable too if only for consistency reasons.

GC> Ideally we'd use only public, documented getters and setters,
GC> which don't seem to be provided yet. E.g., here:
GC>   http://docs.wxwidgets.org/3.1/classwx_html_cell.html
GC> I see GetWidth() const, but the first line of code above needs to
GC> modify the width. This is unlikely to be a real issue today, but
GC> in the long term it seems fragile.

 I could agree that making the members private from the beginning and only
providing protected methods to set them could be better because it would
allow us to change how they're stored or maybe do something when they're
set. But now it's 20 years too late to do it as we can't make all these
members private any more and there doesn't seem to be much point in adding
accessors for them IMHO. Also, empirically, we can note that during these
20 years we never had any need to change their representation, so providing
direct access to them here doesn't look like a horrible mistake (but, to be
fair, it definitely was in other wx classes).

 Regards,
VZ