[DriveSetup] Add Text Control, in addition to size slider, to partition creation dialog

[jwlh172]

While specifying the sizes of my new partitions, I had difficulty getting the size exactly where I wanted it using the slider alone.

The requested enhancement is a text control, which will work together with the slider, so that either one may be used to specify the size of the new partition.

[stippi]

Thanks for the patch, it looks good in principle. A couple things you could improve:

• The BString should be named just "sizeString" in both places. "sSizeString" is a naming pattern we use for static variables.
• Even if it blows up the code a bit, I would declare the string in both case statements, surrounding them each with parenthesis. We prefer to declare variables where they are first needed. While not a problem in this example, I have seen switch statements that allocate any and all variables before the switch block, most of them never needed in any particular invokation of the switch, and it makes the code really messy and hard to follow and search for bugs.
• While the above two items are cosmetical, the next one is a real issue: The text control allows for arbitrary input by the user. The atoi() function may not be able to do anything with the string. So first of all, you should check the return value from the atoi() method. If it's invalid, or falls out of range, you should reject the input and reset the text field's contents.
• And to make this situation less likely to begin with, you can disable all but numerical chars on the BTextControl. It has a DisallowChar() method for that. There is a BeBook example, IIRC, and you could grep the code for other examples of how this can be used.

Thanks for your work!

[bonefish]

Replying to stippi:

• While the above two items are cosmetical, the next one is a real issue: The text control allows for arbitrary input by the user. The atoi() function may not be able to do anything with the string. So first of all, you should check the return value from the atoi() method. If it's invalid, or falls out of range, you should reject the input and reset the text field's contents.

There's ​strtol() which allows to check whether parsing the string actually worked (and to which character). atoi()'s return value in case of error is actually undefined.

[jwlh172]

Replying to stippi:

Thanks for the patch, it looks good in principle. A couple things you could improve:

• The BString should be named just "sizeString" in both places. "sSizeString" is a naming pattern we use for static variables.
• Even if it blows up the code a bit, I would declare the string in both case statements, surrounding them each with parenthesis. We prefer to declare variables where they are first needed. While not a problem in this example, I have seen switch statements that allocate any and all variables before the switch block, most of them never needed in any particular invokation of the switch, and it makes the code really messy and hard to follow and search for bugs.
• While the above two items are cosmetical, the next one is a real issue: The text control allows for arbitrary input by the user. The atoi() function may not be able to do anything with the string. So first of all, you should check the return value from the atoi() method. If it's invalid, or falls out of range, you should reject the input and reset the text field's contents.
• And to make this situation less likely to begin with, you can disable all but numerical chars on the BTextControl. It has a DisallowChar() method for that. There is a BeBook example, IIRC, and you could grep the code for other examples of how this can be used.

Thanks for your work!

Thanks for the feedback! I've made the changes you suggested and attached the updated patch.

[jscipione]

Replying to bonefish:

There's ​strtol() which allows to check whether parsing the string actually worked (and to which character). atoi()'s return value in case of error is actually undefined.

According to ​atoi() the function returns 0 on error, and either INT_MAX or INT_MIN on overflow. I see that the OP ignored your strtol() suggestion in the latest patch. I am trying to determine whether or not that is a problem. I believe that atoi is okay with the caveat that if you input some crazy not-number string then the slider value will get set to 0.

[stippi]

New patch looks fine. I just don't get what you are doing in case of invalid input from the text control. Wouldn't one just reset it to the slider value? It seems like you are just removing the last char from the text control. Anway, I will no attempt to apply the patch... :-)

[stippi]

I've applied your patch in hrev42759. I've refactored the method to update the text control and used that also when the user entered invalid input, but I didn't actually test this change. Please reopen if I managed to break things.