#77 Option to allow TEdit controls to be considered required fields

[Joe Slater]

There are times when an edit control needs to be considered a required field, such that the user must enter a value. I believe that Microsoft missed the boat on this one and should've had an ES_REQUIRED control style, but that's a mute point. There are a couple of ways I envision this being handled, either modifying the TEdit class or the TValidator class(es). I'll begin with the approach to simple modifications to the TEdit class:

TEdit approach

Add a protected data member to the class:

:::C++
bool Required;

Modify all of the constuctors to initialize it so as not to break existing applications:

:::C++
Required = false;

Add the following public member functions to the class:

:::C++
inline void SetRequired (const bool required = true) { Required = required; }
inline bool IsRequired (void) const { return (Required); }

Add the following protected member function to the class:

:::C++
bool MeetsRequirement (void);

Modifying the existing IsValid member function at the very beginning to have:

:::C++
if (!MeetsRequirement()) {
  if (reportError)
    MessageBox("You must enter a value for this field.", 
      "Required Field", 
      MB_OK | MB_ICONWARNING);
    return false;
}

Ideally the MessageBox above would load the string from a resource file, but for simplicity's sake I provided it as is.

The member function MeetsRequirement would be rather simple, beginning with:

:::C++
if (!IsRequired()) return (true);

The remaining portion of the function would simply examine the contents of the field (i.e., using GetText) and return true/false accordingly. However, there may be a debate as to whether a blank field is different from an empty field.

Consider an edit field that represents a name, where it would be appropriate to use a TFilterValidator that allows alphabetic characters and a blank such that the user could enter a first name, or a first and last name separated by a blank. Wanting this to be a required non-blank field wouldn't be handled by the above if MeetsRequirement returns true only for a non-empty field.

TValidator approach

The alternate approach would be to add this feature to the TValidator class, which is a bit more changes involved, but similar to the above. And it requires the programmer to make use of one of the TValidator classes, even if simply requirement is desired.

It would probably be more apt to use TValidatorOptions instead of bool, so that two distinct options could be added:

:::C++
voRequired = 0x0008,
voNonBlank = 0x0010,

The virtual IsValid member function for TValidator would check for these options and act accordingly. It would also require that most of the derived classes (e.g., TFilterValidator, TRangeValidator) that override the function to be modified to call the base class IsValid in addition to their own implementation.

There's also the issue of the virtual Error member function, for which the base class does nothing. And there may be other modifications needed, but this is just my initial examination. The above has not been implemented (i.e., tested); it is merely my suggestions for implementing this feature.

[Vidar Hasfjord]

I consider the TValidator approach to be the correct one here. If the user does not want to use validators, the conventional approach is to override CanClose to perform explicit validation. In this case, it is easy enough to add the checks for empty fields.

As for the TValidator approach, I think your ideas are sound. Another idea for dealing with blanks is to add options for stripping leading and trailing whitespace, e.g. voStripLeadingWhitespace and voStripTrailingWhitespace. With these options there would be no need to deal with blank non-empty fields, since a stripped field would either contain visible content or nothing.

[Joe Slater]

Ignoring the TEdit approach, and pursuing the TValidator approach probably is the best method, and I'll limit my discussions to such.

Using a voRequired TValidatorOptions feature should be simple enough to be performed by the base class TValidator. However, for applications that only need this type of validation, the TValidator class would need to be setup such that it can exist as a base class and a standalone class.

The handling of white space is a more complex issue.

Using your suggestion of voStripLeadingWhitespace and voStripTrailingWhitespace however, implies modifying not validating. In the interest of semantics, perhaps voNoLeadingWhitespace and voNoTrailingWhitespace might be better. Not all descendant classes would need to worry about these, such as TRangeValidator which won't allow blanks to be entered.

Disallowing leading white space should also be simple enough for Validator classes (ideally the base class) to simply not allow a blank to be entered at the start of the field as the data is entered (i.e., filtered out).

Disallowing trailing white space is not quite as simple, at least not in the sense as data is entered. Using my example of entering a first and last name separated by a blank: if trailing white space is filtered as I type, I would never be able to enter the last name because as soon as I type the first name and then a blank, the blank would be considered trailing white space and thusly removed. This type of filtering/modifying would need to be performed at CanClose and certain KillFocus times.

