String system upgrade.

[adf88]

String system is not perfect:

• Mysterious SetDParam calls with magic parameter index.
• "{SKIP}{SKIP}{..." chains. Parameters of two strings can't overlap when used in one window.
• Parameters can't be assigned once for ever to strings.
• Need to call SetDParam for every parameter in OnPaint.
• Tips and "query window" title can't have parameters etc.
• Weak memory management, processor wasting (every visible string is built every paint call).

I have an idea how to upgrade string system without loosing backward compatibility.
The point is to make class for string.
Class that allows to assign parameters to strings and manages string memory: reference counting, build string only when there is a need to, map all strings that do not change (i.e. labels like "OK") to have only one buffer for one string.

There will be no need to modify a lot of code to make it work. Every GUI window can stay in its old form until it's upgraded (well it doesn't need to be upgraded at all). There is a few places that will need to be changed: GetString calls, string drawing functions, "query string" framework and Window class little modifications.

If you like it I would make a pre-implementation to show other, numerous and smaller ides that I didn't include here. Further work may be common.

It could be something like this:


Possible usage (when I write "reference" I mean reference in general concept, it may be C++ reference or pointer as well):

• static strings (parameterless), parametrized strings

  Code: Select all

  static const Widget _some_window_widgets[] = {
  /*          type, ...,  data (changed type to WidgetData),    tooltips (changed type to String)    */
  {   WWT_CLOSEBOX, ...,  String::Static(STR_BLACK_CROSS),         String::Static(STR_TOOLTIP_CLOSE_WINDOW)}, 
  {    WWT_CAPTION, ...,  String::Static(STR_SOME_WINDOW_CAPTION), String::Null},
  
  {      WWT_LABEL, ...,  STR_NULL,                             String::Static(STR_TOOLTIP_PLAYER_NAME)}, //WIDGET_NAME,      
  {       WWT_TEXT, ...,  STR_NULL,                             String::Static(STR_TOOLTIP_OPPONENTS)},   //WIDGET_OPPONENTS
  {       WWT_TEXT, ...,  STR_NULL,                             String::Static(STR_TOOLTIP_STATS)},       //WIDGET_STATS
  //...
  
  class SomeWindow : public Window
  {
     int statA;
     int statB;
     char statC[255];
     
     SomeWindow(const char *player_name, byte opponents, ...) : Window(...)
     {   
         this->widget[WIDGET_NAME].data = ParamStr(
                    STR_PLAYER_NAME, //"Name: {STRING}"
                    ConstStrParam(player_name)); /*const string parameter, 
                    * memory pointed by player_name do not changes while string exists */
  
         this->widget[WIDGET_OPPONENTS].data = ParamStr(
                    STR_OPPONENTS_COUNT, //"Opponents: {NUM}"
                    IntParam(opponents); //integer parameter (any integer type)
         
         this->widget[WIDGET_STATS].data = ParamStr(
                    STR_PLAYER_STATS, //"A: {COMMA}, B: {NUM}, C: {STRING}"
                    RefParam(&statA), RefParam(&statB), //reference (pointer) to integer
                    StringParam(statC); //var string
     }
     
     virtual OnPaint()
     {
        this->DrawWidgets(); //nothig more needed !
     }
  //...              

• parametrized strings, unlimited parameters

  Code: Select all

  static const Widget _some_window_widgets[] = {
  {   WWT_TXTBTN, ..., ParamStr(STR_JUST_INT, IntParam(1)), String::Null}, // button with caption "1"
  {   WWT_TXTBTN, ..., ParamStr(STR_JUST_INT, IntParam(2)), String::Null}, // button with caption "2"
  {   WWT_TXTBTN, ..., ParamStr(STR_JUST_INT, IntParam(3)), String::Null}, // button with caption "3"
  /* constructor can take up to 3 parameters */
  {   WWT_TXTBTN, ..., ParamStr(STR_THREE_INTS, IntParam(1), IntParam(2), IntParam(3)), String::Null},
  /* if we want more, we can use << operator */
  {   WWT_TXTBTN, ..., ParamStr(STR_FIVE_INTS) << IntParam(1) << IntParam(2) << IntParam(3) 
                                << IntParam(4) << IntParam(5), String::Null},
  //...             

• positions of parameters, behaviour like values of enums

  Code: Select all

  class SomeWindow : public Window
  {
     SomeWindow(...) : Window(...)
     {
         ParamStr str = ParamStr(STR_SOME_TEXT); //"{NUM} {SKIP}{SKIP}{SKIP}{SKIP}  {NUM}{NUM}{NUM}  {SKIP}{SKIP}{SKIP}  {NUM}{NUM}"
  
         /* when parameter is the first and his position is ommited, he gest position 0 */
         str << IntParam(n1); 
         
         /* we can explicitly specify parameter position - position 4 */
         str << IntParam(n2, 4);        
         /* when position is ommited, parameter gets position behind previous parameter (in this case 5) */
         str << IntParam(n2); 
         /* next position - 6 */
         str << IntParam(n3); 
  
         /* position 10 */
         str << IntParam(n4, 10); 
         /* position 11 */
         str << IntParam(n5); 
         
         this->widgets[SOME_WIDGET].data = str;
         
  //...             

• change parameters at runtime

  Code: Select all

  class SomeWindow : public Window
  {
     ParamStr strA; //we must remember string whose parameters we want change
     ParamStr strB;
     
     int varA1, varA2;
     int varB1, varB2;  
     
     SomeWindow(...) : Window(...)
     {
         /* 'widget[WIDGET_TEXT_A].data' and 'strA' are not separated copies of string. 
          * Internally they are pointing to the same string and same set of parameters
          */
         this->widget[WIDGET_TEXT_A].data = strA = ParamStr(STR_TEXT_A, IntParam(1), RefParam(&varA1)); 
         this->widget[WIDGET_TEXT_B].data = strB = ParamStr(STR_TEXT_B, ConstStrParam("one"), RefParam(&varB1));
     }
     
     void SomeMethod()
     {
        strA[0] = IntParam(2);      // 1 changes to 2
        strA[1] = RefParam(&varA2); // varA1 reference (pointer) changes to varA2 reference
        strB[0] = ConstStrParam("two");  // "one" changes to "two"
        strB[1] = RefParam(&varB2); // varB1 reference (pointer) changes to varB2 reference
     }
  //...             

• Giving parameters to title of ShowQuery/ShowQueryString window was imposible

  Code: Select all

  // STR_TYPE_NEW_NAME_FOR_COMPANY - "Type new name for {STRING} company"
  
  ShowQueryString(//now parameters has a type of String, not StringID
     ParamString(STR_JUST_RAW_STRING, ConstStrParam(old_company_name)),
     ParamString(STR_TYPE_NEW_NAME_FOR_COMPANY, ConstStrParam(old_company_name)), 
     ...
  );  

• backward compatibility, parameters overlapping

  Code: Select all

  /* We can use strings in traditional way. 
   * Parameters of traditional strings can't overlap. When one string uses parameter number 1
   * (i.e. "blah blah {SKIP}{NUM} blah blah") no other string can use it. It produces long 
   * "{SKIP}{SKIP}{SK...." chaines. 
   * Parameters of traditional strings still can't overlap with parameters of new ParamString strings
   * (well not always, if we are sure that traditional strings will be evaluated before ParamString
   * strings their parameters may overlap), but if we use only ParamString strings their parameters
   * may overlap freely !
   */
  static const Widget _some_window_widgets[] = {
  /*       type, ..., data,                tooltips  */
  { WWT_MATRINX, ..., 0x501,               String::Null}, // still can use 'data' field as int16
  {    WWT_TEXT, ..., STR_SOME_TEXT,       String::Null}, // in this case String class is not used for 'data' field
  {    WWT_TEXT, ..., STR_SOME_OTHER_TEXT, STR_NULL},     // using String::Null and STR_NULL (casted to String) is the same
  //..
  
  class SomeWindow : public Window
  {
     virtual void OnPaint()
     {
        this->widget[SOME_WIDGET].data = STR_SOME_TEXT; // still can use traditional mechanics
        SetDParam(1, some_var1); // parameters must be set like in traditional system
        SetDParam(2, some_var2);
        SetDParam(3, some_var3);
        this->DrawWidgets();
     }
     
     void SomeMethod()
     {
        /* traditional form may be used */
        SetDParam(0, value);
        ShowQueryString(STR_JUST_INT, ...); //STR_JUST_INT is implicitly casted to String
     }
  //...             

[fonso]

This sounds like a great idea. I also think the string system is quite inflexible and could use an overhaul. Your proposal seems quite elegant. Some notes:

• A patch which shows those concepts as code, even if only prototypical, says more than a thousand words.
• Do you actually need that special constructor for ParamString? Does it have any benefits over operator<<?
• Do you really need a special class for strings without parameters? Does that have any benefits over just using ParamString and not assigning parameters? If it doesn't you can scrap two classes which makes your proposal quite a bit less complex.

[adf88]

A patch which shows those concepts as code, even if only prototypical, says more than a thousand words.

I've already started coding

Do you actually need that special constructor for ParamString? Does it have any benefits over operator<<?

The effect is the same. It costs very little so why not ? Parameters passed through constructor is more intuitive way.

Do you really need a special class for strings without parameters? Does that have any benefits over just using ParamString and not assigning parameters? If it doesn't you can scrap two classes which makes your proposal quite a bit less complex.

This is connected to memory management and the way that string decide if they need to be rebuilt or not. I planned that there will be 3 types of strings, each one will have it's own StringData derivative.
1) Raw strings (returned by static String::Raw method), strings without parameters. They are built only once at first use (or when language changes). All of them will be held in static std::map (keyed by StringID) to not duplicate them (if we create new string and its ID exist in the map only a new reference is taken).
2) Traditional strings created with String(StringID id) constructor (there must be a backward compatibility when casting StringID to String). They are rebuilt at every use (at every GetStringcall). Designed to use parameters set through traditional mechanism - SetDParam.
3) Strings with associated parameters to them - ParamString class. They are rebuilt at GetString call, but only if at least one of their parameters is changed.
Raw and traditional strings cant have parameters associated to them so they can be one class. But if I want joint String and ParamString classes into one I would have to do one of these:

• Use only one StringData class for every string type. This leads to memory and performance lost (strings will have unused fields, string type would have to be a field which entails switch(type) {...} in various calls), and worse readability/extensibility (we loosing object orientation).
• Replace StringData object to ParamStringData object when adding first parameter to string.

Some things will show up while implementing. I'll think about this.

Pre-implementation will be ready maybe even today (UTC).

PS. There is one more benefit of this system. Custom string derivatives i.e.

Code: Select all

BalanceString : base
{
//...
   virtual void RebuildString()
   {
      string_id = (value < 0) ? STR_RED_NUMBER : STR_BLACK_NUMBER;
      base::RebuildString();
   }
//...
}; 

Hmmm, when I invented this example another idea showed up - assign colour to strings at runtime

[Hirundo]

Hmmm, when I invented this example another idea showed up - assign colour to strings at runtime

This is already possible using the colour parameter of DrawString(...)

[adf88]

Custom drawing is a bypass on lower level, this is what I want to deal with.


Ok, pre-implementation is ready. Tell me what you think about this.
http://pastebin.com/f4cc38d61

[DaleStan]

http://pastebin.com/f4dcf6cd8

Is there some reason you didn't post a diff like a normal person would?

[adf88]

It's only portion of possible code for preview and discussion.

[planetmaker]

code preview in the forums is a bit limited. A diff helps with code (re-)viewing a lot. Just my 2ct.

[adf88]

Diff for this code would be useless. It's one totally new file, it's not even split between .cpp and .h. It compiles itself, but there is no way to run it.

If you want a full patch at least express your opinion. Is it worth of further work? For now I've seen only complaint "why not diff", and I don't know if anybody just looked on this code preview, or like new string system idea (except fonso, thanks for opinion).

[Thief^]

I've had a look, and it's a fairly nice system.

"Allways" should be "Always" though.

[DaleStan]

If you want a full patch at least express your opinion.

I already have expressed my opinion: You'd get more useful comments if you posted a diff.

Unlike TTDPatch[0], you cannot add a new feature to OpenTTD without modifying at least one existing file. Where is the diff for the file(s) you modified?

[0] Admittedly, it's not common, but it is possible. http://svn.ttdpatch.net/trac/changeset/1457

[Thief^]

He hasn't actually done any work to openttd yet, this is just a concept.

[adf88]

I made next step and started integrating new string system into ottd. Code was little buggy but now it's all right (I think so ). New strings are used now by drawing functions (DrawString family) and Widget.tooltip field.
Almost everything works fine Now I'm fighting with buffer underruns and overruns.

This is my current work (dirty), there is still at leas one bug to fix but you can take a look at new system in action.

string_class_r16308.diff

(38.97 KiB) Downloaded 99 times

For now I fixed problem caused by broken custom vsnprintf implementation written for VS and also inconsistent seprintf implementation.

This is the present vsnprintf implementation for VS:

Code: Select all

#ifdef _MSC_VER
/* *nprintf broken, not POSIX compliant, MSDN description
 * - If len < count, then len characters are stored in buffer, a null-terminator is appended, and len is returned.
 * - If len = count, then len characters are stored in buffer, no null-terminator is appended, and len is returned.
 * - If len > count, then count characters are stored in buffer, no null-terminator is appended, and a negative value is returned
 */
int CDECL vsnprintf(char *str, size_t size, const char *format, va_list ap)
{
    int ret;
    ret = _vsnprintf(str, size, format, ap);
    if (ret < 0 || ret == size) str[size - 1] = '\0';
    return ret;
}
#endif /* _MSC_VER */                   

Quick look at documentation:

• http://www.opengroup.org/onlinepubs/009695399/
  http://msdn.microsoft.com/en-us/library/1kt27hek.aspx

and I wrote implementation closer to POSIX standard:

Code: Select all

/**
 * Almost POSIX compliant implementation of \c vsnprintf for VC compiler.
 * The difference is value returned on output truncation. This implementation returns size.
 * POSIX implememntation returns size or more (the number of bytes that would be written to str
 * had size been sufficiently large excluding the terminating null byte).
 */
int CDECL vsnprintf(char *str, size_t size, const char *format, va_list ap)
{
    int ret;
    errno = 0;
    ret = _vsnprintf(str, size, format, ap);
    if (ret < 0) {
        if (errno != ERANGE) return ret; //if errror was different than insufficient buffer size then return -1
    } else if ((size_t)ret < size) {
        return ret; //if buffer is big enought return number of characers stored (excluding null)
    }
    /* if buffer is too small write null-terminator at its end and return size */
    str[size - 1] = '\0';
    return size;
} 

Second problem was inconsistent implementation of vseprintf:

Code: Select all

/**
 * Safer implementation of vsnprintf; same as vsnprintf except:
 * - last instead of size, i.e. replace sizeof with lastof.
 * - return gives the amount of characters added, not what it would add.
 * @param str    buffer to write to up to last
 * @param last   last character we may write to
 * @param format the formatting (see snprintf)
 * @param ap     the list of arguments for the format
 * @return the number of added characters
 */
static int CDECL vseprintf(char *str, const char *last, const char *format, va_list ap)
{
    if (str >= last) return 0;
    size_t size = last - str + 1;
    return min((int)size, vsnprintf(str, size, format, ap));
} 

On sufficient space it was returning number of characters stored excluding null-terminator, but on output truncation it was returning number of characters stored including null-terminator.

I'll post above bugs to bug-tracker, here's a diff:

fix_vseprintf_vsnprintf_r16308.diff

(2.37 KiB) Downloaded 97 times