SkOSMenu changes

[reed1, 2011-07-26 20:02:12]

http://codereview.appspot.com/4827042/diff/1/include/views/SkOSMenu.h
File include/views/SkOSMenu.h (right):

http://codereview.appspot.com/4827042/diff/1/include/views/SkOSMenu.h#newcode29
include/views/SkOSMenu.h:29: 
/**
 * Each of these has an associated value, which is stored in the event payload
for the item.
* Each type has a specific type for its value...
*     slider : scalar
*     textField : string
...

http://codereview.appspot.com/4827042/diff/1/include/views/SkOSMenu.h#newcode48
include/views/SkOSMenu.h:48: 
Why is this guy public?

http://codereview.appspot.com/4827042/diff/1/include/views/SkOSMenu.h#newcode51
include/views/SkOSMenu.h:51: private:
Should this be a SkString, so we can easily hold a copy from the caller?

http://codereview.appspot.com/4827042/diff/1/include/views/SkOSMenu.h#newcode72
include/views/SkOSMenu.h:72: const SkString* getTitle() const { return &fTitle;
}
either

const char* getTitle() const or getTitle(SkString*) const

Either way to we don't have to commit to using SkString internally

http://codereview.appspot.com/4827042/diff/1/include/views/SkOSMenu.h#newcode75
include/views/SkOSMenu.h:75: //NOTE: event must be dynamically allocated
Document what the return int means

http://codereview.appspot.com/4827042/diff/1/include/views/SkOSMenu.h#newcode75
include/views/SkOSMenu.h:75: //NOTE: event must be dynamically allocated
add slotName parameter, to name where the answer is stored?

http://codereview.appspot.com/4827042/diff/1/include/views/SkOSMenu.h#newcode88
include/views/SkOSMenu.h:88: 
Static functions are Capitalized

Document why it returns a bool

Is item name the same as my "slot name"?

[yangsu, 2011-07-26 23:00:34]

http://codereview.appspot.com/4827042/diff/1/include/views/SkOSMenu.h
File include/views/SkOSMenu.h (right):

http://codereview.appspot.com/4827042/diff/1/include/views/SkOSMenu.h#newcode29
include/views/SkOSMenu.h:29: 
On 2011/07/26 20:02:12, reed1 wrote:
> /**
>  * Each of these has an associated value, which is stored in the event payload
> for the item.
> * Each type has a specific type for its value...
> *     slider : scalar
> *     textField : string
> ...

Done.

http://codereview.appspot.com/4827042/diff/1/include/views/SkOSMenu.h#newcode48
include/views/SkOSMenu.h:48: 
Made private
On 2011/07/26 20:02:12, reed1 wrote:
> Why is this guy public?

http://codereview.appspot.com/4827042/diff/1/include/views/SkOSMenu.h#newcode51
include/views/SkOSMenu.h:51: private:
On 2011/07/26 20:02:12, reed1 wrote:
> Should this be a SkString, so we can easily hold a copy from the caller?

Done.

http://codereview.appspot.com/4827042/diff/1/include/views/SkOSMenu.h#newcode72
include/views/SkOSMenu.h:72: const SkString* getTitle() const { return &fTitle;
}
On 2011/07/26 20:02:12, reed1 wrote:
> either
> 
> const char* getTitle() const or getTitle(SkString*) const
> 
> Either way to we don't have to commit to using SkString internally

Done.

http://codereview.appspot.com/4827042/diff/1/include/views/SkOSMenu.h#newcode75
include/views/SkOSMenu.h:75: //NOTE: event must be dynamically allocated
On 2011/07/26 20:02:12, reed1 wrote:
> Document what the return int means

Done.

http://codereview.appspot.com/4827042/diff/1/include/views/SkOSMenu.h#newcode75
include/views/SkOSMenu.h:75: //NOTE: event must be dynamically allocated
On 2011/07/26 20:02:12, reed1 wrote:
> Document what the return int means

Done.

[reed1, 2011-07-27 14:36:17]

http://codereview.appspot.com/4827042/diff/6002/include/views/SkOSMenu.h
File include/views/SkOSMenu.h (right):

http://codereview.appspot.com/4827042/diff/6002/include/views/SkOSMenu.h#newc...
include/views/SkOSMenu.h:57: //Trigger event associated with the menu item
Where is the slot updated with the new value?
Why is "Matching" part of this name?

http://codereview.appspot.com/4827042/diff/6002/include/views/SkOSMenu.h#newc...
include/views/SkOSMenu.h:63: const char* getSlotName() const { return
fSlotName.c_str(); }
don't need 'const Type', just Type

http://codereview.appspot.com/4827042/diff/6002/src/views/SkOSMenu.cpp
File src/views/SkOSMenu.cpp (right):

http://codereview.appspot.com/4827042/diff/6002/src/views/SkOSMenu.cpp#newcode43
src/views/SkOSMenu.cpp:43: SkEvent* evt, SkEventSinkID target) {
'new Item' is sufficient, don't need the SkOSMenu:: if you don't want it.

http://codereview.appspot.com/4827042/diff/6002/src/views/SkOSMenu.cpp#newcode44
src/views/SkOSMenu.cpp:44: SkOSMenu::Item* item = new SkOSMenu::Item(label,
type, slotName, evt, target);
Another pattern for adding this is

*fItems.append() = new Item(....);

all in one line.

http://codereview.appspot.com/4827042/diff/6002/src/views/SkOSMenu.cpp#newcode45
src/views/SkOSMenu.cpp:45: fItems.append(1, &item);
Might be slightly clearer if we said

return item->fCmdID (or whatever its called), rather than referring to the
global (which might have been changed in another thread).

[yangsu, 2011-07-27 15:31:10]

http://codereview.appspot.com/4827042/diff/6002/include/views/SkOSMenu.h
File include/views/SkOSMenu.h (right):

http://codereview.appspot.com/4827042/diff/6002/include/views/SkOSMenu.h#newc...
include/views/SkOSMenu.h:57: //Trigger event associated with the menu item
On 2011/07/27 14:36:17, reed1 wrote:
> Where is the slot updated with the new value?
> Why is "Matching" part of this name?

Done.

http://codereview.appspot.com/4827042/diff/6002/include/views/SkOSMenu.h#newc...
include/views/SkOSMenu.h:63: const char* getSlotName() const { return
fSlotName.c_str(); }
On 2011/07/27 14:36:17, reed1 wrote:
> don't need 'const Type', just Type

Done.

http://codereview.appspot.com/4827042/diff/6002/src/views/SkOSMenu.cpp
File src/views/SkOSMenu.cpp (right):

http://codereview.appspot.com/4827042/diff/6002/src/views/SkOSMenu.cpp#newcode43
src/views/SkOSMenu.cpp:43: SkEvent* evt, SkEventSinkID target) {
On 2011/07/27 14:36:17, reed1 wrote:
> 'new Item' is sufficient, don't need the SkOSMenu:: if you don't want it.

Done.

http://codereview.appspot.com/4827042/diff/6002/src/views/SkOSMenu.cpp#newcode45
src/views/SkOSMenu.cpp:45: fItems.append(1, &item);
On 2011/07/27 14:36:17, reed1 wrote:
> Might be slightly clearer if we said
> 
> return item->fCmdID (or whatever its called), rather than referring to the
> global (which might have been changed in another thread).

Done.

[reed1, 2011-07-27 16:23:28]

LGTM