Ideally, this common functionality of handling white space would be performed by the base class, so the descendant classes need not duplication of effort dealing with it, including any user-defined classes not a part of OWL. Note that implementing just the leading white space filter would take care of my original concern of having non-blank fields. But I fear the complexities of trailing white space might be a bit more cumbersome (i.e., time-consuming) to implement, even though I do find it useful.

[Vidar Hasfjord]

Hi Joe,

Lot's of good comments there. Very helpful!

voRequired [...] should be simple enough to be performed by the base

I intuitively agree. To require a value sounds like a primary feature.

TValidator class would need to be [a] standalone class

It is currently not pure abstract, so making it useful standalone should only be good. A simple implementation in IsValid, and perhaps IsValidInput (see below), should do it, I guess. These currently just return true.

[Regarding voStripLeadingWhitespace and voStripTrailingWhitespace,] perhaps voNoLeadingWhitespace and voNoTrailingWhitespace might be better.

I fully agree, that better reflects the primary concern; validation.

However, on reflection, these options are not very useful for validation; they could easily frustrate a user (scratching her head after pasting a number, inadvertently followed by a space, into a form, and then being unable to commit the dialog due to an invisible issue). So, I now think your original voNonBlank is a better solution, since in this case the reason for validation failure is more obvious to the user.

Disallowing leading white space [by] simply not allow a blank to be entered

I am not a fan of filtering the input (IsValidInput). Restricting the input feels like the keyboard is failing to me. In my view, a better solution is to allow any input, but indicate validity of the field visually, e.g. by using a red frame around invalid fields. This could be an interesting feature request in its own right.

Disallowing trailing white space is not quite as simple [as the user types]

As you suggest, this is not an issue in IsValid. I propose we just ignore filtering.

Here's a simple off-the-cuff implementation of TValidator::IsValid with your suggested extension, voRequired and voNonBlank:

:::C++
bool TValidator::IsValid(LPCTSTR s)
{
  PRECONDITION(s);
  const auto isEmpty = *s == _T('\0');
  if (HasOption(voRequired) && isEmpty)
    return false;
  if (HasOption(voNonBlank) && !isEmpty)
  {
    auto is = tistringstream{s};
    is >> std::ws;
    CHECK(!is.fail());
    if (is.peek() == tistream::traits_type::eof())
      return false;
  }
  return true;
}

[Joe Slater]

Hi Vidar,

As a sidebar, I agree with your statements of filtering the input (IsValidInput) such that the keyboard feels like it is failing.

Pursuing the implementation using voRequired and voNonBlank I offer these comments…

Your off-the-cuff version of TValidator::IsValid looks okay. Since the direct child classes (i.e., TFilterValidator, TLookupValidator, and TPXPictureValidator) knew that the base class function did nothing, each of their IsValid functions will now need to be modified such that at the beginning of the function they have something of the nature:

:::C++
if (!TValidator::IsValid()) return false;

It would also be beneficial to add documentation to the release notes, that any user-defined child classes with TValidator as the parent would need to modify their IsValid function as noted above, to make use of this new feature.

There would also need to be changes made to TValidator::Error, but here we run into a new complexity that currently doesn't exist with any of the validator classes. Currently, each of the validators have only reason to fail; now, we have two additional ones. TValidator would need to know the reason why it is invalid to generate the appropriate error message. Here is a pseudo-code implementation of TValidator::Error:

void
TValidator::Error(TWindow* owner)
{
  PRECONDITION(owner);
  if (reason for error is required field)
    owner->MessageBox(IDS_VALREQUIRED, 0, MB_ICONEXCLAMATION|MB_OK);
  else if (reason for error is nonblank)
    owner->MessageBox(IDS_VALNONBLANK, 0, MB_ICONEXCLAMATION|MB_OK);
}

Since several of the descendent child classes (i.e., TRangeValidator, TFilterValidator, TStringLookupValidator, and TPXPictureValidator) knew that the parent class function did nothing, or had a preferred error message to generate, each of their Error functions will now (perhaps) need to be modified such that at the beginning of the function (after the PRECONDITION(owner); statement) they have something of the nature:

:::C++
if (!TValidator::IsValid()) {
  TValidator::Error(owner);
  return;
}

It would also be beneficial to add documentation to the release notes, that any user-defined validator child classes would (perhaps) need to modify their Error function as noted above, to make use of this new feature.

