[haiku-bugs] Re: [Haiku] #7991: [DriveSetup] Add Text Control, in addition to size slider, to partition creation dialog

  • From: "jwlh172" <trac@xxxxxxxxxxxx>
  • Date: Sun, 18 Sep 2011 09:26:46 -0000

#7991: [DriveSetup] Add Text Control, in addition to size slider, to partition
creation dialog
---------------------------------------+----------------------------
   Reporter:  jwlh172                  |      Owner:  stippi
       Type:  enhancement              |     Status:  new
   Priority:  normal                   |  Milestone:  R1
  Component:  Applications/DriveSetup  |    Version:  R1/Development
 Resolution:                           |   Keywords:
 Blocked By:                           |   Blocking:
Has a Patch:  1                        |   Platform:  All
---------------------------------------+----------------------------

Comment (by jwlh172):

 Replying to [comment:2 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.

-- 
Ticket URL: <http://dev.haiku-os.org/ticket/7991#comment:4>
Haiku <http://dev.haiku-os.org>
Haiku - the operating system.

Other related posts:

  1. Home
  2. haiku-bugs
  3. Archive
  4. 09-2011