#209 Hide public data members

[Ognyan Chernokozhev]

In OWLNext 7.x the macros public_data and protected_data are still in use and default to public.

It is time for the data members to be made protected or private. See [discussion:668612c54d] and [feature-requests:#36] "Add accessors for data members".

[Vidar Hasfjord]

Review of [r5981]:

• Rather than use GetWindowAttr to set style, use SetStyle or ModifyStyle.
• Rather than use GetWindowAttr to set ID, add TWindow::SetId to complement TWindow::GetId.
• Rather than use GetWindowAttr to set AccelTable, use SetAcceleratorTable.

[Damon Register]

yesterday I got the latest 7,1,0,6050 from svn. I built it without any trouble. I attempted to build my project and got a few build errors, all related to data members that are no longer public.
I found this comment in defs.h
// Strict data makes all data members private. Accessors must then be used
I believe that I have found most of those accessors and have replaced
myTEdit.Attr.Id with myTEdit.GetId().
The only one I can't find is THostInfoManager::HostEntry. HostEntry is not public any more and I have not found an accessor for HostEntry. I don't know all the ways that one might use HostEntry but in my case I need it for this: HostEntry->GetINetAddress()

[Ognyan Chernokozhev]

I am not at all familiar with the THostInfo* classes and I have never used them.
Looking at the code, we should add an accessor for HostEntry to make it a clean encapsulation. Since it seems that it does not change after constructions, it should return a const pointer (or even a const reference?)
Also the methods of THostEntry should be made const-correct.

[Vidar Hasfjord]

@jogybl wrote:

Since it seems that [THostInfoManager::HostEntry] does not change after constructions, [GetHostEntry] should return a const pointer (or even a const reference?)

Actually, with an accessor as a replacement, HostEntry is no longer needed. It is only a pointer to HostInfoBuffer, and outside the constructor, it is not referenced anywhere else. I agree that the accessor should return const-reference:

auto GetHostEntry() const noexcept -> const THostEntry&
{ return *reinterpret_cast<const THostEntry*>(&HostInfoBuffer); }

But note that this cast, as in the current constructor, is not standard-compliant. The item type in the buffer is the Winsock type hostent, which is the base class of THostEntry, so the cast is an illegal downcast. However, you would probably have to redesign the whole thing to fix that.

PS. As these Winsock encapsulation classes are horrible, and since there are better libraries today for this (e.g. use Boost, or even the Winsock library directly, rather than this kludge), I propose that the Winsock encapsulation classes are moved to OwlExt. They have nothing to do with basic GUI application functionality, which is the domain of the OWLNext core. In Owlet, I have removed them altogether.

Edit: I have now circumvented this issue altogether by removing the use of the macro "public_data" in the Winsock classes [r6053]. The members are now public as in the original API. Note that I have deprecated and moved the Winsock classes to OWLExt [r6052], and I don't expect there will be any further improvements to the API of these classes.

[Vidar Hasfjord]

Hi Ognyan,

I've quickly reviewed [r6054], and it looks good (except for a tab in "window.h").

However, my review of TWindow::SetId got me thinking about improving things. Your straight-forward implementation perfectly mirrors GetId, in that it only accesses the Attr member structure. This is unlike GetStyle and SetStyle. If called before window creation, they will modify the creation attributes in Attr, but after creation they will use GetWindowLong and SetWindowLong with GWL_STYLE. I think GetId and SetId should work similarly, by calling GetWindowLong and SetWindowLong with GWL_ID after window creation.

This improvement belongs in a separate ticket, though, so I will make another feature request.

Edit: See [feature-requests:#211].