OWLNext public data members

Hello,

There are a lot of OWLNext class members, which are declared as "public_data" or "protected_data", which are macros defined as follows:

#if defined(OWL_STRICT_DATA)  
# define public_data private  
# define protected_data private  
#else  
# define public_data public  
# define protected_data protected  
#endif  

I plan to try to enforce OWL_STRICT_DATA by default, except when building in OWL5.0 compatibility mode.

Also, there are some classes like TCmdLine, which are exposing their data members as public. I will hide them and add the necessary accessor methods.

We should provide the getter/setter method first (in 6.30.x) and enforce the
use in next mayor release (6.40) with support for OWL5_COMPAT_MODE.

Sebas

Hello,
it seems to me there is no need to hide all data members. If no other data
values depend upon this one the declaration it as "protected" ot "private"
becomes "the strictness for the sake of strictness".
Thank you very much for 6.30.5 release.
The problem of writing and reading pointers to streamable objects under MSVS
2008 was fixed.
Igor

In the 6.31.04 development version the "public_data:" sections are changed to
be protected instead, unles building with OWL 5 compatibility.
Accessors are added and should be used instead of directly accessing the data
members.

Hi, I have a few reservations about this. There is a risk that we may reduce
the code quality, and actually introduce bugs, by providing accessors and
mutators indescriminately. Mutators in particular need special consideration.

I ran into a bug recently that was probably caused by the introduction of a
mutator; see "TPrinter::SetData leaks memory",
bug #3128107. In this case the mutator failed to manage ownership
(memory management). In general, a properly implemented mutator should always
uphold the class invariant (valid state).

This is of course also a problem with public access to internal data. But it
becomes worse when the class provides a broken interface to internal details.
Most programmers are wary of manipulating internal members, even, or
especially, if the members are public. The latter is a strong signal that the
class interface is poorly designed. On the other hand, a programmer expects
that a mutator interface is safe and upholds the class invariant, e.g.
TPrinter::SetData above.

There is also the question of the signature of accessors and mutators. For
example, TPrinter::GetData and SetData returns and receives a pointer. This is
most likely because the data is held internally by pointer. But this is an
implementation detail, and it makes no sense for GetData to return a null-
pointer or SetData to receive a null-pointer. TPrinter will always maintain
ownership of a TData instance in its valid state; i.e. a valid data member is
part of the class invariant. So there is a strong argument for the accessor
and mutator to use references instead.

Some members are truly internal implementation details and should have no
interface at all. There are probably many programs out there (heart surgeants)
that relies on access to truly internal details of OWL classes. For these we
should continue to provide a build option that provide public access; with the
warning that the innards may change and become private in the future, and that
we, for that reason, deprecate the build option. It makes no sense to provide
an interface (GetHeart and SetHeart) to these internal details. Even if the
accessors and mutators are implemented correctly they still violate the
encapsulation principle in good design.

So I suggest that public data members are categorized:

• part of the class interface; provide access via accessors and mutators; the latter only if strictly needed, and if so, ensure the implementation upholds the class invariant.

• truly internal; hide and don't add an interface. The programmer should either use a compatibility build option, or rewrite the code to not rely on internal details.

Regards,
Vidar Hasfjord

Hi, Vidar,

So I suggest that public data members are categorized:

• part of the class interface; provide access via accessors and mutators; the latter only if strictly needed, and if so, ensure the implementation upholds the class invariant.
• truly internal; hide and don't add an interface. The programmer should either use a compatibility build option, or rewrite the code to not rely on internal details.

I agree. As a start, I created a list of the public_data members of most OWL
classes:

TApplication
    HINSTANCE     hPrevInstance; 
    int           nCmdShow;
    TDocManager*  DocManager;
    TFrameWindow* MainWindow;
    TLangId       LangId;
    TTooltip*     Tooltip;

hPrevInstance - not used at all under 32-bit. There are Get and Set methods,
but the Set method makes no sense to be used.
nCmdShow - the value from WinMain() arguments. There are Get and Set methods,
but the Set method makes no sense to be used.
DocManager - There are Get and Set methods.
MainWindow - There are Get and Set methods. Cusrious thing is the the Set
method is protected.
LangId - There are Get and Set methods.
Tooltip - There are public Get and protected Set methods