The file validate.rh would need to be changed with something of the nature:

:::C++
#define IDS_VALREQUIRED         32524
#define IDS_VALNONBLANK         32525

The file validate.rc would need to be changed (Japanese version would need appropriate ones too) with something of the nature:

:::C++
STRINGTABLE LOADONCALL MOVEABLE DISCARDABLE
{
  IDS_VALREQUIRED        "Value is required"
  IDS_VALNONBLANK        "Value must not be blank"
}

There may be some decendent validator class constructors that might set one or more of these TValidatorOptions as the default. For instance, I believe that TRangeValidator should set voRequired but not voNonBlank, as its own error message is better suited that the non blank error message.

[Vidar Hasfjord]

Hi Joe,

Great stuff! You are well on the way to an implementation of this feature. Do you want me to set you up as the owner (and implementer) of this ticket? I have already registered you as a developer so that you have write access to the code repository.

I will of course provide review.

TValidator would need [...] to generate the appropriate error message.

Ideally, yes. But the provision of fine-grained error messages is strictly an extension in its own right, so you could delegate that to a separate ticket. With the current single error message per class, you could just ensure that the error message provided by the descendant class is general enough to cover any reason for validation failure in the class and its bases.

Aside: Ideally, I would like a much cleaner validation solution. In the Owlet branch I've already removed Adjust and Transfer, which are orthogonal concerns unrelated to validation. I would also like to remove IsValidInput (mutation/filling), Error (reporting) and consequently Valid, as well as the global flags and related functions (which introduce coupling between base and descendants).

For the polymorphic interface, I just want a single function:

:::C++
using TValidationResult = std::pair<bool, tstring>; // valid, message
virtual auto Validate(const tstring&) const -> TValidationResult = 0;

[Joe Slater]

Hi Vidar,

I appreciate the comments, but no, I am not ready to take ownership of this ticket. You are doing a fine job and I still have a bit of catching up to do.

If TValidator is to exist as a standalone class as we discussed earlier for simple (TEdit) controls that wish to only be declared Required and/or Non-Blank as original stated with no need/desire for the child classes, then TValidator would need a simple Error function to describe the reason for the error. IsValid needs to remember the reason (class data member) so Error would know. Trying to generate a singe error message for two distinct reasons could be confusing to the user.

Whether descendant classes wish to call the base class Error routine is up to those classes, but for some (not all) it would be useful (and easy).

Right?

[Vidar Hasfjord]

Hi Joe, I have now implemented my mentioned simplified validator scheme for Owlet. See [r2818] and related revisions (history). Your requested feature should be particularly simple to implement with this cleaner validator design, since validating a multitude of requirements within a single validator is straight-forward. In particular, see my new TRangeValidator implementation. Adding a restriction for empty/blank fields here is a piece of cake. You can test it out with the accompanying example ("examples/validate").

[Joe Slater]

Interesting Vidar!

I had been toying with developing a robust Regular Expression class with a validator class based on this class, from which almost any child validator class (integers, floating points, lookup strings, picture strings, etc.) could easily implement. But that's another story…

[Vidar Hasfjord]

Hi Joe,

I am not ready to take ownership of this ticket. You are doing a fine job

I think you misunderstand. I wasn't offering that you replace me as the owner; this request currently has no owner. While I think the request has merits, and find the design aspects interesting, I am not ready to take it on. So, if you want to see it implemented, I encourage you to take ownership.

TValidator would need a simple Error function to describe the reason for the error. IsValid needs to remember the reason (class data member) so Error would know.

Yes, the current over-complicated validator design makes the extension harder than it should be. I don't even want to think about the implications for the input coercion functionality (IsInputValid).

I have invested some effort into greatly simplifying matters in my Owlet branch. In the long run, I may promote that our mainline (trunk) adopts this simpler design, but for now I think it is a step too far with regard to backwards compatibility.

If TValidator is to exist as a standalone class

On reflection, I've come to view this as a poor solution. Adding functionality by inheritance is generally not a good idea (coupling etc.). TValidator only needs polymorphism from the point of view of TEdit; the only client using validators in a polymorphic manner. The much simpler pure abstract interface in my Owlet branch is sufficient for this. Any reusable functionality, such as tests for empty and blank fields, can be implemented simply as free functions, or by delegation (one validator using another).

