#201 Add helper methods for reading and writing boolean settings in TProfile

[Ognyan Chernokozhev]

It will be helpful if TProfile supports GetBool and WriteBool methods that have similar functionality to the ones in TConfigFile. The GetBool method should process values like "yes", "no", "true", "false", "1", "0".

[Vidar Hasfjord]

Hi Ognyan,

I have reviewed [r5898], and here are some comments:

TProfile::GetBool uses a small buffer (20) for the string value. Although that is plenty for the currently defined strings for representing a bool ("yes", "no", "true", "false", etc.), it is a bit arbitrary assumption. And C-style buffers are ugly. For nicer, simpler and more robust code you can use the GetString overload returning a tstring instead.

I would like to merge this feature to Owlet, but it unfortunately depends on TConfigFile, which is not included in Owlet. It would be nice to make it independent. The string tests performed by TConfigFile::StringToBool can be made in just a couple of lines of code, so avoiding the dependency should be simple.

auto TProfile::GetBool(LPCTSTR key, bool defaultBool) -> bool
{
  PRECONDITION(key);
  const auto stringToBool = [&](const tstring& s) { ... };
  return stringToBool(GetString(key));
}

Also note that to_tstring(boolVal) can be used instead of TConfigFile::BoolToString.

I've implemented these suggestions in [r5899], and hence reverted your change to TConfigFile (previously protected members made public). I hope you don't mind.

Tip: For short overloads implemented inline in the class definition, add ///< \overload after the definition to generate standard Doxygen documentation for overloads.

[Ognyan Chernokozhev]

Hi,

The problem is that even though the code is small, it is still a duplicated code. And if a change is made (like eliminating the "on" and "off" values, or adding more values - internationalization maybe?), then it has to be done in both places.

Would not it be better to put the definition of a stringToBool function in a common header like def.h and the implementation in a suitable common cpp file (or add a new one)?

Also, it got me thinking of allowing the users to supply their own implementation, for example to support different languages - can be done by adding the function as a parameter to the GetBool/WriteBool functions and having it default to the built-in implementation.

[Ognyan Chernokozhev]

Also, there is a bunch of other conversion functions in TConfigFile, like Read/WriteDate, Read/WriteFont, Read/WriteTime, etc.

All of them use some logic of converting the object to a string on a write, and then parsing it back on a read.

All this logic can be put in a common file and used by both TConfigFile and TProfile, and user code as well.

[Vidar Hasfjord]

By the way, I am not sure it is a good idea to allow both "on" and "no" as valid Boolean values, as either is easily a misspelling of the other, but have the opposite value.

[State of the world]
Still_blue_and_green = yes
Rapid_global_heating = yes
Peaceful_coexistance = no
All_devastating_wars = yes
Nuclear_annihilation = on

Spelling error or human demise?

Perhaps generating an exception (or diagnostic message at least) for invalid values would be a good idea.

[Vidar Hasfjord]

@jogybl wrote:

The problem is that even though the code is small, it is still a duplicated code.

The nice thing, though — from a perspective of coupling and backwards compatibility — it doesn't have to be! Keeping them separate means that we can make different (e.g. safer) choices for TProfile, while we keep TConfigFile compatible for existing users.

By the way, TConfigFile is an addition to OWL, that in my view belongs in OWLExt. Since I have no use for it in my applications, I removed it in Owlet. TProfile is the core OWL encapsulation around the Windows system's use of "profile" files.

Would not it be better to put the definition of a stringToBool function in a common header [...]?

That would be a good idea for code reuse. But I don't think we should necessarily duplicate and "globalise" the TConfigFile behaviour. In my view, TProfile should be kept simple and use (de-facto) standard and non-surprising interpretation of profile files. What does Windows use for Boolean values? 0 and 1?

The simplest (standard compliant) string-to-bool conversion can be coded in a single short line of code (a.k.a. lexical cast via standard streams):

auto boolVal; tistringstream(str) >> boolVal;

(Note that std::boolalpha can be used here to parse "true" and "false" instead of "0" and "1".)

Perhaps it would be nice to have an inline function for this conversion, but I don't think it is crucial, since application requirements may likely differ from the provided behaviour.

Also, there is a bunch of other conversion functions in TConfigFile, like Read/WriteDate, Read/WriteFont, Read/WriteTime, etc.

It may make sense to adopt some of the functionality, for added convenience. However, I still think TProfile should be independent.