TButton
    /// Indicates whether the button is to be considered the default push button. Used
    /// for owner-draw buttons, IsDefPB is set by a TButton constructor based on
    /// BS_DEFPUSHBUTTON style setting.
    bool IsDefPB;

Has public Get and Set methods.

TCheckBox
    /// If the check box belongs to a group box (a TGroupBox object), Group points to
    /// that object. If the check box is not part of a group, Group is zero.
    TGroupBox* Group;

Has public Get and Set methods.

TComboBox
      uint  TextLimit;  /// Maximum length of text in edit subcontrol

Has public Get and Set methods. The Set methos also sends CB_LIMITTEXT to the
window, so definitely it should be used

TDialog
      TDialogAttr  DialogAttr;

Has public Get and Set methods. Initialized in the constructors and used
during dialog creation - so maybe only the Get method is needed.

TDocManager
    /// Holds the list of attached documents, or 0 if no documents exist.
    TDocument::List       DocList;

Has public Get method

TDocument
    /// Holds a pointer to the application-defined data. Typically, you can use Tag to
    /// install a pointer to your own application's associated data structure. Tag,
    /// which is initialized to 0 at the time a TDocument object is constructed, is not
    /// otherwise used by the document view classes.
    void *     Tag;

Has public Get and Set methods

TView
    /// Application hook, not used internally
    /// Holds a pointer to the application-defined data. Typically, you can use Tag to
    /// install a pointer to your own application's associated data structure. TView
    /// zeros Tag during construction and does not access it again.
    void *    Tag;

Has public Get and Set methods

TEditFile
    /// Contains information about the user's file open or save selection.
    TOpenSaveDialog::TData FileData;

    /// Contains the name of the file being edited.
    LPTSTR FileName;

FileData - Has public Get and Set methods
FileName - Has public Get and Set methods. Also, it is better to replace it
with owl_string.

TEditSearch
/// The SearchData structure defines the search text string, the replacement text
/// string, and the size of the text buffer.
    TFindReplaceDialog::TData SearchData;

/// Contains find or replace dialog-box information (such as the text to find and
/// replace) and check box settings.
    TFindReplaceDialog*       SearchDialog;

/// Contains the search command identifier that opened the dialog box if one is open.
    uint                      SearchCmd;

All have public Get and Set methods

TFrameWindow
/// Indicates if keyboard navigation is required.
    bool            KeyboardHandling;

Has public Get and Set methods

TGadget
/// If Clip is false, clipping borders have not been established. If Clip is true,
/// the drawing for each gadget is restrained by the gadget's border.
    bool             Clip;

/// Initially set to false, WideAsPossible indicates whether the gadget width will
/// be adjusted by the gadget window to be as wide as possible in the remaining
/// space.
    bool             WideAsPossible;

Clip - No accessors. It is not used by TGadget, but by TGadgetWindow instead,
when painting the gadget
WideAsPossible - Has public Get and Set methods. It is not used by TGadget,
but by TGadgetWindow instead, to count how many gadgets want to be wide as
possible

TGroupBox
/// Flag that indicates whether parent is to be notified when the state of the group
/// box's selection boxes has changed. NotifyParent is true by default.
    bool  NotifyParent;

Has public Get and Set methods

TInputDialog
/// Points to the prompt for the input dialog box.
    _TCHAR * Prompt;

/// Pointer to the buffer that returns the text retrieved from the user. When passed
/// to the constructor of the input dialog box, contains the default text to be
/// initially displayed in the edit control.
    _TCHAR * Buffer;

/// Contains the size of the buffer that returns user input.
    int          BufferSize;

All three are initialized by TInputDialog constructors and have public Get
methods. Changing them after construction makes no sense

TListView
    int  DirtyFlag;   ///< flag to indicate if the view is dirty

VnIsDirty() returns DirtyFlag != 0. In some cases DirtyFlag is initialized
with true and false, but in others - with 2 (to detect our own close
notification). Should be hidden, and if needed provide SetDirtyFlag(bool)
which allows only true or false. The value 2 should be absolutely internal use
only.

TMDIClient
/// ClientAttr holds a pointer to a structure of the MDI client window's attributes.
    LPCLIENTCREATESTRUCT  ClientAttr;

Has Get and Set methods. The Set methods has comment: "Assumes ownership of
the pointer, so it will be deleted ? And the old one is orphaned and leaks?"