I had been toying with developing a robust Regular Expression [validator]

Interesting. It should be quite simple to implement with std::regex. I guess the validator interface would look much like TPictureValidator.

[Joe Slater]

Hi Vidar,

You are correct; I did misunderstand. In light of such, yes, please set me up as the owner and implementer. I have almost all of the changes done locally.

Having said that, I need help with essentially:

1. How do I make the changes? I've never used SourceForge in that manner before.
2. How do I make the documentation changes (Release Notes and Usage of TEdit)?
3. How do I get the Japanese version of the error messages? This is the only change I do not have yet.

After extensive consideration of our rather detailed discussion, I have come to the following conclusion.

This is a simple request, and the implementation should be simple. It now is.

Validation is solely a feature of TEdit, not TValidator. TValidator is a support class available solely for TEdit.

Therefore, this feature belongs to TEdit. Attempting to implement in TValidator as we have seen, is tedious within its current design, especially maintaining 100% backward compatibility. Besides, the TValidator classes are really meant to validate actual input. This new feature really only makes sense when used with TFilterValidator (and thusly TRangeValidator) or with no TValidator at all. The other TValidator classes really have no use for this new feature, as they will probably already fail.

Furthermore, the voRequired and voNonBlank options are not mutually exclusive. It's really a question of whether all blanks are allowed when requirement is declared.

So, my latest implementation is simple, and 100% backwards compatible (as new features essentially should be). It will not break nor require changes to existing applications. I have tested the logic, but not the implementation of course.

Since I am to make the changes and you have offered to review the changes, I will not post this latest implementation here unless you request it.

[Vidar Hasfjord]

Hi Joe,

Welcome aboard! Could you please update this ticket yourself, setting yourself up as the owner? If you aren't allowed to, let me know.

As for the milestone, I propose you target version 7. Version 6.40 is stabilising, and I am reluctant to make a major change like this to a release branch.

How do I make the changes? I've never used SourceForge in that manner before.

To work with the code repository you need a Subversion client; I use and recommend TortoiseSVN. Then do a checkout of the "trunk" of the repository, as described in our installation guide. Make your changes to this working copy, and when completed and tested, commit your changes to the repository. For more information about the Subversion workflow, read the excellent TortoiseSVN documentation or search the web.

Also, carefully read and follow our Coding Standards, especially "Patches and version control". For examples of our logging style, see the code log, especially for revisions referencing tickets.

How do I make the documentation changes (Release Notes and Usage of TEdit)?

Now that you have developer rights you can make changes to our wiki, where you can edit release notes, add articles in the Knowledge Base, etc.

How do I get the Japanese version of the error messages?