In my view, we should move TConfigFile to OWLExt as an optional component that can be used for those who need more than TProfile, then make TProfile powerful and nice enough for basic needs. I don't think it makes sense to have both in the OWLNext core.

To avoid code duplication, perhaps we should make TConfigFile use (inherit) TProfile functionality, then migrate the good stuff to TProfile? Public static conversion functions in TProfile or free functions in the owl namespace may make sense in this regard, if wholesale inheritance isn't an option. And the conversion functions may be useful to users in general as well, as you suggest.

[Ognyan Chernokozhev]

By the way, TConfigFile is an addition to OWL, that in my view belongs in OWLExt. Since I have no use for it in my applications, I removed it in Owlet. TProfile is the core OWL encapsulation around the Windows system's use of "profile" files.

That is true, I checked and saw it was added in OWLNext 6.07.
But it is used in TMemComboBox, so we will have to either rewrite it to use TProfile or move it to OWLExt as well.

TConfigFile provides a nice common interface that can be used for writing to either ini files or registry and easy switching between both. TProfile does not have that flexibility.

To avoid code duplication, perhaps we should make TConfigFile use (inherit) TProfile functionality, then migrate the good stuff to TProfile? Public static conversion functions in TProfile or free functions in the owl namespace may make sense in this regard, if wholesale inheritance isn't an option. And the conversion functions may be useful to users in general as well, as you suggest.

If we make the conversion functions public and easily accessible, then as a matter of fact there will not be need for all those specific conversion methods.

You could use something like

profile.WriteString("StartTime", timeToString(startTime));
startTime = stringToTime(profile.GetString("StartTime"), defaultStartTime);

instead of having to implement profile.WriteTime and profile.GetTime.

We can also keep the existing TConfigFile conversion functions for backward compatibility but mark them as deprecated and point out to the new usage.

[Vidar Hasfjord]

@jogybl wrote:

But it is used in TMemComboBox, so we will have to either rewrite it to use TProfile or move it to OWLExt as well.

Yes, in Owlet, TMemComboBox got the chop as well. Let's move it to OWLExt!

TConfigFile provides a nice common interface that can be used for writing to either ini files or registry and easy switching between both. TProfile does not have that flexibility.

True, but not needed in OWLNext/Owlet. Fantastic for OWLExt though!

If we make the conversion functions public and easily accessible, then as a matter of fact there will not be need for all those specific conversion methods.

Very good point! Less coupling and better composability! The software design teachers would be very happy with us! :-)

It is a little less convenient for the common use case e.g. in TProfile, though. So having both is ideal, I guess, provided that we are talking about common use. Convenience in the very rare case may not be worth the code maintenance burden.

We can also keep the existing TConfigFile conversion functions for backward compatibility but mark them as deprecated and point out to the new usage.

My take: For simplicity and backward compatibility just keep TConfigFile as is, put it in OWLExt and let any users decide its future. Then let's make TProfile do what we need and want in the OWLNext core.

[Ognyan Chernokozhev]

Yes, in Owlet, TMemComboBox got the chop as well. Let's move it to OWLExt!

I agree, TMemComboBox is an OWLNext addition that does not really belong to the core library.

It is a little less convenient for the common use case e.g. in TProfile, though. So having both is ideal, I guess, provided that we are talking about common use. Convenience in the very rare case may not be worth the code maintenance burden.

Yes, convenience overrides in TProfile like GetBool/WriteBool can then be inlines that simply call the global conversion functions.

[Vidar Hasfjord]

@jogybl wrote:

I agree [to moving TConfigFile and TMemComboBox to OWLExt]

Great! Since you agree, and unless and until I get any objections, I will move TConfigFile and dependants (TMemComboBox) to OWLExt.

Regarding conversion functions, implementing lexical conversions to and from standard streams (ala boost::lexical_cast) will allow conversion for any type that has stream operator overloads. I used lexical conversions in the implementation of Dialog Data Transfer (see TransferDlgItemText).

We also now have very convenient conversion to tstring with the to_tstring overloads.

Such features can allow very flexible template solutions. For example:

template <class K, class V>
void TProfile::Write(const K& key, const V& value, const V& def = V{})
{ WriteString(key, to_tstring(value), to_tstring(def)); }

PS. All error handling should be converted to exceptions, and output parameters (buffers) eliminated and converted to return values.