TScrollBar
    /// The number of range units to scroll the scroll bar when the user requests a
    /// small movement by clicking on the scroll bar's arrows. TScrollBar's constructor
    /// sets LineMagnitude to 1 by default. (The scroll range is 0-100 by default.)
    int  LineMagnitude;

    /// The number of range units to scroll the scroll bar when the user requests a
    /// large movement by clicking in the scroll bar's scrolling area. TScrollBar's
    /// constructor sets PageMagnitude to 10 by default. (The scroll range is 0-100 by
    /// default.)
    int  PageMagnitude;

Both have public Get and Set methods.

TStatic
/// Holds the size of the text buffer for static controls. Because of the null
/// terminator on the string, the number of characters that can be stored in the
/// static control is one less than  TextLimit.  TextLimit is also the number of
/// bytes transferred by the Transfer member function.
      uint  TextLimit;

Has public Get and Set methods

TCommandEnabler
/// Command ID for the enabled command.
    const uint  Id;

Initialized by constructor. Has public Get method. Set method makes no sense

TWindow
/// Holds the handle to the associated MS-Windows window, which you'll need to
/// access if you make calls directly to Windows API functions.
      HWND         Handle;

/// Points to the window's caption. When there is a valid HWindow, Title will yield
/// the same information as ::GetWindowText if you use TWindow::SetCaption to set
/// it.
    LPTSTR             Title;    // Logical title. Usually the same as window text

/// Points to the interface object that serves as the parent window for this interface object.
    TWindow*          Parent;   // Owl parent, use GetParentO(), SetParent()

/// Holds a TWindowAttr structure, which contains the window's creation attributes.
/// These attributes, which  include the window's style, extended style, position,
/// size, menu ID, child window ID, and menu accelerator table ID, are passed to the
/// function that creates the window.
    TWindowAttr       Attr;

/// Holds the address of the default window procedure. DefWindowProc calls
/// DefaultProc to process Windows messages that are not handled by the window.
    WNDPROC           DefaultProc;

/// Points to the scroller object that supports either the horizontal or vertical
/// scrolling for this window.
    TScroller*        Scroller; // Scrolling helper object

Handle - Has public Get method and protected Set which is to be used by
derived classes that create their window handle in a class-specific way.
Title - Has public GetCaption and SetCaption methods. SetCaption must be used
in order to set both the member and the actual window title
Parent - Has public Get and Set methods. SetParent() must be used in order to
keep the parent TWindow child list correct
Attr - Has public Get method. Used in window creation. Often some of it's
members are changes after the construction of the C++ object.
DefaultProc - Modified by the public SubclassWindowFunction(). Should not be
directly accessed.
Scroller - Has public Get method. Usualy instantiated by TWindow-derived
classes.

TXWindow
/// Points to the window object that is associated with the exception.
    TWindow*      Window;

Initialized by constructor. Has public Get method. Set method makes no sense

Great summary, Jogy! I think you've already identified some instances of the
issues I feared.

TMDIClient::SetClientAttr seems like it has an obvious bug (lack of memory
management, as commented). Is there any need for an interface to replace the
struct? The struct can be modified through the non-const pointer returned by
GetClientAttr. If so, the return type of the latter should be a reference
rather than a pointer; although that is a breaking change, of course.

TEditFile::FileData, TEditSearch::SearchData, TEditSearch::SearchDialog

Does the struct need replacement? If not, a non-const getter is more
appropriate for mutation.

TDialogAttr

Agree, I see only sense in a Get method. I see no need to replace the struct.
If mutation of the struct is required; then add a non-const overload for Get
returning a non-const reference.

MainWindow - There are Get and Set methods. Cusrious thing is the the Set
method is protected.

SetMainWindow is designed to be used in a derived class; i.e. in
InitMainWindow. Could there be a need to set up the main window from outside
the class; i.e. should it be public?

TDocument::Tag, TView::Tag

Get and Set are ok, but on another note, I'd like these to be deprecated.
These void tags encourage C-style programming style. Derived classes,
polymorphism and/or templates provide safer and more flexible code.

TEditFile::FileName - Has public Get and Set methods. Also, it is better to
replace it with owl_string.

Agree, same goes for TInputDialog::Prompt and Buffer, as well as
TWindow::Title. These should use owl_string.

I agree on the non-sense setters; we should remove (i.e. put in compatibility
mode) or deprecate these.