My only advice is to ask for help in the forum. That said, the Japanese translation has not been maintained in a long time (if ever), so in the worst case, just use the English string for now (I don't think you even need to duplicate it in the resource file, since I think the resource loading functions will use the default language as a fallback, but you should test this).

Since I am to make the changes and you have offered to review the changes, I will not post this latest implementation here unless you request it.

If you want feedback on code before you commit it (esp. with regard to design decisions), TortoiseSVN allows you to very simply create a patch file containing your changes, which you can e.g. post here as an attachment. Just as easily, TortoiseSVN can apply a patch to a working copy of the code, for simple review.

Validation is solely a feature of TEdit, not TValidator. TValidator is a support class available solely for TEdit. Therefore, this feature belongs to TEdit.

Sorry, but I don't find that line of reasoning convincing. Currently, TValidator is the mechanism for validation. Essentially, TEdit delegates validation to an abstract helper. So, I think your solution should follow suit.

Attempting to implement in TValidator as we have seen, is tedious within its current design

It is. But, as discussed, the complication is related to input coercion (IsInputValid), which I recommend you ignore. I.e. test for blank and empty fields only when the field is committed (IsValid). Then the only complicating issue, I think, is the need to communicate the reason for failure from IsValid to Error (due to the current lack of support for multiple reasons for validation failure). But this can be done by setting some private error state. Then amend Error (and its documentation) to report the most recent validation failure.

The remaining open design question is whether to implement the functionality by extending the TValidator base class (as discussed), or by delegating to new stand-alone validators (my preference, I think).

For example, here is how delegation might look:

:::C++
bool TRangeValidator::IsValid(LPCTSTR s)
{
  PRECONDITION(s);
  IsEmpty = !NonEmptyValidator.IsValid(s);
  IsBlank = !NonBlankValidator.IsValid(s);
  if (IsRequired && (IsEmpty || IsBlank)) return false;
  // ...Test range, setting IsOutsideRange on failure...
}

void TRangeValidator::Error(TWindow* owner)
{
  PRECONTITION(owner);
  if (IsRequired && IsEmpty) return NonEmptyValidator.Error(owner);
  if (IsRequired && IsBlank) return NonBlankValidator.Error(owner);
  if (IsOutsideRange) // ...
  // ...
}

[Joe Slater]

Hi Vidar,

I am setup as the owner; no problem. I set the milestone as v7; no problem. I have downloaded/installed TurtoiseSVN; no problem. I did a checkout of 640, which downloaded everything; no problem. I made the local changes to the appropriate files; no problem (of course). When I perform a commit using TurtoiseSVN, it detects the changed files correctly, but I get an "Authorization failed" error message.

Am I doing something wrong?

[Joe Slater]

Hi again Vidar!

It is my belief that TEdit ultimately is the implementer of validation, hence the function such as IsValid.

It is also my belief that the TValidator classes are designed to validate actual content, not the existence of content. While TEdit currently delegates the validation of content to Validator classes, it doesn't mean it can't be responsible for validating the existence of content.

My design breaks nothing. All existing TValidator classes, including child classes created by the user community, continue to work as designed with no changes required. All existing TEdit classes, including child classes created by the user community, continue to work as designed with no changes required.

If one wants to make use of this new feature, one merely has to make one call to set the requirement once the TEdit (or child) class has been instantiated.

Using the TValidator delegation method you suggested above, would not only require the application developer to set the requirement once the TValidator (or child) has been instantiated, it would also require design changes to most all TValidator classes to perform that delegation, including those child classes (perhaps) of TValidator created by the user community. If the application developer depends upon an OWL extension library created by a third party (e.g., TRangeFloatValidator), then the developer is up a creek without a paddle.

Attempting to implement this simple feature in TValidator, is just too convoluted and requires too many additional changes by the user community for me to justify it.

[Vidar Hasfjord]

I did a checkout of 640

Note that you should not commit to release branches. Development should happen on the trunk (see our Coding Standards).

I get an "Authorization failed" error message

You need to provide user authorisation and the "svn+ssh:" protocol to check out a working copy with write-access. The correct Subversion command is provided in the Code section by pressing the RW command (rather than the RO button).

svn checkout --username=joeslatersf svn+ssh://joeslatersf@svn.code.sf.net/p/owlnext/code/trunk C:\OWLNext

[Vidar Hasfjord]

My previous reply referred to the Subversion command line tools. For TortoiseSVN, use the simpler "https:" address to the repository. See SourceForge Documentation.

https://svn.code.sf.net/p/owlnext/code/trunk

[Joe Slater]

Changing svn: to https: took care of the problem, Vidar. Thank you.

The changes should now appear at: [r2864]

Please note that DIFF shows several changes to edit.cpp that wasn't intentional; I suspect that my editor stripped off trailing white space at these several locations. If that's a problem, please let me know. The only true effective changes were a single statement added to all constructors initializing Requirement, simple modifications to the member function IsValid, and the addition of the member function MeetsRequirement.

[Joe Slater]

Just as a final addendum, I verified that the several inadvertant changes to edit.cpp [r2864] did indeed deal with removal of trailing white space. To repeat myself myself, please let me know if that's a problem (I still have an old copy of edit.cpp).

Thanks again for all your help Vidar.

[Joe Slater]

Hi Vidar,

For completeness, I'd really like to set the appropriate Japanese error message text properly (even if nobody uses it anymore). I discovered http://translate.google.com/ which gave me this to use to put in the string table found in validate.rc:

:::C++
  IDS_VALREQUIRED        "入力が必要です"
  IDS_VALNONBLANK        "非ブランクの入力が必要です"

But that is Unicode, and the file validate.rc is not a Unicode text file. I also noticied that in examining DIFF for validate.rc [r2864], it converts and displays Unicode similar to the above. Any idea how to convert Unicode to the "stuff" that validate.rc contains?

[Vidar Hasfjord]

Hi Joe,

Kudos for taking ownership of this ticket and providing a solution! Except for a few nitpicks noted below, your solution looks neat and simple.

Nitpicks (see Coding Standards):

• Commit to the trunk, not release branches.
• Types should have "T" prefix (ref. RequireOptions).
• enum type names should be singular (i.e. TRequireOption).
• Don't use top-level const on parameters (ref. SetRequired, SetRequiredOption), at least not in the declaration of the function.
• IsAllBlank should be declared static, since it does not use this.
• Prefer member initializers to assignment/setter calls in constructors.
• Always include an empty line before comment blocks (ref. Requirement).
• Use a verbatim copy of the ticket title in your commit log message.
• Use the conventional "TODO:" tag in the source for outstanding issues (i.e. "TODO: Need Japanese translation!").

And a suggestion:

• Simplify enum names; no need to repeat "Required"; i.e. {roNone, roNonEmpty, roNonBlank}.

Regarding the design, I have only two comments:

• Having all of SetRequired, SetNotRequired and SetRequiredOption is confusing. In particular, SetRequired (false) is easily mistaken to mean SetNotRequired. It seems that a single function SetRequired (TRequireOption) should suffice. Then, for symmetry, rename GetRequiredOption to GetRequired.
• MeetsRequirement and IsAllBlank seem to be internal helpers only. If so, there is no need to expose them in the header. Move them to the implementation file as free functions; add any necessary parameters, add a trailing underscore to their names and put them in an anonymous namespace (ref. Coding Standards).

My design breaks nothing.

I guess that is the clincher argument. Admittedly, I was reluctant to take this ticket on due to risk of regressions. However, I plan to explore an alternative design on the Owlet branch.

my editor stripped off trailing white space

Unfortunately, whitespace changes like that can easily happen. Luckily, TortoiseMerge has an option to ignore whitespace changes in the viewer, so it wasn't an issue for me.

Tip: Always thoroughly inspect your changes in TortoiseMerge before you commit, e.g. using the command "Compare with base" in the Commit dialog, or more safely, using the commands "SVN Diff" and "SVN Check for modifications" from Windows Explorer. If you spot unintended changes, you can undo them directly in the viewer (commands "Use other text block" etc. are helpful for this purpose). If you spot unrelated changes that you want to commit separately, then you can temporarily undo them in the Commit dialog by using the "Restore after commit" command. Then undo these changes in TortoiseMerge before you commit. After the commit, TortoiseSVN will restore the file so that you can commit the unintended changes separately, if appropriate.

[Regarding Japanese text,] the file validate.rc is not a Unicode text file

Actually, the Microsoft resource compiler supports Unicode input files, so we could convert our resource files. See Unires Sample and search the web.

PS. Note that I have moved your changes over to the trunk. Please continue any further work on the trunk. I.e. checkout the following repository path:

https://svn.code.sf.net/p/owlnext/code/trunk

[Joe Slater]

Hi Vidar,

I appreciate very much your patience with a newbie (i.e., me). Checking out with SourceForge, using TortoiseSVN, using the OWLNext coding standards (as opposed to my own or those still in OWL existence), … has been a bit overwhelming. And I knew to make changes to the trunk, I just didn't know I wasn't.

My first question is: Can we just delete the release 2864 and I start over? Or should I issue another release (kind of unnecessary clutter would result)?

I have newer (local) changes to submit with regard to your nitpicks and suggestions. In short, I have performed them all except that I just trashed IsAllBlanks and MeetsRequirement; that logic is simple enough to just implement in IsValid (with comments) and not needed anywhere else.

And I figured out the Japanese "stuff" in validate.rc so it need not be a Unicode text file. It is multi-byte characters (I've never used that before). Using the WinAPI WideCharToMultiByte with Code Page 932 gave me the desired results. So that is no longer a TODO: item.

Rather than screw-up again I am going to post the code here…

The changes to validate.rh and validate.rc are no different other than originally posted except that I have the proper Japanese new error messages.

Additions to the (original) edit.h:

:::C++
    /// \name Requirement type and functions
    //@{
    enum TRequireOption { roNone, roNonEmpty, roNonBlank };
    void SetNotRequired();
    void SetRequired(TRequireOption option = roNonBlank);
    TRequireOption GetRequired() const;
    bool IsRequired() const;
    //@}

I added a blank line before the comment and declaration in the protected_data section for Requirement:

:::C++

    /// Input requirement setting
    //
    TRequireOption Requirement;

The inline implementations for each of the above member functions are also present, modified only to satisfy the above changes:

:::C++
//
/// Sets the requirement option for the edit control.
//
inline void TEdit::SetRequired(TEdit::TRequireOption option)
{
  Requirement = option;
}

//
/// Sets the requirement option for the edit control to reflect a value
/// is not required.
//
inline void TEdit::SetNotRequired()
{
  SetRequired(roNone);
}

//
/// Returns the requirement option for the edit control.
//
inline TEdit::TRequireOption TEdit::GetRequired() const
{
  return Requirement;
}

//
/// Returns true if a value is required for the edit control.
//
inline bool TEdit::IsRequired() const
{
  return GetRequired() != roNone;
}

The changes to edit.cpp include modifying the constructors to no longer have SetNotRequired in them, but use member initializers (but I left the initialization of Validator as it has been which is an assignment statement — I can change this too if you like). Here is one example of a change to a constructor (the others are of course similar):

:::C++
TEdit::TEdit(THandle hWnd, TModule* module)
: TStatic(hWnd, module), Requirement(roNone)
{
  EnableTransfer();
  Validator = 0;
}

And the remaining change to edit.cpp is simply the IsValid member function (with comment additions/changes):

:::C++
//
/// Always returns true if the TEdit object does not have an associated TValidator
/// object (i.e. if TEdit.Validator == 0) and it is not required.
/// If the edit control is required, IsValid calls MeetsRequirement.
/// If the edit control has a validator, and the reportError parameter is set to true,
/// then IsValid calls the validator's Valid method. If the reportError parameter is
/// false, IsValid calls the validator's IsValid method.
//
bool
TEdit::IsValid(bool reportError)
{
  bool valid = true;

  //
  /// Perform validation if needed.
  //
  if (Validator || IsRequired()) {

    //
    /// Lock the buffer to perform validation.
    //
    LPTSTR buffer = LockBuffer();

    //
    /// If requirement option set, check if requirement met.
    //
    if (IsRequired()) {
      bool reqNonBlank = GetRequired() == roNonBlank;
      valid = *buffer;
      if (valid && reqNonBlank)
        valid = _tcsspn(buffer, _T(" \t\n\v\f\r\xA0")) != _tcslen(buffer);
      if (!valid && reportError)
        MessageBox(reqNonBlank ? IDS_VALNONBLANK : IDS_VALREQUIRED, 0, MB_ICONEXCLAMATION|MB_OK);
    }

    //
    /// If still valid and validator present, have it validate.
    //
    if (valid && Validator)
      valid = reportError ? Validator->Valid(buffer, this) : Validator->IsValid(buffer);

    //
    /// Unlock the buffer
    //
    UnlockBuffer(buffer);
  }
  return valid;
}

At your convenience, let me know how/whether to proceed with this. Thank you!

[Vidar Hasfjord]

Hi Joe,

I appreciate very much your patience with a newbie

I am happy to help and welcome your interest in the project. There is a little bit of self-interest here for me: One of the main reasons for getting involved with OWLNext was to learn and gain experience with collaborative open-source development, and that requires more than one to play. :-)

I knew to make changes to the trunk, I just didn't know I wasn't.

You must check out the correct directory in the code repository. The repository is arranged in the conventional manner with top-level directories "trunk", "branches" and "tags". The Subversion documentation explains the purpose of each directory. You can familiarise yourself with the structure of the repository in the Code section.

Click History to inspect the revision log. You can click History within each directory to only get the relevant changes for that part of the repository, down to each file. TortoiseSVN has similar commands for browsing the repository and the revision log.

Can we just delete the release 2864 and I start over?

No need. I merged your changes over to the trunk (see [r2865]) and then undid your changes on the 6.40 release branch (see [r2866]). You can now continue development on the trunk. Just check out a new fresh working copy using the correct "https:" address to the trunk.

By the way, you can have multiple independent working copies checked out for different directories in the repository. To prevent you from committing changes on the release branches by mistake, I recommend you use the read-only "svn:" address for those. For example, here are my current working copies, with the recommended checkout path in parentheses:

C:\OWLNext\branch-6.34 (svn://svn.code.sf.net/p/owlnext/code/branches/634)
C:\OWLNext\branch-6.40 (svn://svn.code.sf.net/p/owlnext/code/branches/640)
C:\OWLNext\branch-owlet (svn://svn.code.sf.net/p/owlnext/code/branches/owlet)
C:\OWLNext\trunk (https://svn.code.sf.net/p/owlnext/code/trunk)

If this is still unclear, I recommend you go through a Subversion tutorial and read up on the documentation. It should be well worth your while. Then, by all means, ask for clarification of any remaining issues, and I'll try to help.

I have newer (local) changes to submit with regard to your nitpicks and suggestions.

Your changes look good. Just commit your changes to the trunk when you feel ready.

Nitpicks:

• Do not use Doxygen-style comments (///) within functions. Use OWL-style code comments, and note that, within functions, these do not have the empty "//" leading line, just a trailing empty "//" connecting with the code. E.g. see TEdit::Search.
• Opening curly brace should be on its own line (the OWL code is inconsistent, I know, but this is the style going forward).
• Use spaces around operators (MB_OK | MB_ICONEXCLAMATION).

I figured out the Japanese "stuff"

Great! Good work.

I left the initialization of Validator as it has been which is an assignment statement — I can change this too if you like

Please do. That would be an improvement. But commit it as a separate revision.

TEdit::IsValid

I now noticed something I didn't before; TEdit::IsValid is not exception safe. In particular, it calls LockBuffer, but fails to call UnlockBuffer, in case an exception is thrown in the intermediate code. This is not an issue introduced by your changes, so I suggest it is treated separately, i.e. create a bug ticket.

[Joe Slater]

Hi Vidar,

I have your latest nitpicks performed (OWL-style comments within functions, opening curly brace on its own line, spaces around operators). And I'll take care of Validator initialization in the constructors in a separate revision. Before trunk check-out and commit however, I noticed one more thing which may need attention…

I am still somewhat old-school and do not use streams often, but I am aware of them. There are two stream support member functions in TEdit, Read and Write. Don't these need to be modified to stream the new data member Requirement and Read perhaps check the version to see if it is even present? I'll make the necessary changes, but I need your help here if anything is to be done.

[Vidar Hasfjord]

Don't these need to be modified to stream the new data member Requirement

Ideally, yes, but I don't know if it has much value in practice. See section "Persistent streams" in my article "Replacing the Borland C++ Libraries".

PS: Note that, in the Owlet branch, I have removed the Persistent Streams altogether, and it is my long term vision to do so on the trunk as well, perhaps already for version 7.

Read [should] perhaps check the version to see if [member Requirement] is even present?

Yes, if you want to take it on, you should make sure that old streams will read OK. Other than that, just a conversion to a streamable type, e.g. int, is needed, as far as I can see.

:::C++
// Write
//
os << static_cast<int>(GetObject()->Requirement);

// Read
//
if (version >= 2)
{
  int r = 0; 
  is >> r;
  GetObject()->Requirement = static_cast<TEdit::TRequireOption>(r);
}
else
{
  GetObject()->Requirement = TEdit::roNone;
}

[Joe Slater]

As you might've guessed, I don't like partial implementations. Otherwise, I wouldn't have spent two days trying to figure out the Japanese "stuff" which probably nobody uses anymore. And I agree that this streaming probably isn't useful/used. Nonetheless, I understand your code except for the version number of 2… I don't know where that even gets set (I don't use this feature). But I trust your judgment. And just to be clear, as streaming order is important, I'm posting my proposed changes here for your review before committing it:

:::C++

//
/// reads an instance of TEdit from the given ipstream
//
void*
TEdit::Streamer::Read(ipstream& is, uint32 version) const
{
  ReadBaseObject((TStatic*)GetObject(), is);
  is >> GetObject()->Validator;
  if (version < 2)
    GetObject()->Requirement = TEdit::roNone;
  else
  {
    int requirement = static_cast<int>(GetObject()->roNone);
    is >> requirement;
    GetObject()->Requirement = static_cast<TEdit::TRequireOption>(requirement);
  }
  return GetObject();
}

//
/// writes the TEdit to the given opstream
//
void
TEdit::Streamer::Write(opstream& os) const
{
  WriteBaseObject((TStatic*)GetObject(), os);
  os << GetObject()->Validator;
  os << static_cast<int>(GetObject()->Requirement);
}